Skip to content

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bhavanesh2001
Copy link
Contributor

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):

  1. User selects an item on native platform view
  2. The native handler updates the xplat SelectedItem property
  3. PropertyChanged callback fires, re-applies the value back to the platform view
  4. Native view updates again (even though it's already in the correct state)

This unnecessary round-trip results in:

  • Redundant platform calls
  • Extra property mapping
  • Possible race conditions during fast user interactions

Controls/Properties fixed in this PR

Note: These are the properties I have observed so far; there may be others as well.

Control Property Fixed in this?
CollectionView (iOS, Windows) SelectedItem
CollectionView (Android) SelectedItem
CollectionView SelectedItems
CheckBox IsChecked
DatePicker Date
Editor Text
Entry Text
Picker SelectedIndex
RadioButton IsChecked
SearchBar Text
Slider Value
Stepper Value
Switch IsOn
TimePicker Time

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

  • In Android 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 with IsToggled, 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

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jun 12, 2025
Copy link
Contributor

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.

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX
Copy link
Contributor

MartyIX commented Jun 13, 2025

Could your PR affect performance of creating a complex grid (i.e. #21787) please?

@bhavanesh2001
Copy link
Contributor Author

bhavanesh2001 commented Jun 15, 2025

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;
Copy link
Contributor Author

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:

public void SetBinding(BindableProperty targetProperty, BindingBase binding)
=> SetBinding(targetProperty, binding, binding != null && targetProperty != null && binding.GetRealizedMode(targetProperty) == BindingMode.TwoWay ? SetterSpecificity.FromHandler : SetterSpecificity.FromBinding);

// 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StephaneDelcroix thoughts?

Copy link
Contributor

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 ?

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

self.SetValue(property, value, SetterSpecificity.FromHandler);
self.SetValue(property, value, SetterSpecificity.FromDontKnow);
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor

@StephaneDelcroix StephaneDelcroix left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FromUnknown

Copy link
Contributor Author

@bhavanesh2001 bhavanesh2001 Jun 19, 2025

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

Copy link
Contributor Author

@bhavanesh2001 bhavanesh2001 Jun 22, 2025

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
Copy link
Contributor

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or a random value?

Copy link
Contributor Author

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;
Copy link
Contributor

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community ✨ Community Contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants