[Feature] MVVM Toolkit - Remove 'sealed' from the four Command Classes to allow extension

[thomasclaudiushuber]

Describe the problem this feature would solve

Command classes are marked as sealed. That makes it impossible to extend these classes with custom behavior. The issue regarding WPF's CommandManager can be handled in different ways a) Developers have to create completely new commands for WPF if they want to use CommandManager b) Developers can extend existing Toolkit Commands to hook up CommandManager c) Developers can add a NuGet Package called Microsoft.Toolkit.Mvvm.Input.Wpf to get the WPF specific commands if they want to continue to use WPF's CommandManager

In either case, unless there's a specific reason to mark the Command classes as sealed, I would mark them as unsealed, as this would allow not only option a) and c), but also b). And b) could also be a more elegant way for c) without the need to re-implement the whole thing.

So, this issue is a request to make the command classes open for extension.

Describe the solution

Remove the sealed keyword from the command classes in Microsoft.Toolkit.Mvvm.Input.

Also mark at least the CanExecuteChanged event in those command classes as virtual:

public virtual event EventHandler? CanExecuteChanged

Helpful, but not required, would be a property that says whether the Command can always be executed or not. Because only in the latter case, the CommandManager's RequerySuggested event needs to be attached.

Consider implementing this property in the Command classes:

public bool CanAlwaysExecute => canExecute is null;

Describe alternatives you've considered

Alternative to the changes is option a) for WPF developers to implement commands on their own, which means they don't get the commands from the Toolkit, which reduces the value of the Toolkit for them if they want to continue using WPF's CommandManager.

Additional context

With the changes applied, there could be a WPF-specific RelayCommand class like below (Just taking this command class as an example, the other three command classes can also be extended like this with the changes of this issue applied). And this class could be implemented by a developer itself (option b)) or we deliver a separate NuGet package that is WPF-specific. But first of all, the RelayCommand of the Toolkit and the other ICommand implementations should be open for extension to write the below code:

namespace Microsoft.Toolkit.Mvvm.Input.Wpf
{
  public class RelayCommand : Microsoft.Toolkit.Mvvm.Input.RelayCommand
  {
    public RelayCommand(Action execute) : base(execute) { }

    public RelayCommand(Action execute, Func<bool> canExecute) : base(execute, canExecute) {  }

    public override event EventHandler? CanExecuteChanged
    {
      add => CommandManager.RequerySuggested += value;
      remove => CommandManager.RequerySuggested -= value;
    }
  }
}

If even the additional CanAlwaysExecute property is added, the WPF-specific command could look like this:

namespace Microsoft.Toolkit.Mvvm.Input.Wpf
{
  public class RelayCommand : Microsoft.Toolkit.Mvvm.Input.RelayCommand
  {
    public RelayCommand(Action execute) : base(execute) { }

    public RelayCommand(Action execute, Func<bool> canExecute) : base(execute, canExecute) {  }

    public override event EventHandler? CanExecuteChanged
    {
      add 
      {
        if(!CanAlwaysExecute)
        {
          CommandManager.RequerySuggested += value;
        }
      }
      remove 
      {
        if(!CanAlwaysExecute)
        {
          CommandManager.RequerySuggested -= value;
        }
      }
    }
  }
}

[ghost]

Hello, 'thomasclaudiushuber! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

[Sergio0694]

Hey @thomasclaudiushuber - thank you for the feedbacks! 😄

I'll start with the CanAlwaysExecute property, which seems absolutely fine to me, sure. I guess I could add that to the IRelayCommand interface, since that applies to all 4 command types in the toolkit. If @michael-hawker is fine with the idea as well, I can include that in my current branch with the changes for Preview 5 (#3562). That does indeed seem useful!

As for the proposal to unseal the command types and make the event virtual, I'm afraid I don't really like that 😟 Let me elaborate on that. @sonnemaf and I already had a conversation about this very same point before, see here) and my reply here. To summarize, and also to keep all the info relevant to this issue in this same thread, here are the main points for me:

• The proposed solution would strictly be a workaround to address (and not even completely, as the usage of that type would still remain clunky on WPF) a specific lack of proper support on a very specific framework. One of the points of the MVVM Toolkit was to build a new, modern MVVM toolkit that didn't have to contend with back-compat holding it back, and that could offer a good codebase that was meant to be used in a completely platform agnostic scenario. It seems to me that the proposed solution would negatively impact the API surface of all the command types purely so that users on a single platform might be able to more easily inject a workaround there. I'm afraid I don't really like the idea of making such an alteration to the API surface because of this. The performance loss aspect is secondary, though I don't like that either, for the same reason.
• One of the reasons those class are sealed is simply that they're not meant to be extended, both conceptually and due to how they're implemented, specifically the async command ones which specifically rely on not having other external code modifying any of the state or changing things, in order to work correctly. These command types are meant to be used as is.
• Conceptually, I'd argue that this proposal is not the correct way to address the issue. In this case we wouldn't be extending a class to add more functionality to it (to make an example, like our GroupedObservableCollection<T> types in Microsoft.Toolkit), but merely to inject a workaround for a specific platform. I reckon one of the proper wasy would be either to have users bridge the missing functionality manually or through a helper attached property for WPF.
• Quoting from my other comment: "we figured [one of] the main reason why devs would want to extend them would be just to add some extra properties that are unrelated to the actual commanding logic. In this case extending them would not really be ideal, especially because if you also wanted to have the same extra properties in other commands, you'd need to extend every single one of them and add the same property every time. For cases such as this I think the best approach is to use a type like StandardUICommand (from WinUI), or equivalent, which is meant to be used exactly in these situations." As in, in these situations I'd recommend using composition over inheritance.

EDIT: if we wanted to offer a Microsoft.Toolkit.Mvvm.Wpf package, I don't see why it couldn't reimplement those commands directly. I don't see the advantage in inheriting from the ones in the main package other than just saving some duplicated code there, but again I don't feel like it's worth sacrificing the API surface in the main package for. Those separate commands would still be interoperable with the ones in the main package, as they'd still be able to implement the same interfaces (eg. IRelayCommand).

Hope that helps! 🙂

[thomasclaudiushuber]

Hi @Sergio0694 , thanks for your reply.

I'll start with the CanAlwaysExecute property, which seems absolutely fine to me, sure. I guess I could add that to the IRelayCommand interface, since that applies to all 4 command types in the toolkit

Yes, is already done like that in the PR for this issue.

the usage of that type would still remain clunky on WPF

I don't understand this. It would behave exactly the way how people know Commands using CommandManager and WPF. If there would be another NuGet package, all developers have to do is changing the using directive from Microsoft.Toolkit.Mvvm.Input to Microsoft.Toolkit.Mvvm.Input.Wpf, and CommandManager magically works.

specifically rely on not having other external code modifying any of the state or changing things, in order to work correctly.

If we keep the stuff in the command classes private, then subclasses cannot modify anything from the state, which means the classes will always work as is. And it's really only the CanExecuteChanged event that is required to hook up WPF's CommandManager, nothing else.

The question is: Is it worth to allow extension if the only use case is WPF's CommandManager? Maybe, maybe not. It's something you can discuss with Michael.

Inheritance would not only mean less code duplication, it would also mean that WPF developers or the WPF specific package would automatically use the latest code of the MVVM toolkit. Without inheritance, the code is at two places. But of course, you could reimplement everything WPF-specific and rely just on the interface. It's also an option with pros and cons.

For me both approaches are ok, Inheritance or complete re-implementation for WPF.

Conceptually, I'd argue that this proposal is not the correct way to address the issue.

Feel free to close this issue and the PR after discussing with @michael-hawker. It's really just a way to show how it could be done to bring more light into the dark for those who are interested in the CommandManager stuff. But if you have already doubts about marking the classes non-sealed, I see that this issue and PR is not for you :-) Then you could think about another path moving forward. That other path could be a) a separate package that implements IRelayCommandwith CommandManagerfor WPF b) nothing WPF-specific in WCT, and force developers to use NotifyCanExecuteChanged instead of CommandManageror they have to implement their own Commands. Makes migration harder for those who relied on CommanagManager in the past.

[Sergio0694]

Yes, is already done like that in the PR for this issue.

Ah, sorry I had missed the PR, as it wasn't linked to this issue. I've fixed that now 😄

I don't understand this. It would behave exactly the way how people know Commands using CommandManager and WPF.

That was more of a side note, not really an argument against this. What I meant by "clunky" is that the fact remains that WPF still doesn't properly support the ICommand interface - it would essentially be the only platform where you'd have to use a platform-specific command type, which means you'd have to either keep all your backend code in a WPF project, or somehow inject commands at runtime through some factory service or something like that (very clunky). I mean it's not just working out of the box with proper framework support like it does on eg. UWP XAML or WinUI 3, is all 🙂

If we keep the stuff in the command classes private, then subclasses cannot modify anything from the state, which means the classes will always work as is.

I didn't phrase that part properly, let me clarify that. Take the AsyncRelayCommand type. What I meant is that you would either just keep every member private, or make them visible to inheriting classes. In the first case, making the class non sealed would be pointless, as derived types wouldn't really be able to interact with the state in a meaningful way other than just adding eg. new members on top of the command. And at that point you might as well just use composition instead. In the second case, that'd mean those classes could potentially interfere with how those classes work, especially the asynchronous commands. Not a major issue, sure, but since we were discussing the PR in general I thought I'd mention this point as well for completeness.

The question is: Is it worth to allow extension if the only use case is WPF's CommandManager? Maybe, maybe not.

Yes, that's really main main concern here at the end of the day 🤔

Inheritance would not only mean less code duplication, it would also mean that WPF developers or the WPF specific package would automatically use the latest code of the MVVM toolkit. Without inheritance, the code is at two places. But of course, you could reimplement everything WPF-specific and rely just on the interface. It's also an option with pros and cons.

Yes, if we really were to actually make a .Wpf extension, we could just duplicate those types I'd imagine we'd just keep those types in sync, so I'm not really worried about consumers not being able to always use the latest version available.

Leaaving this issue open and adding the "open discussion" tag, so that others can chime in as well with their thoughts 😊

[thomasclaudiushuber]

Ah, sorry I had missed the PR, as it wasn't linked to this issue. I've fixed that now 😄

Thanks for linking it, I owe you a cup of coffee. :-)

Yes, if we really were to actually make a .Wpf extension, we could just duplicate those types I'd imagine we'd just keep those types in sync, so I'm not really worried about consumers not being able to always use the latest version available

Yes, right. And if the .Wpf extension package is added, inheritance is not super important, as that package can be consumed by toolkit users. Regarding file updates, it would also be possible to have a linked file with #if logic for the WPF extension, but maybe that's also more ugly than just copying the commands over to the WPF-specific project.

If the .Wpf extension is not added, command inheritance would be more important. But I guess, if that Wpf package won't be in the toolkit, someone else might create it. :)

What I meant by "clunky" is that the fact remains that WPF still doesn't properly support the ICommand interface

Ok, got it. I think you're refering here to CommandParameter property that triggers CanExecuteChanged in UWP/WinUI, but in WPF it doesn't. Yes, this is a difference. But to be honest, in all bigger WPF MVVM applications, I've never seen CommandParameter being used. I also don't use it in UWP/WinUI. I think having everything in the ViewModel is much cleaner. And then, when you move all that logic to your ViewModel, you have to call NotifyCanExecuteChanged anyway, and it works across all XAML platforms.

So, instead of saying WPF does not support ICommand properly, we could also ask ourselves the question Is using CommandParameter the right way to do MVVM? I don't want to answer this, but you might know my opinion when you read the lines above. But just want to bring this into the discussion. Curious what others think about this, especially those devs who do only UWP/WinUI.

But I guess there are three ways to do MVVM: a) Make it work on all platforms: Use RelayCommand.NotifyCanExecuteChanged in your ViewModels, it works across all XAML platforms. b) Make it work in UWP/WinUI Use CommandParameter property, it triggers CanExecute automatically c) Make it work in WPF Use CommandManager, it triggers CanExecute automatically.

So, looks a bit like b) and c) are platform-specific ways to do it. We could also push a PR to WPF to support b) in addition.

[laurentkempe]

@Sergio0694

What I meant by "clunky" is that the fact remains that WPF still doesn't properly support the ICommand interface - it would essentially be the only platform where you'd have to use a platform-specific command type, which means you'd have to either keep all your backend code in a WPF project

That's sad but it is the way it is with WPF and there is no really way to escape it. And I don't see this changing soon in WPF. I guess if WCT wants to support WPF it has to accommodate it.

@thomasclaudiushuber

c) Make it work in WPF Use CommandManager, it triggers CanExecute automatically.

It is definitely needed otherwise developers adding the MVVM WCT in WPF will get frustrated and might even realize late that it doesn't work with Command CanExecute.

[Sergio0694]

Yes, right. And if the .Wpf extension package is added, inheritance is not super important, as that package can be consumed by toolkit users. Regarding file updates, it would also be possible to have a linked file with #if logic for the WPF extension, but maybe that's also more ugly than just copying the commands over to the WPF-specific project.

I wouldn't be opposed to linking those 4 files for convenience, as long as the actual code in the MVVM Toolkit itself doesn't have to compromise just to make this possible on WPF, sure. Just using #ifdef around the event/accessibility would be reasonable 🙂

Ok, got it. I think you're refering here to CommandParameter property that triggers CanExecuteChanged in UWP/WinUI, but in WPF it doesn't. Yes, this is a difference. But to be honest, in all bigger WPF MVVM applications, I've never seen CommandParameter being used. I also don't use it in UWP/WinUI. I think having everything in the ViewModel is much cleaner. And then, when you move all that logic to your ViewModel, you have to call NotifyCanExecuteChanged anyway, and it works across all XAML platforms.

What I meant was in part that, but also that those frameworks give developers the choice of using either of those approaches, and both work. Not forcing a specific approach and letting developers free of using whatever architecture/setup they felt most comfortable with is another of the main principles we used to build the MVVM Toolkit, so I feel like that ties in quite nicely with that. The important bit there (also in reply to your points b) and c) below) is that on frameworks such as UWP XAML and WinUI, you can use either NotifyCanExecuteChanged manually in your viewmodels, or just rely on the framework updating the UI for you when using CommandParameter, and both approach work fine without the need to do any sort of platform specific changes in the backend. That's why I don't really see your points b) and c) on the same plane - b) just works "for free" on UWP/WinUI along with a), whereas c) would need platform specific support. Hope that clarifies my previous point better 😊

EDIT: what about a WPF attached property to just mirror CommandParameter using multi-binding (is this what you had in mind too?), so that this way both UWP XAML, WinUI and WPF would get exactly the same level of support in scenarios a) and b)? 🤔

[thomasclaudiushuber]

@Sergio0694 Ok, I understand your point. In WPF, the framework-specific part (CommandManager) has to sit in the ViewModel, while theCommandParameter property is a ViewModel-independent UI thing that does not sit in the ViewModel. That's a good point, and yes, from that perspective, my points b) and c) are not at the same level. Great argument!

Yes, definitely, adding an attached property to have that UWP/WinUI behavior in WPF would work for CommandParameter. But I'm not sure if the CommandParameter property is actually the need that @laurentkempe has.

@laurentkempe Are you using the CommandParameter property in your WPF projects? Or is the wish for CommandManager more about just using Commands (without CommandParameter) and raising CanExecuteChanged event automatically when any input event happens? (Instead of manually calling NotifyCanExecuteChanged on the commands in the ViewModel)

[sonnemaf]

@thomasclaudiushuber can you use my OnNotifyCommandParameterChangesChanged attached property solution. I just updated it to support MenuItem and Hyperlink too.

[martinskuta]

I was just pointed to this thread by @laurentkempe and found it interesting. I come from WPF world and I have been using several MVVM toolkits and what I find most important is consistency in the behavior. By consistency I mean, if I use the command from the toolkit, no matter what platform I am on, I would expect same behavior. Essentially the main difference between the platforms is how CanExecuteChanged event is fired, there are basically three approaches that you already mentioned: a) Manually call RaiseCanExecuteChanged - platform agnostic b) CommandParamter update triggers CanExecuteChanged - platform specific to WinUI/UWP c) CommandManager - eg on any user input CanExecuteChanged is triggered automatically - platform specific to WPF

Each approach has pros and cons to it, but I am thinking, instead of unsealing the command classes or providing specific command implementation that has same class name, different behavior and is in different namespace (fell down this rabbit hole so many times, that we created static code analyzer to check correct namespace of RelayCommand). From developer point of view I would prefer to have something like the following, behaving always the same no matter the platform (namings are work in progress):

1. ManualRelayCommand - which covers approaches a) + b)
2. AutoRelayCommand - which would on top of a) raise also the CanExecuteChanged whevener user input happens - WPF command manager behavior basically.

The AutoRelayCommand I can imagine would be either part of WPF nuget or I can also imagine that it could be platform agnostic if the toolkit would have its own implementation of "command manager" listening to input events and triggering the CanExecuteChanged automatically, optionally falling back to CommandManager from WPF in case the toolkit is used in WPF application. I think that would be ideal for developers, no need to mess around and extend the implementation etc.

[thomasclaudiushuber]

@sonnemaf Thanks Fons. Your sample is a great starter to learn how to use an attached property to solve the CommandParameter issue. I appreciate the link in this discussion.

But as stated above and also in the linked issue from you, CommandManager is not only about CommandParameter. It's also and I think even especially about binding just the Command property of for example a Button. With CommandManager, you don't have to raise CanExecuteChanged on your Commands to refresh the UI. The CommandManager will do that work for you.

I'm a XAML developer who never uses CommandParameter. I create properties in the ViewModel instead. I believe that's much cleaner from an MVVM perspective. But that's my personal opinion. I don't want to say it's bad to use CommandParameter, but I don't recommend it due to different reasons:

• ViewModel API is not clean, as CommandParameter is an object. Often requires casting
• Logic often sits in XAML instead of the ViewModel, like selection logic. Make it hard to test the ViewModel
• Especially for beginnners CommandParameter seems to be a way to pass anything to the ViewModel, and passing View stuff would break the idea of MVVM. Experienced devs of course don't do that, but most often experience comes after a lot of code has been written.

If you have all the properties for the View in the ViewModel, CommandParameter is not needed.

But I know, some are fans of CommandParameter, so I guess providing an Attached Property for WPF to invalidate commands when the CommandParameter-property changes is a great idea.

But as mentioned, CommandManager goes beyond CommandParameter.

[thomasclaudiushuber]

@martinskuta The AutoRelayCommand is a fantastic idea. I have to admit that I love it, but at the same time it also brings up some old nightmares. 😅

It would actually require the implementation of a platform-specific CommandManager for Non-WPF-Platforms, because the input events are platform-specific (Unless you grab low-level events directly from the application window)

But it's great that you wrote this. I actually thought back in the Silverlight days about creating something like that.

The platform-independent part could be that these commands are collected in a list. So, there must be a list that contains all the AutoRelayCommands. From an implementation point of view, AutoRelayCommands are normal RelayCommands with a piece of logic that adds them to the central list.

So, that list can be platform independent.

Then there needs to be a central place in the platform-specific app where you hook up that list to platform-specific input events. In WPF you can re-use CommandManager instead of manually hooking up to input events, and this would look like this somewhere in App-constructor or App-Startup event

CommandManager.RequerySuggested+=(e,args)=>
{
  foreach(var command in listOfAutoRelayCommands)
  {
     command.NotifyCanExecuteChanged();
  }
};

But when I tried this, I noticed it's really hard in bigger applications. When you have for example a tabbed UI with many commands, you don't want to raise CanExecute all the time for all the commands. Then you start thinking about how to detect if the command target is visible on the screen. Is this really something a command should know about?

Then there's the approach that does it on a "per-ViewModel-basis", and that one is also platform-independent and maybe even better.

In the SetProperty method that is usually in a ViewModelBase class - or here ObservableObject - where you raise NotifyPropertyChanged, you can also use Reflection to grab all the ICommand Properties of the ViewModel and call NotifyCanExecuteChanged on all of them. And that would be something that you could configure on a ViewModel. It's a platform-independent approach that gives you a bit of CommandManager feeling per ViewModel. It does not work across ViewModels though.

But in the end, all these "raise-it-automatically" approaches are not easy. So, I decided to just use RelayCommand and call NotifyCanExecuteChanged manually. This is what I did in the past years. Hey, doesn't seem too bad, my ViewModels work across all platforms. :)

[sonnemaf]

@martinskuta, think the AutoRelayCommand isn't the way forward. What to do with the AsyncRelayCommand. Do you also introduce an AutoAsyncRelayCommand?

@thomasclaudiushuber, I use sometimes CommandParameters but most of the times I create the properties in my ViewModel. It is just one of the many solutions. And yes we will have to call NotifyCanExecuteChanged manually. That is not too bad. I like to be in control. Never liked/used the CommandManager so no problems for me.

[Sergio0694]

Commenting here even though the prototype I have is still rough around the edges. I've experimented with an attached property that (seems to) solve the issue in all cases. Essentially, consider this code (deliberately not using CommandParameter):

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="*"/>
        <RowDefinition Height="60"/>
    </Grid.RowDefinitions>
    <TextBox Text="{Binding Query, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
    <Button
        Grid.Row="1"
        Content="CLICK ME"
        Command="{Binding WriteCommand}"
        ex:RelayCommandExtensions.IsCommandUpdateEnabled="True"/>
</Grid>

Note that RelayCommandExtensions.IsCommandUpdateEnabled extension. The command in use has a simple CanExecute function that checks whether the Query property in the viewmodel is not null or empty. This is the result when using the app, without the viewmodel ever calling NotifyCanExecuteChanged - everything is done automatically on the UI side:

As in, with something like this, developers would only need to set that attached property and then use the same classes as usual. Would something like this work? 🙂

EDIT: to clarify, this extension supports all cases mentioned above - either users manually using NotifyCanExecuteChanged, using CommandParameter, or just not doing anything at all and letting CommandManager do its thing. They all work on WPF 😄

[laurentkempe]

@laurentkempe Are you using the CommandParameter property in your WPF projects? Or is the wish for CommandManager more about just using Commands (without CommandParameter) and raising CanExecuteChanged event automatically when any input event happens? (Instead of manually calling NotifyCanExecuteChanged on the commands in the ViewModel)

Yes @thomasclaudiushuber, we are using CommandParameter property in our WPF project but it is more about raising CanExecuteChanged event automatically! Otherwise moving from MVVMLight we would need to get back to our huge code base and do the NotifyCanExecuteChanged where it is needed which would be a killer for us.

[Sergio0694]

@laurentkempe Would an extension like the one I showed in my previous message work in your use case scenario? That would allow you to leave your entire backend as is, without any changes (no need to add NotifyCanExecuteChanged calls there), and you'd just need to add that line with the RelayCommandExtensions.IsCommandUpdateEnabled property next to your command bindings.

[laurentkempe]

Honestly @Sergio0694, we already migrated our application to our own RelayCommands implementation for WPF from MVVMLight. So, I don't think we would migrate again to the WCT, even more if we need to go through the whole code and add such an extension. I started the whole discussion knowing the problem we had with MVVMLight and looking at WCT I saw that something was missing for WPF to ease the migration from MVVMLight in that area. It was the argument you pointed for not changing the Messenger.Default which we discussed in the other ticket, which made me understand that you want to ease the migration and have a cross platform framework. But I guess here we see one limitation to those goals which cannot be achieved.

[Sergio0694]

I'm sorry to hear you don't plan to move to the MVVM Toolkit, but I understand where you're coming from.

I guess I should rephrase my question then: hypothetically speaking and going back in time to when you had to write your own WPF-specific RelayCommand type for your application, would this extension have been a valid solution for you then? Because if you say you had to write your own RelayCommand class and migrate your codebase to that, then the question is not about asking developers to do some work, or none - but rather whether doing this work in particular (ie. adding an extension instead of writing their own WPF-specific command) is a good solution. Also considering the fact that such an extension would in return offer more flexibility in the end, as it'd remain a purely UI-related implementation detail, while the rest of the backend would be able to remain completely platform agnostic and easily portable/testable as well. I'm particularly curious because we do care about finding a good migration path for other existing WPF devs that might be looking at the MVVM Toolkit as well 🙂

Curious to hear your thoughts!

[michael-hawker]

Just wanted to point out in the future it'd be great if we could somehow adapt WPF to help with these scenarios directly now that it's open source. There was some discussion of this on their thread for it here: https://github.com/dotnet/wpf/issues/434.

In the meantime, @laurentkempe and @thomasclaudiushuber do you think @Sergio0694's more general solution of having a IsCommandUpdateEnabled would work? https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3609#issuecomment-740721298

[martinskuta]

@martinskuta, think the AutoRelayCommand isn't the way forward. What to do with the AsyncRelayCommand. Do you also introduce an AutoAsyncRelayCommand?

@sonnemaf Yes, another implementation for AutoAsyncRelayCommand, because I think it is important to express that they are different implementation that automatically refresh, but I also understand if you don't want to go that way.

The platform-independent part could be that these commands are collected in a list. So, there must be a list that contains all the AutoRelayCommands. From an implementation point of view, AutoRelayCommands are normal RelayCommands with a piece of logic that adds them to the central list.

@thomasclaudiushuber Yes there are many ways this could be implemented. We also have now access to real implementation of CommandManager from WPF which is already proven to work well. So I thought it would be great to have something like that available on all platforms in one package.

From WPF developer point of view, many of us are used to use RelayCommands based on CommandManager, because when you learn WPF, CommandManager is one of the APIs you are thought to use and also you don't need to add "extra" code to call RaiseCanExecuteChanged, because less code to maintain is better right? So I was thinking it would be nice to have the automagical part provided by the toolkit, because in the end, unless it is a small application, if the commands in your code base depend on the CommandManager, the effort to start using "ManualRelayCommand" is big and it brings lots of risks with it, because suddenly it becomes your code responsibility to notify that can execute changed and you need to also prove it with tests, so I think WPF devs will most likely not use RelayCommand from the toolkit, they will either have their own implementation (like we do in our project) or they would use some WPF toolkit that has the commands implemented to use CommandManager. Also the attached property to raise can execute changed for CommandParameter will not help in the migration.

EDIT: To prove it by tests I mean to add asserts that when something changes the CanExecuteChanged event is raised, we, and hopefully everybode else, already have tests that if this and that is set CanExecute returns false etc.

[thomasclaudiushuber]

Just wanted to point out in the future it'd be great if we could somehow adapt WPF to help with these scenarios directly now that it's open source. There was some discussion of this on their thread for it here: dotnet/wpf#434.

@michael-hawker That's a good point.

In the meantime, @laurentkempe and @thomasclaudiushuber do you think @Sergio0694's more general solution of having a IsCommandUpdateEnabled would work? #3609 (comment)

It would work, but in my opinion it is not ideal. I know we could set the attached property via Styles, but anyway, it requires changes in XAML to migrate to the new toolkit. But on the other side, I like the fact that it could theoretically be an approach that works with all XAML platforms. (I assume @Sergio0694 hooked up WPF's CommandManager behind the scenes).

Instead of the attached property I would prefer a package with WPF-specific command implementations instead of that attached property. It requires far less changes for developers migrating to the new toolkit with that additional WPF-specific package.

I would suggest the following instead of the attached property: Create a new WPF-specific class library project in the toolkit called Microsoft.Toolkit.Mvvm.Input.Wpf. As inheritance is not something wanted like in this PR, link the Command files of the Microsoft.Toolkit.Mvvm package in the new project and do an "#if WPF" to hook up CommandManager for WPF. That's it. I think it doesn't make the toolkit less universal and modern, and it doesn't hurt.

But I think I'm not the person who should answer this here, as I personally don't use CommandManager, and I wouldn't have requested to bring it to the MVVM toolkit. I just got interested in the other issue as I know very well (at least I think this 😂) how CommandManager works together with MVVM and also with WPF's RoutedCommands (that rely also on routed events).

But I'm curious what @laurentkempe and @martinskuta think about attached property vs. package with WPF-specific commands.

[Sergio0694]

Instead of the attached property I would prefer a package with WPF-specific command implementations instead of that attached property. It requires far less changes for developers migrating to the new toolkit with that additional WPF-specific package.

I'm a bit confused as to why this would be considered a better solution. I mean, I get the point about not having to do any changes in XAML, of course, but this would effectively mean having to reference an additional library and most importantly, completely preventing your entire backend from being in a separate, portable library - it would get stuck on WPF, with all the related downsides that'd involve, such as more difficulty with testing, possibly having to multi-target and use directives, etc. 🤔

Curious if this is more about the necessary work to port existing apps rather than building new ones with the MVVM Toolkit?

[michael-hawker]

Instead of the attached property I would prefer a package with WPF-specific command implementations instead of that attached property. It requires far less changes for developers migrating to the new toolkit with that additional WPF-specific package.

I would suggest the following instead of the attached property: Create a new WPF-specific class library project in the toolkit called Microsoft.Toolkit.Mvvm.Input.Wpf. As inheritance is not something wanted like in this PR, link the Command files of the Microsoft.Toolkit.Mvvm package in the new project and do an "#if WPF" to hook up CommandManager for WPF. That's it. I think it doesn't make the toolkit less universal and modern, and it doesn't hurt.

There's a lot of overhead in us having another package. Though it isn't unprecedented. We do have a WPF based package for our Graph Behaviors, but that's just using conditional compilation to include the alternate Behavior dependency needed for WPF vs UWP the rest of the code is the same.

We just don't want to open up the gates for the MVVM Toolkit to start having specific packages for every platform to work around specific issues. We really want this to be platform agnostic and work towards general solutions or improvements to the underlying platforms. That doesn't mean we couldn't see another project shipping platforms solutions built on top of the toolkit or just having some small tweaks/helpers within our samples to show how to use the Toolkit more effectively. We just don't want to be shipping a whole other package to do it.

I'm good with trying to open up the classes if that helps others build on top of things from the #3610 PR as long as that doesn't have performance, side-effect, or package size impacts.

I think @Sergio0694's point is to propose how we can move the entire ecosystem forward and improve scenarios for everyone. If the CommandManager is that important to bring forward for WPF developers and application construction, it should also be called out on the WinUI issue tracking the gap here: https://github.com/microsoft/microsoft-ui-xaml/issues/719. FYI @predavid

[thomasclaudiushuber]

Curious if this is more about the necessary work to port existing apps rather than building new ones with the MVVM Toolkit?

I think the point is that there are still many developers who do just WPF. For them it's not important to put the ViewModels in a .NET Standard library, it's absolutely ok to have them WPF-specific. They will build WPF apps for the next 5-10 years, as the company decided that this is the tech-stack to move forward. I know many developers like this. Multi-targeting, .NET Standard etc., all this is not something that is important for them. (Doesn't mean that it's not important for others). So, the downsides you mentioned are not really downsides for them.

If the idea is to implement that attached property for all platforms, then I see your point.

If the idea is to make the attached property only for WPF as a replacement for CommandManager, then I don't see the point. Because you would still have WPF-specific ViewModels that won't work on any other XAML platform without that attached property, as they don't call NotifyCanExecuteChangedon the commands. And why bringing something (CommandManager-like) to other platforms what might not be today's best way to do it? I would just support it as a platform-specific extension as it exists in WPF, or I wouldn't support it.

but this would effectively mean having to reference an additional library

Where does the attached property come from? Also from an additional library I guess?

If companies allow to reference Microsoft.Toolkit.Mvvm, they also allow to reference Microsoft.Toolkit.Mvvm.Input.Wpf. A second package is usually not a problem. I don't see companies who want to multi-target their ViewModels, but I'm not saying that does not exist.

Note: I'm looking at this through my line-of-business-app-glasses, where people work with desktops on ethernet, where it doesn't matter if the app is 100Mb or 120Mb. :)

Hope this makes it clear. So, it really depends. But I think the attached property makes only sense if you plan to create it for all platforms, and I think that's not worth it, as you wouldn't expect something like that in UWP/WinUI. Instead of doing that, I would rather say MVVM toolkit doesn't support CommandManager, but there is a sample app that shows you how you can build your own Command-classes to use CommandManager in your WPF app.

Personally, I would say CommandManager is not super important. I would fully stand behind the decision if you say: Hey, we don't support it in our modern toolkit, as it is not a future-proof approach, it's not part of modern XAML platforms. If you migrate from WPF, you have to use NotifyCanExecuteChanged (CommandParameter would still require WPF-specific extensions though). If you need CommandManager, you can build your own commands, which is actually what @laurentkempe did, if I read correctly.

[laurentkempe]

I'm sorry to hear you don't plan to move to the MVVM Toolkit, but I understand where you're coming from.

🙈 We are at least looking to use the Messenger part! btw @Sergio0694 you should activate the Github discussions (available since yesterday) on this project, I think it helps in this kind of discussions.

I guess I should rephrase my question then: hypothetically speaking and going back in time to when you had to write your own WPF-specific RelayCommand type for your application, would this extension have been a valid solution for you then?

I would say no, because this would involve too much work from our side to check all the places to add this extension. I definitely think that WCT should provide a straight update from MVVMLight, which is one goal I understood and liked about WCT.

Because if you say you had to write your own RelayCommand class and migrate your codebase to that, then the question is not about asking developers to do some work, or none - but rather whether doing this work in particular (ie. adding an extension instead of writing their own WPF-specific command) is a good solution. Also considering the fact that such an extension would in return offer more flexibility in the end, as it'd remain a purely UI-related implementation detail, while the rest of the backend would be able to remain completely platform agnostic and easily portable/testable as well.

A bit of context, we were doing the migration of our WPF application from .NET Framework 4.7.2 to .NET Core 3.1. So, we updated our 3rd party libs and went to the .NET Standard version of MVVMLight to realize that the RelayCommand was not behaving like the WPF version prior to .NET Standard. In fact, exactly the same issue that we are talking about today for WCT. We decided that having our own RelayCommand in our code base was the easiest solution. And with refactoring tools from R# that change was a matter of some minutes. The extension change you are proposing with the extension would be much more effort to introduce in our big code base. So, is we would have to do the change today, and you would propose the extension, I guess we would go same way to have our own RelayCommand.

while the rest of the backend would be able to remain completely platform agnostic and easily portable/testable as well.

I see the argument but in our particular situation, this has no real value. We won't go away from WPF.

I'm particularly curious because we do care about finding a good migration path for other existing WPF devs that might be looking at the MVVM Toolkit as well 🙂

Great ❤️

[laurentkempe]

Instead of the attached property I would prefer a package with WPF-specific command implementations instead of that attached property. It requires far less changes for developers migrating to the new toolkit with that additional WPF-specific package.

Right on the point @thomasclaudiushuber that's exactly why I would prefer that solution in our case! For sure, if you have a small project and needs to update a couple of places it is quite different.

But I'm curious what @laurentkempe and @martinskuta think about attached property vs. package with WPF-specific commands.

😄 I think it should be clear now

@Sergio0694

completely preventing your entire backend from being in a separate, portable library - it would get stuck on WPF, with all the related downsides that'd involve, such as more difficulty with testing, possibly having to multi-target and use directives, etc.

We don't have any need to have our entire backend in a portable library! We are using WPF and we have no plan to move away from it.

@michael-hawker

We just don't want to open up the gates for the MVVM Toolkit to start having specific packages for every platform to work around specific issues. We really want this to be platform agnostic and work towards general solutions or improvements to the underlying platforms. That doesn't mean we couldn't see another project shipping platforms solutions built on top of the toolkit or just having some small tweaks/helpers within our samples to show how to use the Toolkit more effectively.

Now that we understand the goal of WCT with MVVM, it is clear why you don't want to have it. Yes, another project could ship a platform solution fixing this particular issue. In that case, WCT MVVM needs to have a good communication so that people migrating to it from MVVMLight and WPF would not fall in this trap, cause this would create lots of frustration.

@thomasclaudiushuber

I would rather say MVVM toolkit doesn't support CommandManager, but there is a sample app that shows you how you can build your own Command-classes to use CommandManager in your WPF app. Personally, I would say CommandManager is not super important. I would fully stand behind the decision if you say: Hey, we don't support it in our modern toolkit, as it is not a future-proof approach, it's not part of modern XAML platforms. If you migrate from WPF, you have to use NotifyCanExecuteChanged (CommandParameter would still require WPF-specific extensions though). If you need CommandManager, you can build your own commands, which is actually what @laurentkempe did, if I read correctly.

Exactly it has to be super clear for people moving to it that CommandManager is not supported!

[thomasclaudiushuber]

@laurentkempe @michael-hawker @Sergio0694 @sonnemaf @martinskuta Great arguments and discussion. ❤

Maybe we should make a decision regarding the initial thought of this issue: Making the Command-classes sealed or not. If the WPF-CommandManager-case is the only reason to make the classes non-sealed, it might be a better idea to keep them sealed and do a re-implementation for WPF in a separate library (that can be in the WCT or just another OS project). If we think using CommandManager is not today's best approach, we should also keep them sealed, so that it's clear: Hey, you have to re-implement the whole thing if you want to use CommandManager. You find a WPF-MVVM sample in the MVVM-samples of the toolkit that shows you how to do that.

With all the points of the discussion, I think I would keep them sealed. There's still the option for the WCT to have linked files and a separate Microsoft.Toolkit.Mvvm.Input.WpfCommandManager project.

Please vote:

👍 Keep the Command-classes sealed

👎 Remove the sealed keyword and make CanExecuteChanged a virtual event

[thomasclaudiushuber]

Thanks @laurentkempe @michael-hawker @Sergio0694 @sonnemaf @martinskuta. I think the 4 votings out of 6 is already enough to say that we keep the Command-classes sealed (I think it's also what is preferred by those who didn't vote yet)

I think it was good and great to have this wonderful discussion here, and to have a decision. I close this issue and the corresponding PR.