Avoid redundant property round-trips between platform and xplat across multiple controls#29970
Avoid redundant property round-trips between platform and xplat across multiple controls#29970bhavanesh2001 wants to merge 6 commits into
Conversation
|
/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.
@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
maui/src/Controls/src/Core/BindableObjectExtensions.cs
Lines 27 to 28 in af4b90c
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.
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). |
| { | ||
| // support normal/code properties | ||
| self.SetValue(property, value, SetterSpecificity.FromHandler); | ||
| self.SetValue(property, value, SetterSpecificity.FromDontKnow); |
There was a problem hiding this comment.
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.
context can be null here, especially during initialization, so maybe we should stick to setting the specificity explicitly?
StephaneDelcroix
left a comment
There was a problem hiding this comment.
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.
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.
@StephaneDelcroix I’ve moved the specificity changes into a separate PR: #30122
| // 6. Id 0x0000000000FF0000 | ||
| // 7. Class 0x000000000000FF00 | ||
| // 8. Type 0x00000000000000FF | ||
| // 9. InternalFallback 0xFEFEFEFEFEFEFEFE // New |
There was a problem hiding this comment.
so it has the highest priority ?
There was a problem hiding this comment.
or a random value?
There was a problem hiding this comment.
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.
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.
|
@bhavanesh2001 are you still working on this one? :) |
#30122 needs to be reviewed/merged to move this forward |
|
Fixing the round trip for Switch is on, would fix the switch animation on iOS. maui-switch.movYou can see the difference with a native ios switch ios-switch.mov |
|
Closing as a stale one; Please create a new PR if this is still an issue |
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
BindablePropertyupdate, the system would re-map the value back to the platform view, causing a full unnecessary round-trip:Example (CollectionView):
SelectedItempropertyThis unnecessary round-trip results in:
Controls/Properties fixed in this PR
Fix
For most controls, we already set
SetterSpecificity.FromHandlerwhen 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.IsOnstill needs to be handled. Things are tangled withIsToggled, which is invoking the mapper on its own.CollectionView.SelectedItemsrequires special handling since it’s a collection and we are not reassigning a new value directly.Issues Fixed
Contributes to #22585