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

KBM - Keyboard Manager Shortcut Bugs #35201

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

gokcekantarci
Copy link
Contributor

Summary of the Pull Request

With this PR, 2 bugs in KBM were fixed.

PR Checklist

Detailed Description of the Pull Request / Additional comments

KBM bugs are below:

  1. KBM shorcuts don't work when the PC is rebooted or Powertoys is restarted.
  2. When a shortcut and sending text with a common modifier key are invoked one after the other, KBM does not work.

Validation Steps Performed

Shortcut mappings for testing are as follows.

Note 1: Shortcut mappings are with AltGr modifier key. Also tested with only Ctrl Left with the Turkish Keyboard Layout or only with Alt Right with the US keyboard layout.
Note 2: Tested with Italian (Italy) keyboard layout.

Mappings

Bug 1 Test Steps:

  1. Set KBM shortcuts.
  2. Close PowerToys.
  3. Press AltGR and dont release.
  4. Open Powertoys.
  5. Press Action keys. Check shortcuts don't work.
  6. Release all keys and press again shortcuts. Check this time shortcuts work properly.

Bug 2 Test Steps:

  1. Set keyboard layout to Italian (Italy)
  2. Set KBM shortcuts as above in screenshot.
  3. Press AltGr + I and check X is pressed.
  4. Release only I and press " \ " or " ' " . Check related text is sent. ( ■ , ... )
  5. All shortcuts above have same AltGr modifier keys. Test with combination of those shortcuts working properly.

@gokcekantarci gokcekantarci marked this pull request as draft October 2, 2024 13:47
@gokcekantarci gokcekantarci marked this pull request as ready for review October 2, 2024 16:02
@crutkas
Copy link
Member

crutkas commented Oct 11, 2024

🔥🔥🔥

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Oct 21, 2024
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Couldn't repro the original issue for Bug 1 with the instructions in the PR text in 0.85.1.
I did try something that seems to have gotten worse, though.
With the AltGr+I to Shift(left)+X mapping, try the following:
1 - Use the mapping, don't release Alt Gr
2 - Disable KBM
3 - Enable KBM
4 - Try using the mapping again without having released Alt Gr.
5 - release Alt Gr
6 - try the mapping again

On .85.1, Shift(left) gets stuck. On this PR, both Shift(left) and Ctrl(left) get stuck.

@gokcekantarci
Copy link
Contributor Author

Couldn't repro the original issue for Bug 1 with the instructions in the PR text in 0.85.1. I did try something that seems to have gotten worse, though. With the AltGr+I to Shift(left)+X mapping, try the following: 1 - Use the mapping, don't release Alt Gr 2 - Disable KBM 3 - Enable KBM 4 - Try using the mapping again without having released Alt Gr. 5 - release Alt Gr 6 - try the mapping again

On .85.1, Shift(left) gets stuck. On this PR, both Shift(left) and Ctrl(left) get stuck.

Hi @jaimecbernardo,

I added Bug 1 improvements as a precaution for users who press modifier keys while KBM has not started yet. The case you tested is different from this one. When KBM is disabled before the key up event occurs, Shift left remains stuck because it is in the shortcut(Shift Left) in version 0.85.1. This was already what caused the bug in Bug 2. To solve this, I kept the pressed modifier keys (altgr) pressed instead of the invoked shortcut(shift left) modifier key.

Alt gr is a special key and I make special adaptations to it in KBM issues. There is the "isAltRightKeyInvoked" variable that I use for this. In the code, there are special cases where if this bool variable is true. When the code is restarted as in your test case, although Altgr is pressed, I cannot send the Left ctrl up event because this variable is false.

I tried the same steps with Ctrl(Left)+I to Shift(left)+X and the problem did not occur.

Finally, the purpose of this PR is different. Bug 1 changes was made to prevent errors that occur when modifier keys are pressed before starting KBM. Bug 2 changes was made for the errors that occur when trying to send a shortcut and send text with common modifier keys at the same time with not releasing that modifier keys. Disabling KBM while keys are pressed is a different issue. Only difference is keys pressed instead of the set shortcut become stuck in this PR. Also, if you disable KBM while holding down the modifierkey and action key in both 0.85.1 and this PR, you will see that the Shift Left and x keys are stuck.

The bug you mentioned may appear as follows. If KBM crashes while the user is invoking a shortcut using altgr and alt gr is pressed in that time. But I think the probability of this issue to be very low. My suggestion is to fix these two existing bugs now. Because both have been frequently reported. If such an issue reported from users, we will try to find a special solution for it like reset all pressed keys when KBM started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants