-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Avoid redundant property round-trips between platform and xplat across multiple controls #29970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Hey there @@bhavanesh2001! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Could your PR affect performance of creating a complex grid (i.e. #21787) please? |
I don’t think this will have any effect on load time or layouts. This mainly affects controls, especially when values are set from platform to xplat ,although, based on the CI results, things aren’t looking very good 😞. |
var specificity = GetContext(property).Values.GetSpecificity(); | ||
if (specificity == SetterSpecificity.FromHandler) | ||
{ | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephaneDelcroix
Currently, SetterSpecificity.FromHandler
is used in several places where the value is not actually coming from the handler, and this is blocking what we're trying to achieve here.
Here are the locations where it's used:
maui/src/Controls/src/Core/BindableObject.cs
Lines 288 to 289 in af4b90c
public void SetBinding(BindableProperty targetProperty, BindingBase binding) | |
=> SetBinding(targetProperty, binding, binding != null && targetProperty != null && binding.GetRealizedMode(targetProperty) == BindingMode.TwoWay ? SetterSpecificity.FromHandler : SetterSpecificity.FromBinding); |
maui/src/Controls/src/Core/BindableObjectExtensions.cs
Lines 27 to 28 in af4b90c
// support normal/code properties | |
self.SetValue(property, value, SetterSpecificity.FromHandler); |
Wouldn't it make more sense to introduce a separate, low-priority fallback specificity for these cases instead of overloading FromHandler
, which is intended for platform-set values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephaneDelcroix thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new name is very confusing. could you pick a good, self-explanatory one, so we know what this is supposed to do when we come back here next time ?
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
self.SetValue(property, value, SetterSpecificity.FromHandler); | ||
self.SetValue(property, value, SetterSpecificity.FromDontKnow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should be re-firing using the same that is in there? Why a new value when we could re-use the old one? Would that make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context can be null here, especially during initialization, so maybe we should stick to setting the specificity explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this either need a better name, or a ton of doc (I prefer the former)
oh, and some tests
public static readonly SetterSpecificity FromHandler = new SetterSpecificity(ExtrasHandler, 0, 0, 0, 0, 0, 0, 0); | ||
|
||
// New specificity, intended to behave like FromHandler | ||
public static readonly SetterSpecificity FromDontKnow = new SetterSpecificity(ExtrasDontKnow, 0, 0, 0, 0, 0, 0, 0); // New |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FromUnknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry about that, I just wanted to check the CI results quickly, so I went with the first name that came to mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephaneDelcroix I’ve moved the specificity changes into a separate PR: #30122
@@ -133,6 +143,7 @@ public SetterSpecificity(byte extras, ushort manual, byte isDynamicResource, byt | |||
// 6. Id 0x0000000000FF0000 | |||
// 7. Class 0x000000000000FF00 | |||
// 8. Type 0x00000000000000FF | |||
// 9. InternalFallback 0xFEFEFEFEFEFEFEFE // New |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it has the highest priority ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or a random value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my intention is for it to behave identically to FromHandler in terms of priority but still be distinguishable. The value 0xFEFEFEFEFEFEFEFE is just slightly below 0xFFFFFFFFFFFFFFFF (FromHandler)
var specificity = GetContext(property).Values.GetSpecificity(); | ||
if (specificity == SetterSpecificity.FromHandler) | ||
{ | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new name is very confusing. could you pick a good, self-explanatory one, so we know what this is supposed to do when we come back here next time ?
This reverts commit 0f9a218.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This is an follow up of #29887 , addressing the root issue that caused that issue.
The Problem
When the platform updates a property (e.g. user selects an item, types text, etc), the new value is propagated to the xplat property (
BindableProperty
) — which is correct.However, as part of that
BindableProperty
update, the system would re-map the value back to the platform view, causing a full unnecessary round-trip:Example (CollectionView):
SelectedItem
propertyThis unnecessary round-trip results in:
Controls/Properties fixed in this PR
Fix
For most controls, we already set
SetterSpecificity.FromHandler
when setting a property’s value. We can utilize this to skip the mapping and avoid the unnecessary round-trip.Follow-up Work
CollectionView
, the actual selection currently happens during the round-trip; this needs to be fixed.Switch.IsOn
still needs to be handled. Things are tangled withIsToggled
, which is invoking the mapper on its own.CollectionView.SelectedItems
requires special handling since it’s a collection and we are not reassigning a new value directly.Issues Fixed
Contributes to #22585