Skip to content
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

Shift Handler?.UpdateValue to happen after all PropertyChanged events have fired #5587

Closed
PureWeen opened this issue Mar 25, 2022 · 3 comments
Labels
area-core-lifecycle XPlat and Native UIApplicationDelegate/Activity/Window lifecycle events fixed-in-9.0.0-preview.7.24407.4 t/bug Something isn't working
Milestone

Comments

@PureWeen
Copy link
Member

PureWeen commented Mar 25, 2022

Currently our Handler?.UpdateValue call fires during the top call here from BindableObject

image

This means that sometimes the Handler will have a premature vision of the world. The Handler update should fire after as many of the PropertyChanged events have settled as possible.

For example it leads to issues like this

#5586

The Parent on the FormattedText is set inside the BP.OnPropertyChanged which fires after the Handler?.UpdateValue fires. So anything the BP.OnPropertyChanged changes will now have to re-propagate.

@PureWeen PureWeen added this to the 6.0.300-rc.2 milestone Mar 25, 2022
@PureWeen PureWeen added the s/verified Verified / Reproducible Issue ready for Engineering Triage label Mar 25, 2022
@antonfirsov antonfirsov self-assigned this Mar 28, 2022
@antonfirsov
Copy link
Member

antonfirsov commented Mar 28, 2022

@PureWeen should we consider bringing other events which fire in OnPropertyChanged after property.PropertyChanged or are we fine with a minimal solution that pushes Handler?.UpdateValue after all the other events?

Is this issue mostly about validation whether such a minimal change breaks something?


PS: Actually, firing Handler?.UpdateValue outside of OnPropertyChanged may lead to side-effects, since derived custom properties are also calling into OnPropertyChanged and may expect Handler?.UpdateValue to execute as an effect. Should we just exchange the order of property.PropertyChanged and OnPropertyChanged ?

@jfversluis jfversluis added the area-core-lifecycle XPlat and Native UIApplicationDelegate/Activity/Window lifecycle events label Mar 30, 2022
@Redth Redth modified the milestones: 6.0.300-rc.2, 6.0.300-rc.3 Apr 20, 2022
@PureWeen PureWeen modified the milestones: 6.0.300-rc.3, .NET 7 Apr 25, 2022
@Redth Redth modified the milestones: .NET 7 + Servicing, Backlog Aug 29, 2022
@ghost
Copy link

ghost commented Aug 29, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@samhouts samhouts removed the s/verified Verified / Reproducible Issue ready for Engineering Triage label Apr 5, 2023
@samhouts samhouts modified the milestones: Backlog, Under Consideration Jul 26, 2023
@samhouts samhouts added the t/bug Something isn't working label Jul 31, 2023
@PureWeen PureWeen modified the milestones: Under Consideration, .NET 9 Planning May 15, 2024
@PureWeen
Copy link
Member Author

Related
#22347

I think we could clean up 22347 once we conceive of how we want this to work.

We should probably move the handler update code to fire about here

The difficulty comes with maintaining behavior when users manually call OnPropertyChanged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-core-lifecycle XPlat and Native UIApplicationDelegate/Activity/Window lifecycle events fixed-in-9.0.0-preview.7.24407.4 t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants