BindableLayout should dispatch layout changes when collection changes from background thread

[albyrock87]

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.

maui/src/Controls/src/Core/BindingExpression.cs

Line 717 in c4479e6

public void PropertyChanged(object sender, PropertyChangedEventArgs args)

maui/src/Controls/src/Core/BindingExpression.cs

Line 742 in c4479e6

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

[dotnet-policy-service]

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.

[mattleibow]

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...

[mattleibow]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[mattleibow]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[mattleibow]

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

[albyrock87]

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

@mattleibow how are discussions going?

I'm asking because collection view does this already:
https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/Handlers/Items/iOS/ObservableItemsSource.cs#L116

so why BindableLayout shouldn't do the same?

Besides scroll virtualization they have the same purpose: display a list of items.

[PureWeen]

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

@mattleibow how are discussions going?

I'm asking because collection view does this already: https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/Handlers/Items/iOS/ObservableItemsSource.cs#L116

so why BindableLayout shouldn't do the same?

Besides scroll virtualization they have the same purpose: display a list of items.

after chatting with @mattleibow about this one a bit. I don't think this is a rabbit hole we quite want to dive down. I tested WinUI and WinUI has the same restrictions, so, it's a common scenario that these changes aren't auto dispatched. I worry about developers spraying a bunch of AddItem calls which then fires off a bunch of separately queue'd Dispatches.

I get what the documentation says about BPs but I'd say that's still up for interpretation :-)

I wonder if there's a more configurable way we could make this work. If the BindableLayout actually had a handler we could do this via mappers.

Maybe we even just go boring with this for .NET 10 and add a configurable property
BindableLayout.DispatchCollectionChangedEventsToUIThreadIfTheyAreNeeded

@mattleibow thoughts?

[albyrock87]

@PureWeen thanks for discussing this.

I see your point, but can't we say the same thing about everything else?
What if a user changes a bunch of bound properties from a background thread?
That's also gonna cause N dispatches.

And what about the CollectionView, if BindableLayout shouldn't have auto dispatching, then the one in CollectionView should be removed for consistently, don't you think?

At this point, in my opinion it is better if MAUI handles dispatching automatically (as it is doing now on everything else) and then it's up to developers to tailor their code to avoid changing a lot of properties or adding many items on an observable collection from a background thread.

But if they don't, at least MAUI does what's needed to prevent a crash.

[albyrock87]

@PureWeen @mattleibow how's the situation with this one?

As I stated above, CollectionView does this, so I don't see why BindableLayout shouldn't do that too.
Otherwise we should remove that from CollectionView and see how many users complain about their app suddenly crashing - see the linked issue (#10918).

That said, I could hide this behind an app switch if you agree.
I'd like to put this in the next SR if possible.