microsoft / XamlBehaviorsWpf

Home for WPF XAML Behaviors on GitHub.
MIT License
852 stars 140 forks source link

fixing modifiers behavior #166

Closed gianlucacuccostatsperform closed 6 months ago

gianlucacuccostatsperform commented 6 months ago

Description of Change

We add back the previous behavior described in the issue https://github.com/microsoft/XamlBehaviorsWpf/issues/162 and added backward compatiblity to https://github.com/microsoft/XamlBehaviorsWpf/issues/77

Bugs Fixed

Fixing issue https://github.com/microsoft/XamlBehaviorsWpf/issues/162

API Changes

List all API changes here (or just put None), example:

Added:

Changed:

Behavioral Changes

We added a new dependency property to set if the modifiers have to be considered as normal key button or a real modifier

PR Checklist

brianlagunas commented 6 months ago

I'm not a fan of this approach. I don't like the idea of adding a property to control this. The primary issue is if you have two commands using the same key, if one has a modifier and the other doesn't, regardless of if you only want to invoke the modifier-based key, both keys will invoke.

I think the logic has to be updated to check the key and modifier combination, and do not invoke the command unless both of those matches.

Also, you are missing a unit test for your PR.

gianlucacuccostatsperform commented 6 months ago

I'm not a fan of this approach. I don't like the idea of adding a property to control this. The primary issue is if you have two commands using the same key, if one has a modifier and the other doesn't, regardless of if you only want to invoke the modifier-based key, both keys will invoke.

I think the logic has to be updated to check the key and modifier combination, and do not invoke the command unless both of those matches.

Also, you are missing a unit test for your PR.

yes i'm agree with you but we need to refactor that, i can't see a way to implement that with small changes, adding that property was the easiest way that i found with less changes as possible

brianlagunas commented 6 months ago

Thank you for the effort, but as discussed, this is not the preferred approach.