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

[API Proposal]: Introduce Commands and DataContext to WinForms to adapt modern Binding scenarios #4895

Closed
KlausLoeffelmann opened this issue May 11, 2021 · 18 comments · Fixed by #7143
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented

Comments

@KlausLoeffelmann
Copy link
Member

KlausLoeffelmann commented May 11, 2021

Rationale:

We want our customers to modernize their existing WinForms Apps and allow them to adapt new Azure-based technologies. It is important for our customers, that the business logic of their WinForms LOB apps to this end becomes more flexible and can be rearchitected away from the WinForms-typical code-behind and towards UI Controller or ViewModels, which can be easy reused and unit tested. This is also an incentive to pivot away from the 15 years old Typed DataSets in WinForms, which have very limited support in .NET WinForms Apps over .NET Framework, since the necessary Data Source Provider Service is not available for .NET Apps altogether. Feedback for suggesting this as our new Binding strategy was highly engaged, very positive, and shows that we're on the right way with that.

WinForms has a pretty powerful binding engine, but to achieve those things, a few essential features are missing:

  • ToolStripItems (the items that make pull-down menus, status bar labels and other status bar controls) are components rather than controls, and components deriving from ToolStripItem are currently not bindable.
  • Buttons and ToolstripItems don't provide properties to store or bind to Commands based on System.Windows.Input.ICommand.
  • There is no central hooking point on a Form or a UserControl to distribute a data source for data binding purposes from the parent down to every child it's hosting, like the DataContext property does that in WPF (or BindingContext in Maui Apps).

Having these features in WinForms, enables a series of important scenarios:

  • Using ViewModels/Controllers in WinForms: Customer’s LOB logic should be leverage to other UI technologies (Maui, UWP, WPF) and Azure Services, which is not possible or at least only quite difficult to achieve with the typical existing WinForms code-behind methodology.

MauiWinForms

  • Easier adaptation of modern ORMs: Since for the .NET Designer, the Data Source Provider Service is not available, with this new feature, it becomes easier to adapt and migrate away from ancient WinForms data layer architectures: While typed DataSet and DataTable were clearly designed with the WinForms code-behind model and direct binding to UI Elements of WinForms in mind, with the new data binding features present, modernized WinForms Apps could more easily consume typical ViewModels and Models as their Controller and Data Layer abstractions, and, what's important: Models in this scenarios being POCO classes which are generated and maintained by Entity Framework or Entity Framework Core.

  • Making it possible to unit test WinForms Apps business logic: Especially with the introduction of commands and command parameters, it would be way easier to use ViewModels/UI-Controller in binding scenarios for WinForms, that can be reused in Maui scenarios. While WinForms will not be able to achieve the same flexibility as compared to XAML based data binding, binding commands is a considerable step in that direction, Customers have repeatedly asked for that, and making ToolStripItems bindable at the same time, would be equally helpful in easier adapting ViewModels/UI Controller Models.

  • It's almost impossible to unit test Code-behind apps, which mixes business logic and UI-Control code. But that's how WinForms Apps are typically "architected". The new API makes it way simple, to separated the Business Logic from the actual Code-Behind code, so it can be properly unit and integration tested.

Github Repo for Sample-App

https://github.com/KlausLoeffelmann/WinFormsCommandBinding

API-Changes, Overview:

Note: For .NET 7, we want to put this feature out in Preview, attributing respective new classes and properties with the RequiresPreviewFeature attribute, collect feedback and then enable it for .NET 8.

To provide a more ViewModel/View Controller compatible way for a WinForms LOB architecture, I propose the following API-Changes:

  • Introduce the abstract classes BindableComponent, CommandComponent and CommandControl which describes the necessary methods and properties for extending a WinForms Component or a WinForms Control with the command logic. Since a Component is not bindable by default, we introduce a new abstract class which is bringing all the necessary infrastructure to make a Component bindable. CommandComponent (literally) on top of that introduces a Command property, allows to executes an Action assigned to Command, when the context allows for the Command to be executed. The action assigned to Command is being passed the parameter which is stored in the Component's or Control's new property CommandParameter. This is the same way, commands work in WPF, UWP, Xamarin and Maui.

  • Inherit ButtonBase from CommandControl instead of Control, so it will get all the necessary infrastructure for binding to commands, and for executing commands - if the context allows - OnClick.

  • Inherit ToolStripItem for CommandComponent, to make a) all ToolStripItem derived components bindable, and b) introduce a bindable Command property on all ToolStripItem derived components which can be clicked. Note: IBindableComponent is not a new API. It was especially developed to provide Data Binding for components (and it's of course implemented in Control to make it bindable). Implementing this Interface makes the new APIs BindingContext and OnBindingContextChanged necessary. The functionality of BindingContext is handed over to the existing static method BindingContext.UpdateBindings, which was especially designed to handle bindable Components. The idea is to seperate BindableComponent and CommandComponent, so other components can derive from BindableComponent to provide principle binding functionality, without necessarily to provide command binding.

  • Introduce a property DataContext on Control as an ambient property, and - to meet the WinForms DataBinding conventions - implement a virtual protected method OnDataContextChanged and the DataContextChanged event along with it. This way a child (User)Control can marshal a new/changing data source to whatever BindingSourceComponent needs to be assigned to.

FAQ

A few of most frequently asked questions up to now:

Why do we need On[Propertyname]Changed methods and [Propertyname]Changed Events for each property

That's the convention in WinForms to support data binding back to the source: A property is bindable in the direction to the data source only, if the property can raise a respective xxxChanged event. A further convention in WinForms is that for each event there is a Onxxx method, that can be overwritten in a derived class, which raises the event, since events cannot be raised directly in a derived class.

What's the reason for the naming

Naming pf new properties is based on precedence in existing UI stacks (Command, CommandParameter, DataContext in WPF). Naming of additional new events and methods is based on either existing interfaces or naming conventions in WinForms.

Why are we only introducing Commands on ButtonBase and ToolStripItem

Because only those make sense in the context of executing commands, and that's the way is done also in WPF. We have been discussing LinkLabel, but came to the conclusion that it would probably too special a use case. Since we are providing an easy way to extend controls with commands by letting exsting Controls or Commands inherit from CommandComponent or CommandControl, users can easily implement additional scenarios in controls how they see fit. The same is true for control vendors.

Do we need to extend the Designer?

No. As can be seen in the screenshots here, the Designer can deal with every of these changes out of the box. There is no necessity for additional work in the WinForms Designer.

If we have a typical Controller or ViewModel based on a MVVM-Framework like the MVVM Toolkit (or just implementing INotifyPropertyChanged directly like in the following sample), then the Designer can already work with that (see following screenshot):

using System;
using System.ComponentModel;

namespace WinFormsCommandBinding.Models.Controller
{
    public class SimpleEditorController : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler? PropertyChanged;

        private string? _textDocument;
        
        // Simple class which implements the details of ICommand.
        private RelayCommand? _newCommand;

        public SimpleEditorController()
        {
            _newCommand = new(ExecuteRelayCommand, CanExecuteRelayCommand);
        }

        private void ExecuteRelayCommand(object? commandParameter)
            => TextDocument = String.Empty;

        private bool CanExecuteRelayCommand(object? commandParameter)
            => TextDocument != String.Empty;

        public RelayCommand? NewCommand
        {
            get => _newCommand;

            set
            {
                if (!Equals(_newCommand, value))
                {
                    _newCommand = value;
                    PropertyChanged?.Invoke(this, new(nameof(NewCommand)));
                }
            }
        }

        public string? TextDocument
        {
            get => _textDocument;

            set
            {
                var wasEmpty = String.IsNullOrEmpty(_textDocument);

                if (!Equals(_textDocument, value))
                {
                    _textDocument = value;
                    PropertyChanged?.Invoke(this, new(nameof(TextDocument)));

                    if (wasEmpty ^ string.IsNullOrEmpty(_textDocument))
                    {
                        // Execution-Context changed because the TextDocument was empty
                        // and now it's not anymore or viceversa.
                        NewCommand?.RaiseCanExecuteChanged();
                    }
                }
            }
        }
    }
}

image

List of new APIs

Introducing the new abstract component BindableComponent

Note: Even though on first glance this might sense to put in the System.Component namespace and in the runtime, this is for the WinForms binding infrastructure only: IBindableComponent is in System.Windows.Forms and so is BindingContext.
Providing the infrastructure for Updating/Managing binding for Components has been implemented in WinForms all along, see here: BindingContext.cs (dot.net)

namespace System.Windows.Forms
{
    public abstract class BindableComponent : Component, IBindableComponent
    {
        public event EventHandler? BindingContextChanged;
        public ControlBindingsCollection? DataBindings { get; }
        public BindingContext? BindingContext { get; set; }
        protected virtual void OnBindingContextChanged(EventArgs e) { }
    }
}

Introducing the new abstract component CommandComponent

namespace System.Windows.Forms

    internal abstract class CommandComponent : BindableComponent
    {
        public event EventHandler? CommandCanExecuteChanged;
        public event EventHandler? CommandChanged;
        public event EventHandler? CommandParameterChanged;

        public abstract bool Enabled { get; set; }
        public System.Windows.Input.ICommand? Command { get; set; }
        public object? CommandParameter { get; set; }
        protected virtual void OnCommandChanged(EventArgs e) { }
        protected virtual void OnCommandCanExecuteChanged(EventArgs e) { }
        protected virtual void OnCommandParameterChanged(EventArgs e) { }
        protected virtual void OnRequestCommandExecute(EventArgs e) { }
    }
}

Introducing the new abstract control CommandControl

namespace System.Windows.Forms
{
    internal abstract class CommandControl : Control
    {
        public event EventHandler? CommandCanExecuteChanged;
        public event EventHandler? CommandChanged;
        public event EventHandler? CommandParameterChanged;

        public System.Windows.Input.ICommand? Command { get; set; }
        public object? CommandParameter { get; set; }
        protected virtual void OnCommandChanged(EventArgs e) { }
        protected virtual void OnCommandCanExecuteChanged(EventArgs e) { }
        protected virtual void OnCommandParameterChanged(EventArgs e) { }
        protected virtual void OnRequestCommandExecute(EventArgs e) { }
    }
}

Inheriting ButtonBase from CommandControl

namespace System.Windows.Forms
{
    public abstract partial class ButtonBase
-        : Control
+        : CommandControl
    {
          .
          .
          .
    }
}

Inheriting ToolStripItem from CommandComponent

namespace System.Windows.Forms
{
    public abstract partial class ToolStripItem 
-                            : Component,
+                            : CommandComponent,
                              IDropTarget,
                              ISupportOleDropSource,
                              IArrangedElement,
                              IKeyboardToolTip
    {
    .
    .
    .
    }
}

Introducing DataContext on the existing class class Control

We need an extension point for assigning the data sources not only to a Form or a UserControl, but at the same time make that data source available to each of the child (User)Controls which this root control is hosting. To this end, we introduce the DataContext Property of type object on Control, and we implement it as an ambient property. This way, it doesn't use any additional memory resources, as long as it is not used. When it is assigned to a Form, its OnDataContextChanged for the Form (and for each respective child control) is called. OnDataContextChanged then raises the DataContextChanged event. The root control or/and the children can handle that event to decide, what to do with the data source: Usually, the data source then gets assigned to the BindingSource component(s) the Control uses, which then initiates the actual data binding.

namespace System.Windows.Forms
{
    public partial class Control :
        Component,
        Ole32.IOleControl,
        Ole32.IOleObject,
        Ole32.IOleInPlaceObject,
        Ole32.IOleInPlaceActiveObject,
        Ole32.IOleWindow,
        Ole32.IViewObject,
        Ole32.IViewObject2,
        Ole32.IPersist,
        Ole32.IPersistStreamInit,
        Oleaut32.IPersistPropertyBag,
        Ole32.IPersistStorage,
        Ole32.IQuickActivate,
        ISupportOleDropSource,
        IDropTarget,
        ISynchronizeInvoke,
        IWin32Window,
        IArrangedElement,
        IBindableComponent,
        IKeyboardToolTip,
        IHandle
    {
        public event EventHandler DataContextChanged;

        public virtual Object? DataContext { get; set; }

        protected virtual void OnDataContextChanged(EventArgs e) { }
        protected virtual void OnParentDataContextChanged(EventArgs e) { }

    }
}
@KlausLoeffelmann KlausLoeffelmann added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label May 11, 2021
@KlausLoeffelmann KlausLoeffelmann self-assigned this May 11, 2021
@dotnet dotnet locked and limited conversation to collaborators May 11, 2021
@KlausLoeffelmann
Copy link
Member Author

I locked this for the time being, since this is still work in progress.

@KlausLoeffelmann
Copy link
Member Author

KlausLoeffelmann commented Aug 13, 2021

@merriemcgaw: Talked to @OliaG about this in the context of the last survey, and we'd like to plan this in the .NET 7 timeframe.

Unlocking this now. Although we won't start working on this for a while, I am happy for any suggestions folks my have for this!

@dotnet dotnet unlocked this conversation Aug 13, 2021
@KlausLoeffelmann KlausLoeffelmann changed the title [WIP] Introduce Commands and DataContext to WinForms to adapt modern Binding scenarios Introduce Commands and DataContext to WinForms to adapt modern Binding scenarios Oct 8, 2021
@tbolon
Copy link
Contributor

tbolon commented Feb 2, 2022

Regarding your proposition to make ToolStripItem bindable, see also this discussion about using binding with toolstripcontrolhost.

Also, we wanted to use binding in StatusStrip, on ToolStripStatusLabel, and found that we had to wire to the BindingContext of the Form for the binding to work. Maybe this scenario could be supported as well (which I think is certainly common: wanting to show some model information in the StatusStrip of a form).

Thanks,

@KlausLoeffelmann
Copy link
Member Author

KlausLoeffelmann commented Feb 3, 2022

Also, we wanted to use binding in StatusStrip, on ToolStripStatusLabel...

Yes, this is what I had in mind. Since ToolStripItem is the base for so many things, making that bindable in principle should have the requested result.

I'd need to experiment a bit with #6517 and see what's the deal there.

@tbolon
Copy link
Contributor

tbolon commented Feb 3, 2022

For our case, we had to make 3 things for ToolStripStatusLabel:

  1. Create a "BindableStatusStrip" with this kind of code:
public class BindableStatusStrip : StatusStrip
{
    /// <summary>
    /// Raises the <see cref="Control.BindingContextChanged"/> event.
    /// </summary>
    protected override void OnBindingContextChanged(EventArgs e)
    {
        base.OnBindingContextChanged(e);

        foreach (var toolStripItem in Items)
        {
            if (toolStripItem is IBindableToolStripItem bindableToolStripItem)
            {
                bindableToolStripItem.UpdateBindingContext();
            }
        }
    }
}
  1. Create a custom IBindableToolStripItem:
/// <summary>
/// Describe a <see cref="ToolStripItem"/> which implements <see cref="IBindableComponent"/>.
/// </summary>
/// <remarks>
/// <para>
/// This interface must be added to any <see cref="ToolStripItem"/> which wants to implement <see cref="IBindableComponent"/>.
/// It is required because <see cref="ToolStripItem"/> does not inherit from <see cref="Control"/>, and so do not receive notifications
/// when the <see cref="BindingContext"/> change on the parent control.
/// </para>
/// <para>
/// Implementing this interface is required by any class based on <see cref="ToolStripItem"/> and the <see cref="StatusStrip"/> container must be
/// replaced by <see cref="BindableStatusStrip"/>, which track <see cref="Control.OnBindingContextChanged(EventArgs)"/> to call <see cref="UpdateBindingContext"/> on each
/// <see cref="ToolStripItem"/> from <see cref="ToolStrip.Items"/> which implements <see cref="IBindableToolStripItem"/>.
/// </para>
/// </remarks>
public interface IBindableToolStripItem : IBindableComponent
{
    /// <summary>
    /// Notify this <see cref="ToolStripItem"/> that the parent binding context has changed, and that the bindings exposed by <see cref="IBindableComponent.DataBindings"/> must be updated.
    /// </summary>
    void UpdateBindingContext();
}
  1. Create custom controls inheriting from ToolStripStatusLabel or other controls, for example this one:
public class ToolStripStatusBindableLabel : ToolStripStatusLabel, IBindableComponent, IBindableToolStripItem
{
    private BindingContext _bindingContext;

    private ControlBindingsCollection _dataBindings;

    /// <summary>
    /// Implémentation de <see cref="IBindableComponent.BindingContext"/>.
    /// </summary>
    [Browsable(false),
    DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden),
    EditorBrowsable(EditorBrowsableState.Advanced)]
    public BindingContext BindingContext
    {
        get
        {
            if (_bindingContext != null)
                return _bindingContext;

            var parent = (Control)Owner;
            if (parent != null)
                return parent.BindingContext;

            return null;
        }
        set
        {
            _bindingContext = value;
        }
    }

    /// <summary>
    /// Implémentation de <see cref="IBindableComponent.DataBindings"/>.
    /// </summary>
    [DesignerSerializationVisibility(DesignerSerializationVisibility.Content)]
    [RefreshProperties(RefreshProperties.All)]
    [ParenthesizePropertyName(true)]
    public ControlBindingsCollection DataBindings
    {
        get { return this._dataBindings ?? (this._dataBindings = new ControlBindingsCollection(this)); }
    }

    /// <summary>
    /// Libère les ressources associées à ce ToolStrip<see cref="ToolStripFormattedLabel"/>.
    /// </summary>
    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);

        if (disposing)
        {
            this.Events.Dispose();

            if (this._dataBindings != null)
            {
                this._dataBindings.Clear();
                this._dataBindings = null;
            }
        }
    }

    protected override void OnParentChanged(ToolStrip oldParent, ToolStrip newParent)
    {
        base.OnParentChanged(oldParent, newParent);
    }

    public void UpdateBindingContext()
    {
        if (_dataBindings != null)
        {
            for (int i = 0; i < _dataBindings.Count; i++)
            {
                BindingContext.UpdateBinding(BindingContext, _dataBindings[i]);
            }
        }
    }
}

@bairog
Copy link

bairog commented Feb 28, 2022

@KlausLoeffelmann In your Devblogs article Databinding with the OOP Windows Forms Designer you promised to introduce some sample code in context of this issue. Almost a month has passed since February 2 - where we can find some basic tutorial for modern WinForms databinding? I've only found outdated docs for WinForms databinding with EF6 :(

@merriemcgaw
Copy link
Member

We're still working on the blogs. It's taking a bit longer than anticipated but it's actively in progress.

@bairog
Copy link

bairog commented Mar 1, 2022

@merriemcgaw @KlausLoeffelmann Looking forward for some documentation to come soon. Without clear explanation it's not easy to understand new Command approach in WinForms.
BTW Is that also appliable to .NET5 (we are stuck on VS 2019 for a near future - so we cannot use .NET 6/7)? Is that mostly a VS designer capabilities, right? If I write all code manually - it should just work on .NET5? If not - what is recommended approach for WinForms databiding in .NET 5 (VS 2019)?

@KlausLoeffelmann
Copy link
Member Author

I know folks are waiting for more demos and concepts how to implement this in real life demos.
I will start working on this implementation shortly, and we'll aim to enable this or at least parts of this in the .NET 7 timeframe.

To be as transparent as possible: Our SDK, improving the usage of the SDK, and - of course - its documentation has however higher priority, so we need to complete a few work items there, first.

To keep this discussion alive and give folks a clearer idea how this functionality can be used (and also to address the more and more incoming requests for a model-binding demo from the Databinding post, and also as an answer to @bairog, here is a link to my private repo, in which I was collecting ideas around this. It's pretty much what the Databinding posts also reflects.

Also please, keep in mind, the primary idea of the Databinding post was to introduce the new Databinding functionality in the designer. We should lead the discussion about WinForms UI Controllers/ViewModel ideas here.

https://github.com/KlausLoeffelmann/WinFormsCommandBinding

@tbolon
Copy link
Contributor

tbolon commented Mar 15, 2022

I have played just a few minutes with your code.

Did you already think about having a transient DataContext? Where the parent datacontext is by default visible to all contained controls? It's a concept well-known in WPF and it has its advantages but also drawbacks. I am not sure it will fit winforms, basically because the BindingSource is still the way to go and already requires a to add a component on each usercontrol.

And also because to make it work it will certainly requires to implement IDataContextTarget at the Control level, so any child control could receive the parent datacontext (or instead return the parent value if not overridden).
If not, do you expect to implement IDataContextTarget on Form and UserControl (at least)?

In the end, I am not sure I would suggest adding DataContext to the base Control class. I think I will only suggest implementing it in Form and UserControl class, and let developers share the parent DataContext with children.

We have a lot of big forms where we have a master "datacontext". We split our UI in different usercontrols for practical reasons (performances and maintenability) and we pass our "datacontext" from the parent form to each control.

@tbolon
Copy link
Contributor

tbolon commented Mar 15, 2022

Also I did not understand what is the goal of the BindableCommandComponent. The ToUpper menu did nothing on your example and the bindableCommandComponent was bind to this command.

PS: Feel free to ignore my messages if your are focused on other things. I suppose everything will be more clear when you will post in the blog about this topic.

@RussKie RussKie added this to the .NET 7.0 milestone May 5, 2022
@ghost ghost added the 🚧 work in progress Work that is current in progress label May 5, 2022
@KlausLoeffelmann
Copy link
Member Author

KlausLoeffelmann commented May 8, 2022

@tbolon:

If not, do you expect to implement IDataContextTarget on Form and UserControl (at least)?

Yes, that was my plan. Actually, if we implement that, I still want to do it in an Ambient Property-ish way, but probably limit it to Form and UserControl, nevertheless.

@KlausLoeffelmann
Copy link
Member Author

Also I did not understand what is the goal of the BindableCommandComponent. The ToUpper menu did nothing on your example and the bindableCommandComponent was bind to this command.

Yeah, I always was ambivalent about that as well, and wrote "Optional" to begin with. I am gonna drop this, it makes no sense. Or not enough sense.

@KlausLoeffelmann KlausLoeffelmann added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner blocking Used by the API Review Board and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation labels Jun 23, 2022
@KlausLoeffelmann KlausLoeffelmann changed the title Introduce Commands and DataContext to WinForms to adapt modern Binding scenarios [API Proposal]: Introduce Commands and DataContext to WinForms to adapt modern Binding scenarios Jul 12, 2022
@bartonjs
Copy link
Member

bartonjs commented Jul 14, 2022

Video

  • The ICommandProvider interface is rather large, which gives off a feeling that it will end up growing over the next couple of releases. Instead of a large interface the behavior should be pulled off onto a different type.
    • One possible hand-waved example here is included (changed ICommandProvider to CommandControl : Control and changed the base class of ButtonBase)
      • Most of the Raise* members can probably easily be eliminated and just be the On* version.
    • The alternative design (ICommandBindingTarget + class CommandProvider) may also be fruitful
  • We also discussed the DataContext(Changed) aspect and it looks reasonable
namespace System.Windows.Forms
{
    public class CommandControl : Control
    {
        event EventHandler? CommandChanged;
        event EventHandler? CommandCanExecuteChanged;

        /// <summary>
        /// Gets or sets the Command to invoke, when the implementing 
        /// Component or Control is triggered by the user.
        /// </summary>
        System.Windows.Input.ICommand? Command { get; set; }

        /// <summary>
        /// Gets or sets the CommandParameter for the Component or Control.
        /// </summary>
        object? CommandParameter { get; set; }

        /// <summary>
        /// Gets or sets a value indicating whether the control 
        /// can respond to user interaction.
        /// </summary>
        bool Enabled { get; set; }

        // Needed to restore the previous Enabled status
        // when a new Command gets assigned.
        protected bool? PreviousEnabledStatus { get; set; }

        // Needed to call OnCommandChanged to meet
        // WinForms property implementation conventions for Data Binding.
        protected void RaiseCommandChanged(EventArgs e);

        // Needed to call OnCommandCanExecuteChanged to meet
        // WinForms property implementation conventions for Data Binding.
        protected void RaiseCommandCanExecuteChanged(object sender, EventArgs e);

        // Needed to call OnCommandExecuting to meet
        // WinForms property implementation conventions for Data Binding.
        protected void RaiseCommandExecuting(CancelEventArgs e);

        /// <summary>
        /// Handles the assignment of a new Command to the Command property.
        /// </summary>
        /// <param name="commandComponent">Instance of the component which 
        /// provides the Command property.</param>
        /// <param name="newCommand">New Command value to be assign.</param>
        /// <param name="commandBackingField">The backing field of the component 
        /// which holds the command for the property.</param>
        protected static void CommandSetter(
            ICommandProvider commandComponent,
            ICommand newCommand,
            ref ICommand commandBackingField)
        { }

        /// <summary>
        /// Handels the invoke of the command, e.g. called by a button's click event.
        /// </summary>
        /// <param name="commandComponent"></param>
        protected static void RequestCommandExecute(ICommandProvider commandComponent) { }
    }
}
namespace System.Windows.Forms
{
-    public abstract partial class ButtonBase : Control
+    public abstract partial class ButtonBase : CommandControl
}

@bartonjs bartonjs added api-needs-work (3) API needs work before it is approved, it is NOT ready for implementation; applied by repo owners and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Jul 14, 2022
@KlausLoeffelmann KlausLoeffelmann added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed api-needs-work (3) API needs work before it is approved, it is NOT ready for implementation; applied by repo owners labels Jul 19, 2022
@KlausLoeffelmann
Copy link
Member Author

Challenges we face when we put this under Preview by attributing respective new APIs with RequiresPreviewFeature (which is not part of the actual API review discussion, I only state it here for completeness):

Button's new Command property will most probably be used in binding scenarios. And here is the problem with that: WinForms currently binds via reflection; property names are passed to the ControlBindingsCollection by string. So, from the perspective of the compiler, this new API is never used, thus there is no compile error. (This, btw, is code which gets generated into a method called 'InitializeComponent' by the WinForms Designer.)

image

It's different though with binding to a component which has derived from ToolStripItem, because that one never had a ControlBindingsCollection (DataBindings property) to begin with. Since this is a new API, it's been called out as such, but technically, the respective code line should be called out twice: One additional time for using the Command property. And this is also not happening for the same reason.

image

What we're currently exploring to mitigate this is to introduce a (Designer-side) shadow property 'DataBindings' which the Designer examines on CodeDom-serialization. And if we find a binding definition to a property which is attributed with RequiresPreviewFeature, we'd put that Attribute on InitializeComponent itself, preferably along with a message stating that a data binding definition used a Preview API.

@terrajobst
Copy link
Member

terrajobst commented Jul 21, 2022

Video

  • We'd prefer not having the classes CommandComponent and CommandControl
    • Instead just follow the WinForms model of having a naming convention
  • We still need BindableComponent because Component is lower than WinForms (where IBindableComponent lives).
  • We'd like the designer to mark the generated InitializeComponent with [RequiresPreviewFeatures(Url = "<placeholder>")] when these are used
namespace System.Windows.Forms;

[RequiresPreviewFeatures(Url = "<placeholder>")]
public abstract class BindableComponent : Component, IBindableComponent
{
    public event EventHandler? BindingContextChanged;
    public ControlBindingsCollection? DataBindings { get; }
    public BindingContext? BindingContext { get; set; }
    protected virtual void OnBindingContextChanged(EventArgs e);
}

// Suppress warning
public abstract class Control : BindableComponent // Used to be Component
{
}

public partial class ButtonBase
{
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public event EventHandler? CommandCanExecuteChanged;
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public event EventHandler? CommandChanged;
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public event EventHandler? CommandParameterChanged;

    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public ICommand? Command { get; set; }
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public object? CommandParameter { get; set; }

    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnCommandChanged(EventArgs e);
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnCommandCanExecuteChanged(EventArgs e);
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnCommandParameterChanged(EventArgs e);
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnRequestCommandExecute(EventArgs e);
}

public abstract partial class ToolStripItem : BindableComponent // Used to be Component
{
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public event EventHandler? CommandCanExecuteChanged;
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public event EventHandler? CommandChanged;
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public event EventHandler? CommandParameterChanged;

    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public ICommand? Command { get; set; }
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    public object? CommandParameter { get; set; }

    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnCommandChanged(EventArgs e);
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnCommandCanExecuteChanged(EventArgs e);
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnCommandParameterChanged(EventArgs e);
    [RequiresPreviewFeatures(Url = "<placeholder>")]
    protected virtual void OnRequestCommandExecute(EventArgs e);
}

@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Jul 21, 2022
@RussKie RussKie removed the blocking Used by the API Review Board label Jul 22, 2022
@tbolon
Copy link
Contributor

tbolon commented Jul 22, 2022

Button's new Command property will most probably be used in binding scenarios. And here is the problem with that: WinForms currently binds via reflection; property names are passed to the ControlBindingsCollection by string. So, from the perspective of the compiler, this new API is never used, thus there is no compile error. (This, btw, is code which gets generated into a method called 'InitializeComponent' by the WinForms Designer.)

I know it will certainly not be feasible, because the CodeDom serialization code for the designer must be a really dangerous place to look at :D, but if the string is replaced with a nameof() instruction, could this problem be solved?

this._myButton.DataBindings.Add(new System.WindowsForms.Binding(nameof(this._myButton.Command), ...));

@RussKie
Copy link
Member

RussKie commented Jul 24, 2022

This (and many other improvements) will be possible once the designer code DOM generation is migrated to the Roslyn engine. This is work in progress, but no definite timelines.

@ghost ghost removed the 🚧 work in progress Work that is current in progress label Aug 8, 2022
@ghost ghost removed this from the .NET 7.0 milestone Aug 8, 2022
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented
Projects
None yet
8 participants
@tbolon @RussKie @terrajobst @KlausLoeffelmann @bartonjs @merriemcgaw @bairog and others