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

BindableLayout should dispatch layout changes when collection changes from background thread #24714

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Sep 11, 2024

Description of Change

Quoting https://learn.microsoft.com/en-us/dotnet/maui/fundamentals/data-binding/?view=net-maui-8.0

.NET MAUI marshals binding updates to the UI thread. When using MVVM this enables you to update data-bound viewmodel properties from any thread, with .NET MAUI's binding engine bringing the updates to the UI thread.

In MAUI we're used to Binding marshaling, so when doing BindableLayout.ItemsSource={Binding MyCollection} we give for granted this will be automatically marshaled to the main thread which is true for INotifyPropertyChanged event.

public void PropertyChanged(object sender, PropertyChangedEventArgs args)

obj.Dispatcher.DispatchIfRequired(() => _expression.Apply());

INotifyCollectionChanged (like INotifyPropertyChanged) should be also taken into account in this context.

This is the main cause of the issue #10918 reported a while ago.

This PR introduces the use of IDispatcher when reacting to collection changed event.

Issues Fixed

Fixes #10918

@albyrock87 albyrock87 requested a review from a team as a code owner September 11, 2024 12:03
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 11, 2024
Copy link
Contributor

Hey there @albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

This looks good. I wonder why we never dispatched before.

@PureWeen not sure if this rings any bells. I recall EZ was always sad we did dispatching for some things and we should have always done something different. But I may be mixing multiple thoughts here...

src/Controls/src/Core/BindableLayout/BindableLayout.cs Outdated Show resolved Hide resolved
src/Controls/src/Core/BindableLayout/BindableLayout.cs Outdated Show resolved Hide resolved
@mattleibow mattleibow added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Sep 11, 2024
@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow added the do-not-merge Don't merge this PR label Sep 11, 2024
@mattleibow
Copy link
Member

Adding the do not merge as I believe this is still undergoing discussions.

@PureWeen PureWeen marked this pull request as draft September 11, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter community ✨ Community Contribution do-not-merge Don't merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to convert Microsoft.Maui.Handlers.LayoutHandler to UIKit.UIView
2 participants