Skip to content
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

Fix remapping to Alt+Tab/Win+Space style shortcuts #3965

Merged

Conversation

arjunbalgovind
Copy link
Contributor

@arjunbalgovind arjunbalgovind commented Jun 1, 2020

Summary of the Pull Request

This PR adds handling for shortcuts which behave like Alt+Tab and Win+Space. These shortcuts behave differently from others since once you invoke Alt+Tab and release Tab, the window switcher remains open as long as Alt is pressed, so if a user remaps Ctrl+C to Alt+Tab we need to make sure as long as Ctrl is held and a user does not press a key apart from C, then Alt should remain as being held to the OS.

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Added a check at the start of the shortcut remap function to check if any Shortcut is in the "invoked" state. If it is, we directly skip control to that shortcut remap. This is required for the new change, since we leave the shortcut in invoked state even if all the keys are not held down.
  • Changed the case where modifiers of original shortcut are held down and action key is released - Now we only send key up for the new shortcut's action key (and we do not change the modifiers).
  • If a modifier key is pressed down which is already pressed down in the original shortcut, then suppress that key message. (can happen if user presses LCtrl and RCtrl for a Ctrl based shortcut, or if a user has 2 copies of the same modifier key (by remapping)).
  • If any key other than original shortcut keys is pressed, revert the state to the original shortcut followed by the new key pressed. The original shortcut's action key needs to be sent only if it was actually held down, which can be checked by checking async key state of the new shortcut's action key.
  • Added safety code to reset the shortcut invoked flag if none of the cases match. This code shouldn't execute in any real world situation but if it's added to avoid any unexpected behavior to make sure KBM doesn't get stuck on any one shortcut.

Known Issues

  • If a user remaps Ctrl+Alt+A to Alt+Tab (i.e. a shortcut containing old and some other modifier to Alt+Tab), then the window switcher stays up until alt is released (and not Ctrl). This is because in our logic when switching back to the original shortcut we do not resend Alt Up, Alt Down because that causes weird behavior with Alt triggering menu bars. Similar issue exists for Win+Shift+S to Win+Space. A workaround would be to hardcode the behavior for these 2 shortcuts (Alt Tab and Win Space), however there might be more shortcuts which follow this style of shortcut remains held till modifier is released.

Validation Steps Performed

Verified that remapping to Alt+Tab and Win+Space works as expected, and verified that other remaps still work as before.

@arjunbalgovind arjunbalgovind added the Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager label Jun 1, 2020
@arjunbalgovind arjunbalgovind requested review from a team and traies June 1, 2020 16:32
@arjunbalgovind arjunbalgovind removed the request for review from traies June 4, 2020 21:41
Copy link
Contributor

@alekhyareddy28 alekhyareddy28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally when we use alt+tab (alt key held down) and we hold down the other alt key as well, on pressing tab, it continues to tab through items till either of the alt keys are released. However, if I remap Ctrl+C -> Alt+Tab, then when ctrl is pressed down and now alt is pressed, the expected behavior would be that till either Ctrl or Alt is released we would be able to tab, however the moment we press Alt, it opens that app.

src/modules/keyboardmanager/dll/KeyboardEventHandlers.cpp Outdated Show resolved Hide resolved
@arjunbalgovind
Copy link
Contributor Author

Normally when we use alt+tab (alt key held down) and we hold down the other alt key as well, on pressing tab, it continues to tab through items till either of the alt keys are released. However, if I remap Ctrl+C -> Alt+Tab, then when ctrl is pressed down and now alt is pressed, the expected behavior would be that till either Ctrl or Alt is released we would be able to tab, however the moment we press Alt, it opens that app.

This is by design, since pressing any key apart from those in the original shortcut (Ctrl+C) so should revert it back to the unremapped state. For an analogous case, if you remap Ctrl(L)+C to Alt+Tab, then while you have already opened it, pressing Ctrl(R) will close it, but if you remap Ctrl+C to Alt+Tab, then pressing the other Ctrl key will show the expected behavior for Alt as you mentioned.

The main change with this PR, which was mainly observed for Alt+tab and Win+Space (there might be others too which is why it wasnt hardcoded), is that we are sort of mapping the modifiers as well, so in the example you mentioned it should technically be Ctrl behaving like that, and that works as expected if you remap Ctrl(not L/R version)+C -> Alt+Tab

Copy link
Contributor

@alekhyareddy28 alekhyareddy28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. It makes sense to not have that behavior as you mentioned because we aren't remapping shortcuts which are a part of another shortcut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants