Add retro tapping support #55
Conversation
When a retro key is released during the state is UNSETTLED, Achordion doesn't do any plumbing back and as a result the retro key is not sent. - If retro tapping is enabled, the code checks whether the key is a mod tap or a layer tap key. - If per key retro tapping is enabled the code checks for the function that should have been defined in a user's keymap.c to check if retro tapping is enabled for the key. If the function not defined the weak overload returns false by default. If the key is determined to be a retro key, the same plumbing is applied as when the state is settled to be HOLDING, allowing the retro key to be sent correctly.
features/achordion.c
Outdated
| @@ -257,4 +264,10 @@ __attribute__((weak)) uint16_t achordion_streak_timeout(uint16_t tap_hold_keycod | |||
| } | |||
| #endif | |||
|
|
|||
| #ifdef RETRO_TAPPING_PER_KEY | |||
| __attribute__((weak)) bool get_retro_tapping(uint16_t keycode, keyrecord_t *record) { | |||
There was a problem hiding this comment.
Is this needed? Core QMK has a weak definition of get_retro_tapping() already:
https://github.com/qmk/qmk_firmware/blob/0c160e1fbafbf477c74e64fd8ab9a9121eb0f42a/quantum/action.c#L63-L67
Duplicate definitions of a function normally cause a linking error. I'm not sure how that goes for weak definitions.
Come to think of it, it's interesting that a weak definition exists at all. Arguably, a user who set RETRO_TAPPING_PER_KEY could be expected to make a (strong) definition of get_retro_tapping(). So the value in having a weak definition is only in the edge case of a mistaken configuration where that is forgotten.
The function get_retro_tapping() should be accessible in this file: achordion.h includes quantum.h, which in turn includes action_tapping.h where the function is declared.
There was a problem hiding this comment.
You are right! This is removed in the newer version of this PR, since we no longer need to check the retro tapping key status.
features/achordion.c
Outdated
| // The active tap-hold key is being released. | ||
| if (achordion_state == STATE_HOLDING) { | ||
| if (achordion_state == STATE_HOLDING || is_retro_key) { |
There was a problem hiding this comment.
This is clever! It's a simpler change than I first expected.
I think the idea with the change on this line is that:
- QMK core has determined the key as held, but Achordion is unsettled.
- From QMK core's point of view, the key is held and no other keys have been pressed.
So by simply plumbing the hold release event, the subsequent retro tap handling in this condition will tap the tapping keycode. Is that accurate?
There was a problem hiding this comment.
Yep this is accurate, and your explanation with the two bullet points is actually the core idea of the newer implementation. This was more specific for retro tapping only, the newer version supports all kinds of features where no other keypress happens between the press and release of the key.
|
@akaralar thank you so much for this contribution! I very much appreciate this support for retro tap, this rounds out Achordion nicely. The implementation looks good. Just a couple minor comments. |
|
@getreuer Thanks for the review! I have since changed the implementation to be more generic in my userspace. As far as I understood the problem was that when there isn't a second keypress (Retro tapping, retro shift, autoshift, custom macros for hold action all fall in this category) Achordion holds back the hold event for |
|
@getreuer I pushed a new commit, changed the PR description and added comments to the code to explain the changes, let me know what you think. Thanks for taking the time! |
|
current version works for me 👍 hitting a second key on the same hand while holding the first immediately resolves both as taps. |
|
@akaralar I agree with your points! I'll merge the PR in its current form. Thank you again for your contribution! |
|
@getreuer glad I could help! thank you again for making such a great library and making home row mods tolerable 😃 |
I tried to add support for features where a key is held and then released without pressing another key to fix #51
In this PR, we try to circumvent the Achordion timeout if no other keypress happens between the press and release of the tap-hold key. When a tap-hold is pressed and released without the user pressing any other key in between, Achordion normally waits until the timeout finishes to settle the key as held and to send the events back to QMK to be processed. With this PR, If no other keypress happens until the tap-hold key is released, we simulate a hold and then release event to make Achordion play nice with these features.