Caliburn.Micro icon indicating copy to clipboard operation
Caliburn.Micro copied to clipboard

KeyBinding Issue when attaching it to windows with commands with CanExecute methods

Open ahmed-abdelrazek opened this issue 5 years ago • 15 comments
trafficstars

If i add for example cal:Message.Attach="[Key Enter] = [EnterPressed]" to the window/page/panel etc and the EnterPressed command had a false CanExecute method it will set IsEnable property for that controller to false and that will disable the window/page/panel and every element in it and if that KeyBinding have more that one combination and they all true but one it will also do the same thing

an example project https://github.com/ahmedflix25/CaliburnMicroKeyBinding

if the CanExecute method is false it should only disable that KeyBinding and nothing else

ahmed-abdelrazek avatar Jan 09 '20 10:01 ahmed-abdelrazek

I can confirm this problem.

When guard method prevents execution, Caliburn disables the control associated with the action. In this case it disables whatever control has the KeyTrigger, since it technically is target of the action. Even tho that in our case, the control is only used to capture KeyDown events.

Filipsi avatar Mar 06 '20 09:03 Filipsi

a work around would be something like this

void KeyTriggerForStuff() { if (CanDoStuff) { DoStuff(); } }

and you just bind the trigger to that new method

ahmed-abdelrazek avatar Mar 06 '20 12:03 ahmed-abdelrazek

Possible solution might be addition of new dependency property to ActionMessage that would prevent it from applying availability effects to the associated Control. This might also come in handy outside of the keybinding scenario.

Another change would involve extension of the Parser that would allow message customization based on Trigger type. I don't know if this is a best approach to the problem, but I would like to see this resolved in some way.

Filipsi avatar Aug 24 '21 17:08 Filipsi

Could you not just omit the CanExecute method? If the Parser was to know about Trigger type then it would have to be customizable for the user. I supposed the simple solution would be to add a do not validate CanExecute property on the ActionMessage, but then again why not just omit the CanExecute method in the first place?

KasperSK avatar Aug 24 '21 20:08 KasperSK

Yes, omitting the CanExecute method is the simplest answer. But the problem, in my opinion, is in the usability.

Imagine a scenario where you have some buttons, all of them performing an action with associated CanExecute guard methods so they get automatically disabled. You want all of these actions to also have a keyboard shortcut, so you add keybindings to the container and Attach them to the same actions the buttons are using. Now if any of the buttons gets disabled by their guard method whole container get disabled due to the keybinding.

You can solve this by omitting the CanExecute method and explicitly binding the IsEnabled property for each button. But this defeats the whole concept of using named conventions for automatic action binding that Caliburn.Micro does so well.

Or you can create duplicate methods in the ViewModel for each keybinding that does not have a guard method but does the same logic as the action that the buttons are attached to. But this clutters the ViewModel with multiple new methods and makes the API the ViewModel is providing less clean and harder to use.

These solutions work, but I don't think any of them is a good solution.

Filipsi avatar Aug 26 '21 18:08 Filipsi

Point taken, I think that an additional property on the ActionMessage is the right choice. I will look into when I have some free time.

KasperSK avatar Aug 27 '21 07:08 KasperSK