Control Binding should use Invoke

[zeh-almeida]

Is your feature request related to a problem? Please describe

I have a WinForms application which uses CommunityToolkit.Mvvm for bindings.
I am also using the same MVVM library to build a MAUI application for study purposes.

MVVM allows me to have asynchronous commands and messages, which I use to propagate changes across models.
However, because my form binds some controls to those models, I have started to receive UI Thread exceptions when updating values.

After a lot of debugging, I saw that the SetPropValue method in the Bindings class does not use the Invoke method when updating values:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Binding.cs

Line 956 in 77d01dd

private void SetPropValue(object value)

Describe the solution you'd like and alternatives you've considered

It is my understanding that the Control property is available for the Binding object:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Binding.cs

Line 110 in 77d01dd

public Control Control => BindableComponent as Control;

In this case, I propose that the SetPropValue method checks if the Control property is not null and tries to update it's value through an execution of the Control.Invoke method, to ensure the value change occurs in the UI Thread.

Will this feature affect UI controls?

I do not think so because the underlining flow is kept as is.

[elachlan]

@KlausLoeffelmann would be the best bet to answer this as he has investigated Async functionality in Winforms.

Some what related:
#4631
#5917
#5918

[KlausLoeffelmann]

That's definitely something we should look into, but also very cautiously, as it could potentially break existing scenarios if we introduced it unconditionally. So, if we consider it (and we should), we should think of an opt-in mechanism.

[elachlan]

@KlausLoeffelmann what about an AsyncBinding class? Effectively it just calls Control.Invoke on any control operations.

Otherwise, we can check Control.InvokeRequired and call Control.Invoke if needed?
https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.control.invokerequired?view=windowsdesktop-7.0

Reference:
https://stackoverflow.com/a/5143652/908889

[zeh-almeida]

I probably should open a PR for this but I can't pass some Interop tests locally and I can't figure out why.

I did some changes to the Binding class.

First, I added this attribute:
private bool _invokeControl;

Then, I added a new constructor:

public Binding(string propertyName, object dataSource, string dataMember, bool formattingEnabled, DataSourceUpdateMode dataSourceUpdateMode, object nullValue, string formatString, IFormatProvider formatInfo, bool invokeControl)
        {
            DataSource = dataSource;
            BindingMemberInfo = new BindingMemberInfo(dataMember);
            _bindToObject = new BindToObject(this);

            PropertyName = propertyName;
            _formattingEnabled = formattingEnabled;
            _formatString = formatString;
            _nullValue = nullValue;
            _formatInfo = formatInfo;
            DataSourceUpdateMode = dataSourceUpdateMode;
            _invokeControl = invokeControl;

            CheckBinding();
        }

I also added a private method to check for the control data:

        private void SetPropValue(PropertyDescriptor property, IBindableComponent component, object value)
        {
            // If possible, we should use Invoke in order to work with the UI Thread
            if (Control is not null && Control.InvokeRequired && _invokeControl)
            {
                Control.Invoke(() => property.SetValue(component, value));
            }
            else
            {
                property.SetValue(component, value);
            }
        }

And lastly, I made changes to the SetPropValue method:

        private void SetPropValue(object value)
        {
            // we will not pull the data from the back end into the control
            // when the control is in design time. this is because if we bind a boolean property on a control
            // to a row that is full of DBNulls then we cause problems in the shell.
            if (ControlAtDesignTime())
            {
                return;
            }

            _inSetPropValue = true;

            try
            {
                bool isNull = value is null || Formatter.IsNullData(value, DataSourceNullValue);
                if (isNull)
                {
                    if (_propIsNullInfo is not null)
                    {
                        //_propIsNullInfo.SetValue(BindableComponent, true);
                        SetPropValue(_propIsNullInfo, BindableComponent, true);
                    }
                    else if (_propInfo is not null)
                    {
                        if (_propInfo.PropertyType == typeof(object))
                        {
                            //_propInfo.SetValue(BindableComponent, DataSourceNullValue);
                            SetPropValue(_propInfo, BindableComponent, DataSourceNullValue);
                        }
                        else
                        {
                            //_propInfo.SetValue(BindableComponent, null);
                            SetPropValue(_propInfo, BindableComponent, null);
                        }
                    }
                }
                else
                {
                    //_propInfo.SetValue(BindableComponent, value);
                    SetPropValue(_propInfo, BindableComponent, value);
                }
            }
            finally
            {
                _inSetPropValue = false;
            }
        }

Like I said, I couldn't really pass the automated tests but I believe I am at least in the right direction.

Does this help you in any way @KlausLoeffelmann ?

[KlausLoeffelmann]

I probably should open a PR for this but I can't pass some Interop tests locally and I can't figure out why.

You could open a PR anyway and make it Draft for now.

It's a great initiative! -
(My only problem is immediate time to spare on this, to be honest.)

I'll take a look as soon as I can spend a few hours on this - it might take a while, though. In the context of the latest changes in Command Binding, we also got a few pings on Converters for Binding scenarios, so I would need to wrap my head around that as well.

https://devblogs.microsoft.com/dotnet/winforms-cross-platform-dotnet-maui-command-binding/

[KlausLoeffelmann]

One additional thought:
What about exception handling?

What would happen in this case (or what would you expect to happen), if the setter of that property caused an exception?

[zeh-almeida]

I don't think the Invoke method does anything other than bubble it up and the current code doesn't seem to handle it either, so it would bubble up as well.

That's also what I would expect in this case, since this should be setting a value, so it should "just work".

Of course, you may have commands, events and everything else tied to this property but even if you do, it's better to handle exceptions yourself in those cases, right?

[elachlan]

@zeh-almeida could you put in a draft PR so we can investigate the test failures?

[zeh-almeida]

~~Yeah, sure! Will do as soon as possible.~~ @elachlan PR opened. Still have to write tests, though.

[merriemcgaw]

@zeh-almeida, when you're ready to do the official API Review Process you can just edit your first comment to follow their pattern. I would definitely want @KlausLoeffelmann to sign off on a change like this before bringing to the review board.

[zeh-almeida]

@merriemcgaw I created an API Proposal following the template and have linked both this issue and the PR as well.

Thank you all for the support, I am excited to contribute to the project!

Forgot to ask: Should I publish my PR now? It is still in draft.

[KlausLoeffelmann]

Wait for it for a little while.
I am busy the next days with some other urgent tasks, but I try to get to it this week, and take a look!

[KlausLoeffelmann]

Also please note my comment here:
#8582 (comment)

In addition: It would be also nice to have a small WinForms demo app for it, based on the API change of the branch, we're we could quickly test scenarios and experiment with it? Maybe put that in a public GitHub repo and link to it, if you got the time to set that up?

[zeh-almeida]

@KlausLoeffelmann I do have a scenario for it but using current library version.
I'll try to create a version of it using the proposed fix I have.

Here's the link to the commit with the code:
zeh-almeida/6502-sharp@6216195
(Also, shameless plug to my project, why not?)

You can also see a emerging MAUI app using the same infrastructure, they are all using async commands.
In the master branch, you'll see that I had to change the bindings manually to avoid the problem which originated this whole thing.

Regarding your comment: I am not sure I understood correctly, those changes you propose are not directly related to the issue, right? They seem to be directed at the designer, is that correct?

[KlausLoeffelmann]

Yes. But we need to take those Designer issues into account in the decision-making process, because that's an important thing for the/a typical WinForms workflow (designing) process.

Also, if we were to modify Binding and have another feature "pending" so to speak, we should think about doing respective changes in one go.

In addition, I also want of course to have your opinion on the IBindingConverter suggestion! 😃

[zeh-almeida]

Frankly speaking, I would like to have a default constructor for Binding, because I think 9 parameters is a lot.
Not only that, ControlBindingsCollection has a Add method for each different constructor as well, making things very verbose and hard to understand.

I know this is a very history-rich code but still, if we are talking improvements, I guess this would be a good one.

I like the idea of a IBindingConverter but I am not sure how the designer works when detecting bindable properties.
My understanding is that any form Control should use Invoke by default, since they must always be changed in the UI Thread. This way, the user won't be surprised that a feature that compiles and passes unit tests fails with a thread exception.

Could the designer identify the ObservableProperty from the Community Toolkit? This way, it could bind directly to the property generated and have it set up correctly, as well.

[KlausLoeffelmann]

The designer points out INotifyPropertyChanged implementing classes, when you add DataSources (ObservableObject implements INotifyPropertyChanged). I guess ObservableProperty is only used on a field to request property code generation - I don't know if the resulting property is attributed with anything usable in that scenario.

Binding capabilities for properties on the control side (which properties can be bound to on a control) is a different story:

To make a control property like MyProperty bindable, it must have:

• A MyPropertyChanged Event.
• Preferably a virtual OnMyPropertyChanged method, which raises the event.

For the signature changes:
Please keep in mind that we can't do any changes that breaks existing code. We can only add to constructors in a way, that still let existing code run the way it had, especially when said code was originally CodeDOM generated by the Designer for use in InitializeComponent, like it is the case with the Binding class.

[zeh-almeida]

So I believe using the toolkit already covers the DataSource scenario, since the properties and events are created with the names the designer expects.

The only thing missing is telling the designer to bind with invoke. Like I said, I think this should actually be the default behavior.

If this can't be the default behavior because of codegen, API or any other braking changes, I think we should add a property in the designer to make it bind with the invoke option set.

This way no existing scenarios are broken and if you want, you could use the new behavior without issues.

Does that sound feasible @KlausLoeffelmann ?