Skip to content

Introduce Command Binding, Bindable ToolStripItems and DataContext to WinForms#7143

Merged
KlausLoeffelmann merged 40 commits intodotnet:mainfrom
KlausLoeffelmann:CommandBinding
Aug 8, 2022
Merged

Introduce Command Binding, Bindable ToolStripItems and DataContext to WinForms#7143
KlausLoeffelmann merged 40 commits intodotnet:mainfrom
KlausLoeffelmann:CommandBinding

Conversation

@KlausLoeffelmann
Copy link
Member

@KlausLoeffelmann KlausLoeffelmann commented May 5, 2022 •
edited
Loading

This fixes #4895.

  • Implement ICommandBindingTargetProvider with Default Interface Methods holding 90% of the Command logic, so it can be easily reimplemented in other Components/Controls where is makes sense. This is no longer public, but internal, just to not repeat implementation code.
  • Implement BindableComponent to have a Component which can be bound at Design time the same way it works with Control already (both are implementing IBindableComponent).
  • Implement ICommandBindingTargetProvider into ToolStripItem to let it have a Command property of type ICommand which can be bound to from Controllers/ViewModels.
  • Implement ICommandBindingTargetProvider into ButtonBase to let it have a Command property of type ICommand which can be bound to from Controllers/ViewModels.
  • Implement DataContext Property as an ambient property along with DataContextChanged event, OnDataContextChanged and OnParentDataContextChanged.
  • Update demo to test implementations and to show the general usage of Controller/Models for WinForms Apps. See https://github.com/KlausLoeffelmann/WinFormsCommandBinding/tree/main/src. For this to be working, compile PR, patch .NET 7 binaries (assemblies, refs) and use ModifiedRuntime sample.
  • Review the general approach in the internal WinForms Technical Sync.
  • Apply changes from discussions (In addition: removed CommandEventArgs, added CommandParameter property).
  • Add a Maui sample to the demo, which uses the same ViewModel/UIController.
  • Write/Modify the demo for this, and test the hell out of it.
  • Go through API review.
  • Apply changes from API review.
  • Write/Update XML comments.
  • Add unit tests.

Update: After having an internal team discussion, we decided to rename BindableCommand back to be just Command.

Microsoft Reviewers: Open in CodeFlow

@KlausLoeffelmann
Add DataBinding functionality to ToolStripItem.
763cde6 
* Add CommandBinding to ToolStripItem.
@ghost ghost assigned KlausLoeffelmann May 5, 2022
@KlausLoeffelmann KlausLoeffelmann changed the title Introduce CommandBinding, Bindable ToolStripItems and DataContext to WinForms Introduce Command Binding, Bindable ToolStripItems and DataContext to WinForms May 5, 2022
@ghost ghost added the draft draft PR label May 6, 2022
@KlausLoeffelmann
Explore concept with IBindableCommandProvider.
27bce25
@KlausLoeffelmann
Implement IBindableCommandProvider and implement that in ToolStripItem.
8f5e34a 
* Also, refactor things a bit based on the changes.
@KlausLoeffelmann
Implement IBindableCommand into ButtonBase.
9f99455
@KlausLoeffelmann
Merge branch 'main' into CommandBinding
2af5fe1
@KlausLoeffelmann
Implement DataContext on Control as an ambient property.
34cbfe7 
* DataContext Property
* OnParentDataContextChanged Method
* OnDataContextChanged Method
* DataContextChanged Event
@KlausLoeffelmann
* Rename BindableCommand back to just Command
7bda78a 
After having an internal team discussion, we decided we should be in naming-parity with XAML based languages, and not so much afraid about breaking things due to already existing Command properties on Button-derived classes, since they are separated by namespaces anyway.
@KlausLoeffelmann
Merge branch 'main' into CommandBinding
2874aee
KlausLoeffelmann added a commit to KlausLoeffelmann/WinFormsCommandBinding that referenced this pull request May 10, 2022
@KlausLoeffelmann
First draft of demo testing dotnet/winforms#7143 with PR binaries.
a03e416
RussKie
RussKie reviewed May 11, 2022
View reviewed changes
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, please bring the API proposal up-to-date. Few nits and suggestions.

RussKie
RussKie reviewed Jun 17, 2022
View reviewed changes
RussKie
RussKie reviewed Jun 17, 2022
View reviewed changes
RussKie
RussKie reviewed Jun 17, 2022
View reviewed changes
RussKie
RussKie reviewed Jun 24, 2022
View reviewed changes
@RussKie
Copy link
Contributor

RussKie commented Jun 24, 2022

@gewarren when you have a chance, may I get you to review from the docs perspective?

gewarren
gewarren reviewed Jun 24, 2022
View reviewed changes
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it's helpful, the guidelines for documenting event-related APIs are here: https://github.com/dotnet/dotnet-api-docs/wiki/Summary%3A-Event-related

@runfoapp runfoapp bot mentioned this pull request Aug 2, 2022
Closed
RussKie
RussKie suggested changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@ghost ghost added the waiting-author-feedback The team requires more information from the author label Aug 3, 2022
@RussKie RussKie removed the draft draft PR label Aug 3, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 3, 2022
@RussKie
Copy link
Contributor

RussKie commented Aug 7, 2022

@KlausLoeffelmann I just come across this blog https://devblogs.microsoft.com/dotnet/announcing-the-dotnet-community-toolkit-800/.
Do you think this work would make us compatible with MVVM Toolkit?

@KlausLoeffelmann
Copy link
Member Author

KlausLoeffelmann commented Aug 7, 2022

I don't see why not!
I will certainly take it into account, when we'll preparing the WinForms/MAUI example.

RussKie
RussKie previously approved these changes Aug 8, 2022
View reviewed changes
dreddy-work
dreddy-work previously approved these changes Aug 8, 2022
View reviewed changes
Copy link
Member

@dreddy-work dreddy-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RLGTM.

JeremyKuhne
JeremyKuhne previously approved these changes Aug 8, 2022
View reviewed changes
dreddy-work
dreddy-work reviewed Aug 8, 2022
View reviewed changes
@KlausLoeffelmann KlausLoeffelmann enabled auto-merge (squash) August 8, 2022 17:42
@KlausLoeffelmann KlausLoeffelmann merged commit 84042a8 into dotnet:main Aug 8, 2022
@ghost ghost added this to the 7.0 RC1 milestone Aug 8, 2022
Tanya-Solyanik
Tanya-Solyanik reviewed Aug 8, 2022
View reviewed changes
src/System.Windows.Forms/src/Resources/SR.resx
<value>Expanded</value>
</data>
<data name="ToolStripItemOnCommandChangedDescr" xml:space="preserve">
<value>Occurs whenever the bindable command changes.</value>
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whenever -> when?

Copy link
Contributor

@RussKie RussKie Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording is common for event handlers, and it's used in other parts of the codebase.

src/System.Windows.Forms/src/System/Windows/Forms/BindableComponent.cs
namespace System.Windows.Forms
{
/// <summary>
/// Base class for components which provide properties which can be
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 2 whitespaces after the ///

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSeparator.cs
protected override void OnFontChanged(EventArgs e)
{
// Perf: don't call base, we don't care if the font changes
#pragma warning disable CA2252 // Suppress 'Opt in to preview features' (https://aka.ms/dotnet-warnings/preview-features)
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we have the preview warning when raising one of the old events.

Copy link
Member Author

@KlausLoeffelmann KlausLoeffelmann Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cut off the dependencies, when the consumer does not use the Preview feature directly. If we didn't do this, also using OnFontChanged would have been considered a Preview feature, and so on, and so on.

It's removed now - no longer in Preview.

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.cs
{
RaiseEvent(s_clickEvent, e);

// We won't let the preview feature warnings bubble further up beyond this point.
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really follow this comment. Do you mean that application that overrides this method will not show the warning?

Copy link
Member Author

@KlausLoeffelmann KlausLoeffelmann Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but at one point we needed to protect the WinForms runtime against the Preview warnings themselves. It's gone now.

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.cs
protected virtual void OnCommandParameterChanged(EventArgs e) => RaiseEvent(s_commandParameterChangedEvent, e);

/// <summary>
/// Called in the context of <see cref="OnClick(EventArgs)"/> to invoke <see cref="System.Windows.Input.ICommand.Execute(object?)"/> if the context allows.
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; this line is too long

Copy link
Member Author

@KlausLoeffelmann KlausLoeffelmann Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected.

@RussKie RussKie mentioned this pull request Aug 31, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JeremyKuhne JeremyKuhne JeremyKuhne left review comments

+4 more reviewers

@RussKie RussKie RussKie left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik left review comments

@gewarren gewarren gewarren left review comments

@dreddy-work dreddy-work dreddy-work approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@KlausLoeffelmann KlausLoeffelmann

Labels

None yet

Projects

None yet

Milestone

7.0 RC1

Development

Successfully merging this pull request may close these issues.

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

6 participants

@KlausLoeffelmann @RussKie @JeremyKuhne @Tanya-Solyanik @gewarren @dreddy-work