Fix layer switching from tap dances by redoing the keymap lookup #17935
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The set of existing tap dance tests did not cover predefined actions for
layer switching (`ACTION_TAP_DANCE_LAYER_MOVE(kc, layer)`,
`ACTION_TAP_DANCE_LAYER_TOGGLE(kc, layer)` - these actions perform layer
switching on double tap), and also did not cover the case when a tap
dance is used to implement an extended version of the `LT()` keycode
(performing layer switching on hold). Add some tests for these kinds of
tap dances, which actually uncover a bug in the tap dance implementation
(if the tap dance finishes due to being interrupted by pressing some
other key before the tapping term had expired, the layer switch may not
affect that other key properly).
Tests which are known to fail due to the mentioned bug:
Layers/TapDanceLayers.SingleHoldFastLayer/CustomLTCustomFast
Layers/TapDanceLayers.SingleHoldFastLayer/CustomLTCustomSlow
Layers/TapDanceLayers.DoubleTapFastLayer/LayerToggleCustomFast
Layers/TapDanceLayers.DoubleTapFastLayer/LayerToggleCustomSlow
Layers/TapDanceLayers.DoubleHoldFastLayer/LayerToggleCustomFast
Layers/TapDanceLayers.DoubleHoldFastLayer/LayerToggleCustomSlow
All those failures manifest only when using custom keycodes, but only
because there is another bug (the source layers cache gets updated
inside `process_record_handler()`, overwriting its previous state) which
covers up the main bug for builtin keycodes.
Pressing a different key after starting a tap dance sequence interrupts the tap dance and invokes the corresponding `on_dance_finished()` function; that function may perform various operations, including changing the layer state. However, at this point in time the keymap lookup in `process_record_quantum()` had already been performed, therefore the layer state changes done by the tap dance function did not actually affect the key pressed immediately after the tap dance. Although in some cases it was possible to work around this issue by using the `on_each_tap()` function of the tap dance, this workaround is cumbersome and in some cases even impossible to implement (e.g., if you want to activate different layers on tap and hold). Moreover, because of another bug in `process_record_handler()` layer switching from tap dances actually seemed to work properly, but only for keycodes handled inside `process_action()` (the lookup inside `process_record_handler()` was overwriting the source layer cache data that was populated by the lookup in `process_record_quantum()` - this actually results in a mismatch of keycodes for press and release events in some cases). Simply fixing the erroneous lookup in `process_record_handler()` would break layer switching from tap dances for existing users who relied on that buggy behavior, therefore the proper fix for layer switching from tap dances needs to be added first. Looks like the simplest way to fix the problem is to add a `bool` return value to `preprocess_tap_dance()` and then repeat the keymap lookup in `process_record_quantum()` if `preprocess_tap_dance()` indicates that some tap dance has been finished. At this point in the code some functions may had used the keycode that ended up incorrect (including `preprocess_tap_dance()` itself, where that keycode may end up in `state->interrupting_keycode` for the current tap dance), but in some cases this does not really matter (e.g., `preprocess_secure()` does not use the keycode at all), and in other cases this may be fixed by reordering the handlers (e.g., `update_wpm()` may be moved after `preprocess_tap_dance()`).
drashna
approved these changes
Aug 6, 2022
tzarc
approved these changes
Oct 3, 2022
|
can combo also switch layers now ? |
|
@thompson-vii Switching layers from combos should be working for quite some time (since #8591) — just make some combos which output the layer switching keycodes (the changes from #8591 make all Quantum keycodes usable as combo outputs). If you have some problem with that, please report it as a separate issue. |
ramonimbao
pushed a commit
to ramonimbao/qmk_firmware
that referenced
this pull request
Nov 28, 2022
Is there a reason this was not done? |
Mostly because I don't use the WPM feature and don't have any hardware with a display on which I can actually test it. You can make a simple PR for that if you care about getting some inaccurate WPM values in the problematic cases. |
14 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Switching layers from tap dances did not work properly if the tap dance was finished by interrupting it (pressing a different key before the tapping term had expired — this includes both taps and holds): the layer switch was not affecting the interrupting key, but seemingly only in some cases. It turned out that there is another bug which almost makes the layer switching from tap dances work, but only for builtin keycodes handled inside
process_action()—process_record_handler()performs a keymap lookup for a keypress event in a way which overwrites the source layers cache, therefore builtin keycodes which are handled insideprocess_action()are looked up after the tap dance code had updated the layer state. However, if you also have some custom keycodes on the affected layers (or even some keycodes which are handled inprocess_record_quantum()), if the corresponding key interrupts the tap dance sequence, the keycode will not be looked up on the correct layer activated by the tap dance.Just fixing the bug in
process_record_handler()would effectively break layer switching from tap dances for some existing users, therefore some fix for the tap dance code is needed first. Looks like the simplest way is to redo the keymap lookup ifpreprocess_tap_dance()indicates that some tap dance has been finished — then the rest of the code called byprocess_record_quantum()will use the correct keycode.One potential problem is that the code before the call to
preprocess_tap_dance()would use potentially incorrect keycodes. However, some functions that are called that early (e.g.,preprocess_secure()do not use the keycode at all, and forpreprocess_tap_dance()itself (which would write the keycode intostate->interrupting_keycode) the problem is probably just unfixable. The only potentially problematic function there isupdate_wpm(); we may choose to move the call to that function, so that it would be called afterpreprocess_tap_dance().Types of Changes
Issues Fixed or Closed by This PR
Checklist