Sergio0694 commented 4 years ago

Proposal: implicit DispatcherQueue support for x:Bind

Summary

Add automatic (possibly optional) dispatching to the DispatcherQueue instance for the current UI thread in the _BindingsTracking type and to the necessary internal WinUI paths (eg. to deal with collection changed UI updates), specifically in the PropertyChanged event handler and the CollectionChanged. This would completely reverse the chain of responsability for proper thread dispatching from the viewmodel side to the UI side, and make the whole thing much more resilient and flexible. A similar concept would apply to other events handled by code within WinUI.

The core idea is this: it should be responsability of an event handler to dispatch to whatever synchronization context it needs, not to the object raising the event. Specifically because if you have multiple listeners on different threads, the latter simply will never work anyway. And in general, objects raising events shouldn't need to account for who's listening to them.

Details

Note: what follows is just the details for the PropertyChanged event specifically, not the others. I'll describe this one since it's more "easily" addressed by looking at the x:Bind codegen, whereas the others would likely need some tweaks in the code within WinUI dealing with those events. The idea in general is still the same though: WinUI as a framework should automatically deal with dispatching when handling notification events tied to the UI.

Handling the synchronization context for the PropertyChanged event in viewmodels has always been a major pain point when working with MVVM. There have been a variety of different solutions proposed, but each with their shortcomings, namely:

Lots of overhead in the viewmodels to deal with dispatching to the "UI thread". This also breaks modularity as a viewmodel should conceptually just be a .NET Standard and portable component, where the term "UI thread" has no actual meaning. This should be purely a UI-related aspect that the viewmodels should not really be concerned with.
Single-context solutions involve capturing a synchronization context from the viewmodel and dispatching there when raising the PropertyChanged event. Again this has two issues: for one it's not really flexible as it wouldn't work in case the same observable object is being displayed in different ways across more than a single window (where two different UI threads would be used), and the second issue is that it would inject purely platform dependent code back into a more abstract layer, i.e. the viewmodel.
Even when using dependency injection to keep the viewmodels technically platform agnostic, this still adds a lot of potentially unnecessary overhead and complexity to what should be a pretty simple implementation of the INotifyPropertyChanged interface, plus again I'd argue that conceptually the UI thread dispatching should only be relegated to the actual platform dependent code. From the point of view of a .NET Standard component, individual threads should (generally) not matter much.

The proposed solution is to completely flip this over and add a simple extension to the codegen for x:Bind (pinging @MikeHillberg about this) to allow for a more general solution to this issue, built right into the framework itself. The advantages here would be multiple, as mentioned above as well:

Less overhead and complexity in the viewmodels, and increased code modularity 🚀
Less error prone code ✨
More flexible results, with supports for an arbitrary number of receiving UI threads 🧰

Implementation

Consider this simple viewmodel (not implemented, it doesn't matter here):

public class MainPageViewModel : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler? PropertyChanged;

    public string Text { get; set; }
}

And this XAML snippet:

<Page
    x:Class="XBindSample.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:XBindSample">
    <Page.DataContext>
        <local:MainPageViewModel x:Name="ViewModel"/>
    </Page.DataContext>

    <Grid>
        <TextBlock Text="{x:Bind ViewModel.Text, Mode=OneWay}"/>
    </Grid>
</Page>

If we build this and go to inspect the generated MainPage.g.cs file, we'll find a number of classes with various responsabilities - from updating individual UI controls to tracking the bindings, etc. In particular, we're interested in this one:

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Windows.UI.Xaml.Build.Tasks"," 10.0.19041.1")]
[global::System.Diagnostics.DebuggerNonUserCodeAttribute()]
private class MainPage_obj1_BindingsTracking
{
    private global::System.WeakReference<MainPage_obj1_Bindings> weakRefToBindingObj; 

    public MainPage_obj1_BindingsTracking(MainPage_obj1_Bindings obj)
    {
        weakRefToBindingObj = new global::System.WeakReference<MainPage_obj1_Bindings>(obj);
    }

    public MainPage_obj1_Bindings TryGetBindingObject()
    {
        return null; // Removed for brevity
    }

    public void ReleaseAllListeners()
    {
        // Removed for brevity
    }

    public void PropertyChanged_ViewModel(object sender, global::System.ComponentModel.PropertyChangedEventArgs e)
    {
        MainPage_obj1_Bindings bindings = TryGetBindingObject();
        if (bindings != null)
        {
            string propName = e.PropertyName;
            global::XBindSynchronizationContextSample.MainPageViewModel obj = sender as global::XBindSynchronizationContextSample.MainPageViewModel;
            if (global::System.String.IsNullOrEmpty(propName))
            {
                if (obj != null)
                {
                    bindings.Update_ViewModel_Text(obj.Text, DATA_CHANGED);
                }
            }
            else
            {
                switch (propName)
                {
                    case "Text":
                    {
                        if (obj != null)
                        {
                            bindings.Update_ViewModel_Text(obj.Text, DATA_CHANGED);
                        }
                        break;
                    }
                    default:
                        break;
                }
            }
        }
    }
    private global::XBindSynchronizationContextSample.MainPageViewModel cache_ViewModel = null;
    public void UpdateChildListeners_ViewModel(global::XBindSynchronizationContextSample.MainPageViewModel obj)
    {
        if (obj != cache_ViewModel)
        {
            if (cache_ViewModel != null)
            {
                ((global::System.ComponentModel.INotifyPropertyChanged)cache_ViewModel).PropertyChanged -= PropertyChanged_ViewModel;
                cache_ViewModel = null;
            }
            if (obj != null)
            {
                cache_ViewModel = obj;
                ((global::System.ComponentModel.INotifyPropertyChanged)obj).PropertyChanged += PropertyChanged_ViewModel;
            }
        }
    }
}

This is the type that actually subscribes to the PropertyChanged event in our viewmodel and goes about updating the UI components. This is the only part that should need to care about the "UI thread", and this is the only part that should include the code to automatically deal with this for the user, both because it'd make all the rest of the code much simpler, and also because injecting the change here would be pretty efficient and with a small code change required.

In particular, the issue is when the PropertyChanged_ViewModel handler is invoked, since that could be done from an other thread if the PropertyChanged event was raised on another thread. To fix this, I propose the following:

private global::System.WeakReference<MainPage_obj1_Bindings> weakRefToBindingObj;
private readonly DispatcherQueue dispatcherQueue;

public MainPage_obj1_BindingsTracking(MainPage_obj1_Bindings obj)
{
    weakRefToBindingObj = new global::System.WeakReference<MainPage_obj1_Bindings>(obj);
    dispatcherQueue = DispatcherQueue.GetForCurrentThread();
}

public void PropertyChanged_ViewModel(object obj, global::System.ComponentModel.PropertyChangedEventArgs e)
{
    if (dispatcherQueue.HasThreadAccess)
    {
        PropertyChanged_ViewModel_OnDispatcherQueue(obj, e);
    }
    else
    {
        PropertyChanged_ViewModel_Dispatch(obj, e);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
private void PropertyChanged_ViewModel_Dispatch(Object obj, global::System.ComponentModel.PropertyChangedEventArgs e)
{
    _ = dispatcherQueue.TryEnqueue(() => PropertyChanged_ViewModel_OnDispatcherQueue(obj, e));
}

private void PropertyChanged_ViewModel_OnDispatcherQueue(object sender, global::System.ComponentModel.PropertyChangedEventArgs e)
{
    MainPage_obj1_Bindings bindings = TryGetBindingObject();
    if (bindings != null)
    {
        string propName = e.PropertyName;
        global::XBindSynchronizationContextSample.MainPageViewModel obj = sender as global::XBindSynchronizationContextSample.MainPageViewModel;
        if (global::System.String.IsNullOrEmpty(propName))
        {
            if (obj != null)
            {
                bindings.Update_ViewModel_Text(obj.Text, DATA_CHANGED);
            }
        }
        else
        {
            switch (propName)
            {
                case "Text":
                    {
                        if (obj != null)
                        {
                            bindings.Update_ViewModel_Text(obj.Text, DATA_CHANGED);
                        }
                        break;
                    }
                default:
                    break;
            }
        }
    }
}

Here's the git diff:

+using Windows.System;
+
 namespace XBindSynchronizationContextSample
 {
     partial class MainPage :
@@ -170,11 +172,13 @@ namespace XBindSynchronizationContextSample
             [global::System.Diagnostics.DebuggerNonUserCodeAttribute()]
             private class MainPage_obj1_BindingsTracking
             {
+                private readonly DispatcherQueue dispatcherQueue;

                 public MainPage_obj1_BindingsTracking(MainPage_obj1_Bindings obj)
                 {
                     weakRefToBindingObj = new global::System.WeakReference<MainPage_obj1_Bindings>(obj);
+                    dispatcherQueue = DispatcherQueue.GetForCurrentThread();
                 }

                 public MainPage_obj1_Bindings TryGetBindingObject()
@@ -198,6 +202,17 @@ namespace XBindSynchronizationContextSample
                 }

                 public void PropertyChanged_ViewModel(object sender, global::System.ComponentModel.PropertyChangedEventArgs e)
+                {
+                    if (dispatcherQueue.HasThreadAccess)
+                    {
+                        PropertyChanged_ViewModel_OnDispatcherQueue(sender, e);
+                    }
+                    else
+                    {
+                        PropertyChanged_ViewModel_Dispatch(obj, e);
+                    }
+                }
+
+                [MethodImpl(MethodImplOptions.NoInlining)]
+                private void PropertyChanged_ViewModel_Dispatch(Object obj, global::System.ComponentModel.PropertyChangedEventArgs e)
+                {
+                    _ = dispatcherQueue.TryEnqueue(() => PropertyChanged_ViewModel_OnDispatcherQueue(obj, e));
+                }
+
+                public void PropertyChanged_ViewModel_OnDispatcherQueue(object sender, global::System.ComponentModel.PropertyChangedEventArgs e)

Now, this is just a proof of concept, but hopefully it illustrates the concept well enough 😄

Rationale

Offer a new, modern and flexible handling of UI thread dispatching on WinUI
Make working with multiple windows much easier and less error prone
Greatly reduce the complexity on the backend side and increase the code modularity
Address a major pain point in MVVM/similar that has been there for years, with a built-in solution

Scope

Capability	Priority
Update the `x:Bind` codegen to handle the UI thread dispatch	Must
Update internal handlers to handle UI thread dispatch (eg. `CollectionChanged`)	Must
Introduce changes that would break existing codebases	Won't
Introduce overhead in current codebases or when the dispatch is not needed	Won't

StephenLPeters commented 4 years ago

@marb2000 FYI

BreeceW commented 4 years ago

In my scenario, I have a web socket client that is shared across multiple windows. I see two options for displaying content I get from that connection: I could keep redundant instances of my models in the page associated with the window and update them on the page’s dispatcher through events fired by web socket messages, or I could share all the models throughout the windows. I would rather do the second, as it would be much simpler and keep everything consolidated. But here lies the problem—I want to bind my UI to those models.

Here’s what happens:

A web socket message is received and updates a property on a model
The app crashes because it is running on the wrong UI thread.

That was expected (but unwelcome). I don’t want my web socket class to have a concept of a UI thread, though. But what if I run the change on the Dispatcher of the page, anyway?

A web socket message is received and updates a property on a model
My web socket class fires an event that runs an action on the UI thread using Dispatcher.RunAsync

Great. The UI has been updated. But now I open another window, and as soon as the web socket receives a new message, my app will crash. What happened?

A web socket message is received and updates a property on a model
My web socket class fires an event that runs an action on the UI thread using Dispatcher.RunAsync
The app crashes because that dispatcher is trying to update the UI of the other window (different thread) at the same time

None of this is ideal (and I have no solution at all for binding to the same object from different windows), and I very much support changing the generated code to run property changes on the correct thread without having to manually call anything. That would solve my two problems: the improper separation of concerns (my web socket class should not care whether there is a UI thread or not) and the inability to update a property bound to UI on different threads.

maxkatz6 commented 4 years ago

For consistency I would expect same behaviour with old-style Binding as well.

Sergio0694 commented 4 years ago

Thank you for chiming in @BreeceW! Yes that's exactly one of the reasons why having this functionality be built-in would be great, and why handling the "UI thread" on the backend side (like some MVVM libraries do) is not only impractical and breaking the framework-agnostic architecture, but also still not a proper solutions if you have in fact more than one UI thread 👍

Just spotted a minor codegen issue in my original proposal, updating it now 😄

This part:

if (dispatcherQueue.HasThreadAccess)
{
    PropertyChanged_ViewModel_OnDispatcherQueue(sender, e);
}
else
{
    _ = dispatcherQueue.TryEnqueue(() => PropertyChanged_ViewModel_OnDispatcherQueue(sender, e));
}

Actually causes the C# compiler to always allocate the closure class used in the second branch, even when we don't actually need it at all. This would introduce unnecessary allocations even when the dispatch is not actually done, see:

<>c__DisplayClass1_0 <>c__DisplayClass1_ = new <>c__DisplayClass1_0();
<>c__DisplayClass1_.<>4__this = this;
<>c__DisplayClass1_.obj = obj;
<>c__DisplayClass1_.e = e;
if (dispatcherQueue.HasThreadAccess)
{
    PropertyChanged_ViewModel_OnDispatcherQueue(<>c__DisplayClass1_.obj, <>c__DisplayClass1_.e);
}
else
{
    dispatcherQueue.TryEnqueue(new Action(<>c__DisplayClass1_.<PropertyChanged_ViewModel>b__0));
}

The simple solution is just to move the dispatch in a separate, non-inlined method, like so:

public void PropertyChanged_ViewModel(object obj, global::System.ComponentModel.PropertyChangedEventArgs e)
{
    if (dispatcherQueue.HasThreadAccess) PropertyChanged_ViewModel_OnDispatcherQueue(obj, e);
    else PropertyChanged_ViewModel_Dispatch(obj, e);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private void PropertyChanged_ViewModel_Dispatch(Object obj, global::System.ComponentModel.PropertyChangedEventArgs e)
{
    _ = dispatcherQueue.TryEnqueue(() => PropertyChanged_ViewModel_OnDispatcherQueue(obj, e));
}

Which results in the correct codegen here:

public void PropertyChanged_ViewModel(object obj, PropertyChangedEventArgs e)
{
    if (dispatcherQueue.HasThreadAccess)
    {
        PropertyChanged_ViewModel_OnDispatcherQueue(obj, e);
    }
    else
    {
        PropertyChanged_ViewModel_Dispatch(obj, e);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
private void PropertyChanged_ViewModel_Dispatch(object obj, PropertyChangedEventArgs e)
{
    <>c__DisplayClass2_0 <>c__DisplayClass2_ = new <>c__DisplayClass2_0();
    <>c__DisplayClass2_.<>4__this = this;
    <>c__DisplayClass2_.obj = obj;
    <>c__DisplayClass2_.e = e;
    dispatcherQueue.TryEnqueue(new Action(<>c__DisplayClass2_.<PropertyChanged_ViewModel_Dispatch>b__0));
}

Where the overhead/allocation is only paid if we do need to dispatch (which is expected anyway). In all cases where we're already on the right thread, the performance would be just like before 🚀

JeanRoca commented 3 years ago

Thank you for this @Sergio0694. We would be able to look into this more post WinUI 3.0. I will leave this open for now.

kmgallahan commented 2 years ago

As this would make every developer's life easier and it seems straightforward to implement, can we create a list a list of reasons why it can't or shouldn't be done ASAP?

We have entered that "post WinUI 3.0" phase after all...

@Sergio0694 @andrewleader @ryandemopoulos

Sergio0694 commented 2 years ago

"can we create a list a list of reasons why it can't or shouldn't be done ASAP?"

I can list a few off the top of my head:

The code example you see here would only work for INotifyPropertyChanged. You'd need to also adapt it to support INotifyCollectionChanged for it to be consistent. Still doable, but it's a bit more work.
This would technically be a breaking change (as cases that would've crashed before would now work). You'd need to either document this, or provide a switch for developer to opt-in into this new behavior, which means likely new sintax, which is again more work.
You'd then also need to investigate whether, to make this consistent, it should also apply to other related interfaces that have direct support via generated code. Namely, I'm thinking of INotifyDataErrorInfo.

Not saying this means it shouldn't be done - I do think this has been a major issue with the platform since WPF first came out, and I do think it'd be worth it to make this change. Just saying it's still a fair amount of work and not just a 5 minutes commit 😄

michael-hawker commented 2 years ago

🦙 Link back to similar issue https://github.com/microsoft/microsoft-ui-xaml/issues/729

michael-hawker commented 1 year ago

Bumping, though think this is a feature request, based on https://github.com/microsoft/microsoft-ui-xaml/discussions/8638

Johno-ACSLive commented 5 months ago

Needing this capability as well, @Scottj1s or @Sergio0694 this issue has been open for a number of years. Will this be in WASDK 1.6? If not, when can this be implemented.

microsoft / microsoft-ui-xaml