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

feat(windows): right modifer keys include in hotkey if configured #12217

Closed
wants to merge 7 commits into from

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Aug 19, 2024

Fixes:#11471

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Aug 19, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 19, 2024

User Test Results

Test specification and instructions

ERROR: user tests have not yet been defined

@rc-swag
Copy link
Contributor Author

rc-swag commented Aug 21, 2024

@mcdurdin
I got it wrong around the removal of two old flags not fully understanding that there was interaction with a Win32 API RegisterHotkey. I had removed the caching code and I had also handled including the right modifier based on the user flag. However, I did not realise that the RegiserHotkey interaction with the win32 api was still occurring. I have partly fixed it.
However, I need to check the way I should be going forward.
Is it to use the RegisterHotkey API call on right modifier inclusion still? Or is it to totally remove the use of the RegisterHotkey, and just use the keyman engine implementation and just setting testing for right modifier when the flag is set? I think it is the later.

@mcdurdin
Copy link
Member

@rc-swag as discussed in Standup, go ahead and remove all the code that is bracketed by the compatibility flags, including RegisterHotkey calls, and then add support for left-side modifiers to the remaining hotkey management system.

@rc-swag rc-swag closed this Aug 22, 2024
@mcdurdin mcdurdin deleted the feat/windows/11471/right-modifier-hotkey branch August 23, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants