-
-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Unregister previous keycode after dynamic keymap set #24517
base: develop
Are you sure you want to change the base?
Unregister previous keycode after dynamic keymap set #24517
Conversation
lint formatting Co-authored-by: Drashna Jaelre <[email protected]>
quantum/dynamic_keymap.c
Outdated
// Big endian, so we can read/write EEPROM directly from host if we want | ||
eeprom_update_byte(address, (uint8_t)(keycode >> 8)); | ||
eeprom_update_byte(address + 1, (uint8_t)(keycode & 0xFF)); | ||
unregister_code(prev_keycode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on discord, this wont correctly handle keycode that are non-basic, and changing it to unregister_code16
would then only handle basic+modded. Using clear_keyboard()
covers more scenarios but still does not cover any side effects of quantum keycodes.
Im also not a fan of coupling storage directly to its eventual use, wherever in the tmk action code. I agree updating via.c
to be responsible would be error prone for other eventual uses of dynamic_keymap_set_keycode
, but it still feels like the cleaner workaround till a better solution is implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reason I handled things in here was to cover the scenario of:
- key pressed
- dynamic keymap set
- dynamic keymap set (again)
- key released
Is changing unregister_code
to handle non-basic keycodes an option?
Or creating a new, all-encompassing function to handle unregistering any code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when you suggest to have via.c
be responsible, would having the same logic but instead immediately before and after the call to dynamic_keymap_set_keycode
within the id_dynamic_keymap_set_keycode
case suffice? Or would there be more to having this happen in via.c
?
Description
Changes behavior of dynamic_keymap_set_keycode to unregister the keycode previously set before the keymap update.
I would not exactly call this a "bugfix" but it does prevent keys from getting "stuck" when editing layouts through VIA hotswapping, for example.
This works but feels hacky since
unregister_code
is called regardless of if that key was in a pressed state or notTypes of Changes
Issues Fixed or Closed by This PR
Checklist