Open zeh-almeida opened 1 year ago
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
One concern that I have here is that 98% of the binding scenarios in WinForms are not done via code but via the WinForms Designer UI. We would need to figure out a way in the Designer to allow the user to use the new feature interactively. That would also mean changes to the CodeDOM serialization.
One alternative way to address this could be an option in the Designer for a Form/UserControl to use Invoke when necessary. We could implement that on the Designer-side as a Design-Time property for the Form or UserControl. (UseAutoBindingInvoke
).
That said, if we would need to change the signature of Binding
and in the view of the new Command Binding feature in WinForms, we should also take feedback into consideration, where people want a simplified way to handle Converters, preferably based on an Interface we'd need to add. Converters in WinForms binding are possible right now alright, but very awkwardly to hook up (namely via events for each binding item). I could see something like IBindingConverter
similar to IValueConverter
in WPF but with its naming adapted to WinForms to avoid confusion.
To simplify this, we could pass a Converter to the Binding class in addition, which would lead to internally wiring up the required events, running the passed converters when needed.
If we need to change Binding
in more regards, we should do it in one go.
@merriemcgaw FYI.
And of course, FYI @russkie, if time allows! 😸
Thank you for tagging me.
My first reaction was that you'd likely want pretty much all bindings to go via the UI thread. With that in mind, the API signature change isn't required - you only would need to update the implementation (breaking docs notifications and all that entails). May be provide an opt-out mechanism, if necessary - through a feature switch or an API...
@RussKie so instead of having a new constructor like the one I made, are you suggesting I add a property with public get and set?
If that is correct, the behavior could be controlled at any time after instantiation, which sounds easier to manage on direct code but I don't know if that translates well to generator use.
Of course, we could have both options as well, I just want to make it clear because I can adjust my PR
Guys, may be I missed something, but I can't get core point here :( The code from 1 post is wrong even without binding:
var thread = new Thread(() =>
{
textBox.Text = "Updated test text"; // System.InvalidOperationException
});
thread.Start();
🤷♂️
@kirsan31 the binding goes both ways, as you can see by the Assert
calls made.
This code actually came from the PR I opened, so I am sure it works. Of course, I can always adjust it, no problem.
the System.InvalidOperationException
happens on the binding because it does not use Control.Invoke
and with the adjustments I propose, you may be able to avoid it entirely.
I hope I answered your questions, Please let me know if you have more
the binding goes both ways
Yea and from 1 post it's unclear about which way we are talking here. From the text I made assumption that from object to control. And it's ok in principle, but in common case you can change your bind object as your want in any thread and will see no exceptions and no changes in the correspond control property too. Or we talk here only about INotifyPropertyChanged
derived objects?
But from API Usage section we clearly see that you test control to object scenario, which doesn't make sense in any thread other then UI.
I am in the process of building a better example as I only have my code to show for, which wasn't crafted for this case specifically.
Yes, we are talking about INotifyPropertyChanged
derived objects, specifically when you have a asynchronous operation on said object which triggers the event.
I have a previous example here from my project, which illustrates how I was creating the binding.
However, because of my message is asynchronous, some of the message handlers changed their properties and the binding raised the System.InvalidOperationException
.
Hope this clarifies the issue better.
Thank you, now are all clear.
About your proposal. Personally, I think this is not good approach (pass param to constructor). I will try to argue my position.
Binding
class must be used in UI thread only.Binding
class object - UI only (old behavior) and partially (you still can't use all methods from non UI thread, only some of them) cross thread with Invoke
. This will lead to confusion in the code - it will not be clear when you can use Binding
not from the main thread, but when you cannot.Binding
instance cross thread or not. May be to add some property?Invoke
and not BeginInvoke
or able to select? Did you analyze possibilities of deadlocks scenarios in case of Invoke
?Overall I think approach suggested by @RussKie is more suitable here...
I agree, my proposal is probably not the best approach, I only tested using the scenario I found, without analyzing possibilites of deadlocks or other adverse situations.
I used Control.Invoke
instead of Control.BeginInvoke
in this case because the existing code calls the property.SetValue
directly and I wanted to avoid making changes to existing behavior, as you can see here.
I have no problem in changing the proposal to add a property toggle for this behavior instead of changing the constructor, as I now understand that changing any signature could be a problem for the designer and other tools.
I just want to make sure I understand the suggestion correctly:
I should add a public bool UseControlInvoke {get; set;}
property to the Binding
class and, if true, it should execute the property.SetValue
method in the Control.Invoke
action, removing the need to change the constructor.
Is this correct?
I used Control.Invoke instead of Control.BeginInvoke in this case because the existing code calls the property.SetValue directly and I wanted to avoid making changes to existing behavior, as you can see here.
Yea BeginInvoke
will require more investment. But in case of cross thread operation, I think in 90% cases, logically, you will want to use BeginInvoke
instead of Invoke
.
I just want to make sure I understand the suggestion correctly: I should add a
public bool UseControlInvoke {get; set;}
property to theBinding
class and, if true, it should execute theproperty.SetValue
method in theControl.Invoke
action, removing the need to change the constructor.
It seems to me that here we are stepping on thin ice of cross thread UI control🙄 Imho ideal solution will be something like BindingCT
or even BindingAsync
😯
Lets wait for response from maintainers...
Since the current behaviour is sync the fix would be to use Invoke. BeginInvoke or adding an async variant would be a new behaviour.
Just a quick update: We're still very much considering this but need to address a couple of higher priority issues, first.
@KlausLoeffelmann did you want to re-prioritize this?
Related: #8532
Well, I want to get it done for 9. I am not sure exactly when I get to it.
Background and motivation
Binding
s allow for FormControl
s and models to be updated when their values are changed by the system in any way. This would allow the form to be reactive, with processes running in the background performing all the necessary actions and having them reflected by the bindings automatically.This already works with MAUI, specially if you use the
CommunityToolkit.Mvvm
and its generators. However, while Winforms support bindings as well, it does not support asynchronous updates, because doing so would raise aInvalidOperationException
.This happens because the
Binding
class updates the value of the property directly, without checking if the control is in the UI Thread or not.I then propose a new constructor to the
Binding
class in which you can set a flag to prefer usingControl.Invoke
when updating the bindings.Control.Invoke
forces theAction
to run at the UI Thread, solving the problem.I have already opened an issue about this as well as a PR.
API Proposal
API Usage
Alternative Designs
I have added a new constructor to the
System.Windows.Forms.Binding
class and a new public method toSystem.Windows.Forms.ControlBindingsCollection
without changing previous APIs.Risks
In my limited study of the APIs, I didn't see any breaking risk associated with this proposal. There may be performance repercussions in the
Control.Invoke
mechanic on heavy load but I was unable to test it.The PR I mentioned has some tests which could be useful in analyzing those scenarios.