Reduce Ambiguity in MVVM (Full) CommandBinding #12
Conversation
CommandBinding uses internally stored ViewModel for the Execute action. Doesn't make sense to evaluate CanExecute in an arbitrary context - it should also use the stored ViewModel. Also requires change to ICommandBinding
See other proposed change as well. Breaking? change to interface, but removes confusion caused by a CommandBinding potentially evaluating CanExecute in an arbitrary context (Context as object) but always executing the Command with respect to an internally stored ViewModel
Update ICommandBinding.cls
|
Hi there, thanks for your interest in this!
It's been a while, but IIRC the idea was to make sure the command parameter used for evaluating CanExecute would be consistent with the one given to Execute, which - in theory - may or may not be the ViewModel... except I cut corners here and kind of turned the concept of CommandParameter into some "binding context" i.e. the ViewModel outright. I don't disagree with this modification since it's in line with the simplification of command parameterization. OTOH I'm kind of torn, because I would have liked to end up tweaking it the other way around, meaning to undo the corner-cutting and come up with a way to bind command parameters that's closer to how WPF does it, with the greater flexibility afforded by such specialized bindings... But it's been a long time and I'm probably not going to dig into MVVM until Rubberduck 3.0 is at least in beta release, and the additional complexity that comes with the flexibility might actually not really be worth it (since at the end of the day most command parameters would be bound to ViewModel properties anyway).. 🤔 |
| End Sub | ||
|
|
||
| Private Sub EvaluateCanExecute(ByVal Source As Object) |
There was a problem hiding this comment.
I wouldn't remove the parentheses though
|
First time navigating through all of this on GitHub, but I think i've made the changes to put the parantheses back in for the signature. The 'ponder' emoji at end of 1st comment hits nail on head; as a teaching / example tool, the inconsistency threw me for a while because of the Context being passed around, but ViewModel being used within CommandBinding in one place, but not the other. The one consideration you will be in far better place to judge is whether enough people are using this example set of code such that making this change is 'bad idea' since it is not backwards compatible (not terribly so - but its changing an interface signature and that would make previous code that coded against the interface not compile / work) No worries if better to reject this pull request - learning how to work this process has been valuable enough. |
First time using github / proposing changes so apologies if wrong process followed here.
Summary of proposed change (also hopefully documented in specific commits):
Changes should update MVVM example to update ICommandBinding and CommandBinding classes to remove the Context object parameter input to "EvaluateCanExecute" in the ICommandBinding interface.
Couple reasons to remove the Context input to EvaluateCanExcute
The current interface requires a caller/user of the interface to know the Context the CommandBinding is going to 'auto-execute' the Command in (e.g. to be carrying around a copy of ViewModel since the current implementation of CommandBinding will only execute in that stored context) in order to use ICommandBinding.EvaluateCanExecute.
Anything that will want to evaluate the command's canexecute in a alternate specific context can still access the command's can execute through the Command property on the ICommand interface -
As an alternative, one could change the signature for EvaluateCanExecute to EvaluateCanExecute(Optional Context as object). This would have advantage of not making it a breaking change... but this would still leave an ambiguity/confusion between the interface and the specific implementation provided as an example - and it seems to me as this is provided as a teaching example / potential framework, the purpose of a CommandBinding is to encapsulate all of the dependencies on the linkage and so should be independent of evaluating the Command in arbitrary contexts.
Above offered in light of reading about MVVM-lite and hopefully is helpful