Powertoys: Add ability to disable a shortcut or key in UI

[saahmedm]

this is already present in the backend, need to add design + implement for the UI

[arjunbalgovind]

@crutkas @ryanbodrug-microsoft @saahmedm this feature needs a spec in terms of UI design/placement - such as would it be just another entry in the drop down like "disable" or something else altogether. Implementation for disabling a key is straightforward and is already present in the backend. Implementation for disabling a shortcut requires a decision on whether it should happen only if exactly those shortcut keys are pressed (like shortcut to shortcut remaps) or even if the shortcut is pressed in combination with other keys (like shortcut to key remaps).

[arjunbalgovind]

@jmahlers This is the part of the code where we have the "disabling a key" feature https://github.com/microsoft/PowerToys/blob/fc4ac803aad4fd0341e0d5bb5d11ffc75aef869c/src/modules/keyboardmanager/dll/KeyboardEventHandlers.cpp#L27-L33 but we haven't exposed it to the UI yet. Currently for Remap keys, the drop down doesn't have the None option at the top, so it would have to be added in as well, similar to what is done for shortcuts, as done here. There would be other areas of the code where this has to be changed as well.
https://github.com/microsoft/PowerToys/blob/fc4ac803aad4fd0341e0d5bb5d11ffc75aef869c/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp#L29

For disabling shortcuts a similar change would be required in the backend, and in the UI this area of the logic would have to be tweaked to allow selecting None if there is a single drop down, but only on the right side column (column index = 0)
https://github.com/microsoft/PowerToys/blob/fc4ac803aad4fd0341e0d5bb5d11ffc75aef869c/src/modules/keyboardmanager/ui/BufferValidationHelpers.cpp#L76-L127

[jmahlers]

@arjunbalgovind Thanks,

So there actually already is the "none" option you mentioned at the top of the second column of the key remapper, however if it is selected
https://github.com/microsoft/PowerToys/blob/fc4ac803aad4fd0341e0d5bb5d11ffc75aef869c/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp#L155-L158
and a few other pieces of code reset the selection.

What I've found, is if you override these safeties and forcibly input none, rather than writing "0" to the json it writes "". Then, when you attempt to use the binding, the app crashes. There are some other UI issues when you do this because it's trying to fix the empty key binding, but I'm going to try and figure out this backend portion.
This is the relevant code:
https://github.com/microsoft/PowerToys/blob/ff1e04b9577a2961bb82b033ded9629a7d74051a/src/modules/keyboardmanager/common/KeyboardManagerState.cpp#L465-L475

I'll see if I can figure out why its writing "". Hopefully, it can be an easy fix.

[jmahlers]

I think I've figured out why choosing "none" breaks everything.
https://github.com/microsoft/PowerToys/blob/ff1e04b9577a2961bb82b033ded9629a7d74051a/src/modules/keyboardmanager/common/KeyboardManagerState.cpp#L465-L480
Looking in the debugger as the above code gets processed, it.second.index is 0 for key to key remappings.
https://github.com/microsoft/PowerToys/blob/ff1e04b9577a2961bb82b033ded9629a7d74051a/src/modules/keyboardmanager/common/KeyboardManagerState.h#L94
For "none" it.second.index is 1, causing code for key -> shortcut to run instead:
https://github.com/microsoft/PowerToys/blob/ff1e04b9577a2961bb82b033ded9629a7d74051a/src/modules/keyboardmanager/common/KeyboardManagerState.cpp#L479

If in the debugger, you use the UI to add a key to key remapping and then with a breakpoint change it to use NULL the code works fine and writes "0" to the json.

[jmahlers]

https://github.com/jmahlers/PowerToys

So I have been able to get choosing "none" sorta working in the above fork. I say sorta because I have adding the key remapping, and using the key mapping after restarting PowerToys both working.

However, if a keymapping is added and then immediately used, the app crashes.

The issue is that, when you first add the keymapping to "none", it is a shortcut with index==1. However, when it is read from the json at startup, it is read as a key to key remapping with index==0.

https://github.com/microsoft/PowerToys/blob/ff1e04b9577a2961bb82b033ded9629a7d74051a/src/modules/keyboardmanager/common/KeyboardManagerState.cpp#L471
This is an example of the index I'm talking about.

A lot of the changes in that fork/initial issues with the main branch with choosing "none" are due to "none" being added as a shortcut with index==1.

So long story short, the none option should correspond to a key with index==0. Where does this index get set to 1 or 0?

I feel like I've been going in circles in the source code trying to figure out a) how new keys are added to the buffer and b) more importantly how new keys are created/where the index is set to 1 or 0?

[arjunbalgovind]

@jmahlers this is the part of the code that you need to look at. The index gets set by the std::variant class depending on if you assign a DWORD or Shortcut to it. The code below is where the assignment occurs after you make a selection. The issue you're facing is that the method GetKeyCodesFromSelectedIndices filters out blank drop down selections as well as None, since otherwise we would run into issues if there is a selection like "Ctrl, None, A" or something. The code below checks if the number of returned keys is exactly one. I think the correct way to go about this would be to modify GetKeyCodesFromSelectedIndices such that if there is only one non -1 drop down and it has None, it will not skip it, but in all other scenarios you would still have to do that. That should fix all the remaining issues.
https://github.com/microsoft/PowerToys/blob/777e1dc48cd4a8a1af5c1258c34e2fc9d7e356c1/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp#L182-L201

Looking at your fork, I don't think any changes were required in keyboard_layout.cpp or Shortcut.cpp. You might have to verify the logic where it gets added to the keyboard manager state though, i.e. https://github.com/microsoft/PowerToys/blob/777e1dc48cd4a8a1af5c1258c34e2fc9d7e356c1/src/modules/keyboardmanager/ui/EditKeyboardWindow.cpp#L274

since there are checks that that the value should not be NULL, which is 0. Maybe the correct way would be to change None to be something like 261, so its a brand new key code, rather than using NULL, since by default all the buffers are initialized to NULL.

[jmahlers]

@arjunbalgovind Thanks again, once I figured out how to have "none" be interpreted as a DWORD rather than a Shortcut, it became a lot less convoluted. I redid the fork following that to just have the actual changes.

That pull request is with that in mind. Once it was a DWORD, that code in keyboard_layout.cpp and Shortcut.cpp became unnecessary.

That pull request above turns on the backend for mapping a key to null.

[saahmedm]

@arjunbalgovind, removing the needs-spec tag. We can implement similar to #6236 but with a new target called "Delete"

[crutkas]

Delete has a pretty defined context and is on keyboards. Could this cause confusion?

[jmahlers]

Perhaps "Disabled"? I agree that delete isn't a good move since DEL already exists.

[enricogior]

@arjunbalgovind
is someone already implementing this? I'm asking because I would like someone from my team to work on it to start familirazing with the KM code base.

[arjunbalgovind]

@enricogior There's a PR from a community member (@jmahlers) which requires some tweaks

[crutkas]

@enricogior, #6236 is the PR

[crutkas]

@jmahlers would you be ok if @enricogior team does the shortcut side and unit tests? or is that something you'd like to do

[jmahlers]

@crutkas @enricogior Absolutely, right now shortcut -> "disabled" somewhat works due to good error handling on the key event side.

If I understand correctly, essentially what needs to be done now is building in logic to properly handle that like the existing logic for key remappings.

[enricogior]

@jmahlers
OK, so @mykhailopylyp will pick it up from the existing PR to complete the work.
For simplicity, I suggest @mykhailopylyp can create a new PR and give credit to you rather than contributing to your existing PR.

[mykhailopylyp]

@crutkas @enricogior

Implementation for disabling a shortcut requires a decision on whether it should happen only if exactly those shortcut keys are pressed (like shortcut to shortcut remaps) or even if the shortcut is pressed in combination with other keys (like shortcut to key remaps).

Above decision has to be made. It does not seems like trivial choice.

[arjunbalgovind]

For more context on the above comment, right now shortcut to shortcut remappings happen only if those exact keys are pressed (so that a Ctrl+A remap does not affect Ctrl+Shift+A remap), whereas shortcut to key and key to key remappings happen even in combination with other keys (example if a user doesn't have a B key, they can remap Ctrl+A to B, and they can invoke Alt+B by pressing Alt+Ctrl+A). For disable I think it makes more sense to be similar to shortcut to shortcut, because a user may want to disable Ctrl+A but not Shift+Ctrl+A.
@crutkas @enricogior thoughts?

[enricogior]

@arjunbalgovind
I agree, only exact match for disable.

[crutkas]

In the 0.25 release. https://github.com/microsoft/PowerToys/releases/tag/v0.25.0

[h4wwk3ye]

It's not working as intended. I disabled the left alt key but after that none of the shortcuts that use the alt key are working.

[jmahlers]

It's not working as intended. I disabled the left alt key but after that none of the shortcuts that use the alt key are working.

I believe that that is intended behavior.

Just to clarify, are you disabling alt and then the normal shortcuts aren't working, or is disabling alt causing other remappings from a key to shortcuts that use alt to fail?

In the first option, you effectively aren't actually hitting the shortcut since alt is disabled.

The second option would be unintended behavior as for example disabling ctrl+a should not disable ctrl+shift+a. The key difference is one is disabling a key while the other is disabling a shortcut.

[h4wwk3ye]

Then what exactly is the difference between undefined and disabled? I still want the alt key shortcuts to work but disable the single press of alt key. Is that possible with keyboard manager?

[mykhailopylyp]

@h4wwk3ye
Undefined is used for keys that are not supported. For example Fn key. It can be obtained by pressing Fn key in 'Press a key on a selected keyboard' window. Disabled is used for disabling keys or shortcuts. Let us know how did you get undefined.

I am afraid it is not possible to disable the alt key but still use shortcuts with it. We apply key remaps first and then shortcut remaps.

Maybe the problem you are facing can be solved in another way. Feel free to share exact scenarios.
Let's say you want to disable Alt key but still use Shift+Alt+C. You can add Alt -> Disabled in Remap key window and
Shift+C -> Shift+Alt+C in shortcut window.