-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Feature] MVVM Toolkit - Remove 'sealed' from the four Command Classes to allow extension #3609
Comments
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! |
Hey @thomasclaudiushuber - thank you for the feedbacks! 😄 I'll start with the As for the proposal to unseal the command types and make the event virtual, I'm afraid I don't really like that 😟
EDIT: if we wanted to offer a Hope that helps! 🙂 |
Hi @Sergio0694 , thanks for your reply.
Yes, is already done like that in the PR for this issue.
I don't understand this. It would behave exactly the way how people know Commands using
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 The question is: Is it worth to allow extension if the only use case is WPF's 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.
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 |
Ah, sorry I had missed the PR, as it wasn't linked to this issue. I've fixed that now 😄
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
I didn't phrase that part properly, let me clarify that. Take the
Yes, that's really main main concern here at the end of the day 🤔
Yes, if we really were to actually make a Leaaving this issue open and adding the "open discussion" tag, so that others can chime in as well with their thoughts 😊 |
Thanks for linking it, I owe you a cup of coffee. :-)
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. :)
Ok, got it. I think you're refering here to 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: 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. |
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.
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. |
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
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 EDIT: what about a WPF attached property to just mirror |
@Sergio0694 Ok, I understand your point. In WPF, the framework-specific part ( Yes, definitely, adding an attached property to have that UWP/WinUI behavior in WPF would work for @laurentkempe Are you using the |
@thomasclaudiushuber can you use my OnNotifyCommandParameterChangesChanged attached property solution. I just updated it to support MenuItem and Hyperlink too. |
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: 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):
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. |
@sonnemaf Thanks Fons. Your sample is a great starter to learn how to use an attached property to solve the But as stated above and also in the linked issue from you, I'm a XAML developer who never uses
If you have all the properties for the View in the ViewModel, But I know, some are fans of But as mentioned, |
@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
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 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. :) |
@martinskuta, think the @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. |
Commenting here even though the prototype I have is still rough around the edges. <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 As in, with something like this, developers would only need to set that attached property and then use the same classes as usual. EDIT: to clarify, this extension supports all cases mentioned above - either users manually using |
Yes @thomasclaudiushuber, we are using |
@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 |
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'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 Curious to hear your thoughts! |
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. In the meantime, @laurentkempe and @thomasclaudiushuber do you think @Sergio0694's more general solution of having a |
@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.
@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. |
@michael-hawker That's a good point.
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: But I think I'm not the person who should answer this here, as I personally don't use But I'm curious what @laurentkempe and @martinskuta think about attached property vs. package with WPF-specific commands. |
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? |
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 |
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
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 Personally, I would say |
🙈 We are at least looking to use the Messenger part!
I would say no, because this would involve too much work from our side to check all the places to add this extension.
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
I see the argument but in our particular situation, this has no real value. We won't go away from WPF.
Great ❤️ |
Right on the point @thomasclaudiushuber that's exactly why I would prefer that solution in our case!
😄 I think it should be clear now
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.
Now that we understand the goal of WCT with MVVM, it is clear why you don't want to have it.
Exactly it has to be super clear for people moving to it that |
@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 With all the points of the discussion, I think I would keep them Please vote: 👍 Keep the Command-classes sealed 👎 Remove the sealed keyword and make CanExecuteChanged a virtual event |
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. |
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 inMicrosoft.Toolkit.Mvvm.Input
.Also mark at least the
CanExecuteChanged
event in those command classes asvirtual
: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:
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, theRelayCommand
of the Toolkit and the otherICommand
implementations should be open for extension to write the below code:If even the additional CanAlwaysExecute property is added, the WPF-specific command could look like this:
The text was updated successfully, but these errors were encountered: