Bug17281 - Retain momentary layers until the end of tapping #17282

fxkuehl · 2022-06-01T04:33:53Z

Description

"Retain momentary layers until the end of tapping" is my solution for Bug17281.

"Make process_tapping more readable" helped me understand the code better as I was investigating this problem. I'm including it for consideration. It's purely for readability and should have no impact on functionality. It is not needed to fix the bug.

Types of Changes

Issues Fixed or Closed by This PR

[Bug] Mod-tap keys on momentary layers don't work as expected #17281

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

drashna · 2022-06-02T15:48:36Z

If possible, it would be very helpful to add unit tests to ensure that the correct behavior is being triggered, and to ensure that it's not broken in the future.

fxkuehl · 2022-06-02T17:15:03Z

Sounds reasonable. I have no idea how to write or run unit tests for QMK. I'm still pretty new to it. I'll start reading the docs at https://docs.qmk.fm/#/unit_testing. Edit: At least it looks like I haven't broken any existing unit tests, so that's a good start. ;)

drashna · 2022-06-02T17:30:01Z

well, if you take a look at the secure tests and the caps word tests, that may give you a good idea, on how to start.

It's not too different from a keymap, but with some of the special stuff due to the testing code. You can use stuff like layer_state_is(x) still, etc.

If you need further help, the discord would be a good place to discuss it.

precondition · 2022-06-03T17:31:03Z

Since your change affects the tap-hold logic, you might be also interested in taking a look at the tap-hold unit tests I wrote for #15741, especially the test roll_layer_tap_key_with_regular_key. You should be able to simply copy one of those, swap a few lines and change some assertions.

precondition

Review of "Make process_tapping more readable"

quantum/action_tapping.c

fxkuehl · 2022-06-06T03:01:55Z

Since your change affects the tap-hold logic, you might be also interested in taking a look at the tap-hold unit tests I wrote for #15741, especially the test roll_layer_tap_key_with_regular_key. You should be able to simply copy one of those, swap a few lines and change some assertions.

Thanks. I was looking for a good place to add my test. It's not really specific to any of the tap_hold_configurations. I deliberately broke the retaining of modifiers during tapping, to see if it would break an existing test similar to what I was doing with layers. But no tests broke. So I ended up adding two tests to tests/basic/test_tapping.cpp. One for releasing Shift while tapping, one for releasing a layer key while tapping.

bschwehn · 2022-08-12T19:27:35Z

What is needed to get this merged? It solves an annoyance for me and it would be nice not having to maintain this as a private patch :)

quantum/action_tapping.c

Move most #ifdefs into conditionally defined macros to make the logic easier to follow. Signed-off-by: Felix Kuehling <[email protected]>

This allows mod-tap and layer-tap keys on layers to behave as expected. Bug: qmk#17281 Signed-off-by: Felix Kuehling <[email protected]>

Mods and layer key release is delayed while tapping is in progress to ensure that the tap is registered with the modifier state and on the layer where the key was first pressed. Signed-off-by: Felix Kuehling <[email protected]>

KarlK90 · 2022-09-15T07:39:16Z

@precondition reading through the discussions and the code changes it seems that most (if not all?) have come to a resolution, if so could you resolve them?

@fxkuehl reading through the code nothing stands out for me that would prevent a merge from my side. I'm going to try out the changes and add my approval. Thanks for the bugfix and cleaning up that mess that action_tapping.c is.

precondition · 2022-09-18T11:13:01Z

Uh, I don't see a "Resolve" button anywhere in my previous review. Isn't that something that's only shown to the author of the PR? I can make a new approving review though.

KarlK90 · 2022-09-18T19:08:33Z

Interesting, I thought that you should be able to resolve those 🤔 anyway, if you are happy with the current changes I can resolve the threads for you.

KarlK90

I've been testing this PR and haven't encountered any problems. The changes also LGTM! Thanks!

precondition · 2022-10-05T15:51:15Z

quantum/action_tapping.c

+#        else
+#            define TAP_GET_RETRO_TAPPING true
+#        endif
+#        define MAYBE_RETRO_SHIFTING(ev) (TAP_GET_RETRO_TAPPING && (RETRO_SHIFT + 0) != 0 && TIMER_DIFF_16((ev).time, tapping_key.event.time) < (RETRO_SHIFT + 0))


I don't fully understand why did you prefix the macro name with "maybe"?

There are more conditions that are checked before the actual retry-shifting code (e.g. tapping_key.tap.count == 0). The condition here is necessary but not sufficient for retro shifting.

github-actions · 2022-11-20T02:15:24Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

fxkuehl · 2022-11-20T15:43:03Z

What do I need to do to get this merged. I don't think it should just be closed for being stale.

KarlK90 · 2022-11-23T09:10:43Z

What do I need to do to get this merged. I don't think it should just be closed for being stale.

It won't be closed 👍, at the moment it is just missing one more review from a collaborator and then it can be merged.

KarlK90 · 2022-11-24T09:37:44Z

@fxkuehl now that all approvals are in we can merge your PR next week after the breaking changes merge from develop to master

* Make process_tapping more readable Move most #ifdefs into conditionally defined macros to make the logic easier to follow. * Retain momentary layers until the end of tapping This allows mod-tap and layer-tap keys on layers to behave as expected. Bug: qmk#17281 * Add tests for delayed mod/layer release while tapping Mods and layer key release is delayed while tapping is in progress to ensure that the tap is registered with the modifier state and on the layer where the key was first pressed. Signed-off-by: Felix Kuehling <[email protected]>

github-actions bot added the core label Jun 1, 2022

drashna requested a review from a team June 2, 2022 15:48

precondition reviewed Jun 3, 2022

View reviewed changes

fxkuehl force-pushed the bug17281 branch from aeceeed to 7824c5a Compare June 6, 2022 02:55

precondition reviewed Aug 12, 2022

View reviewed changes

quantum/action_tapping.c Outdated Show resolved Hide resolved

Make process_tapping more readable

d1d6401

Move most #ifdefs into conditionally defined macros to make the logic easier to follow. Signed-off-by: Felix Kuehling <[email protected]>

fxkuehl force-pushed the bug17281 branch from 7824c5a to 23438db Compare August 21, 2022 04:11

fxkuehl added 2 commits August 21, 2022 13:43

Retain momentary layers until the end of tapping

e47d337

This allows mod-tap and layer-tap keys on layers to behave as expected. Bug: qmk#17281 Signed-off-by: Felix Kuehling <[email protected]>

Add tests for delayed mod/layer release while tapping

be6b675

Mods and layer key release is delayed while tapping is in progress to ensure that the tap is registered with the modifier state and on the layer where the key was first pressed. Signed-off-by: Felix Kuehling <[email protected]>

fxkuehl force-pushed the bug17281 branch from 23438db to be6b675 Compare August 21, 2022 17:46

KarlK90 approved these changes Oct 5, 2022

View reviewed changes

KarlK90 requested a review from a team October 5, 2022 15:42

precondition reviewed Oct 5, 2022

View reviewed changes

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Nov 20, 2022

KarlK90 added breaking_change_2022q4 awaiting review labels Nov 20, 2022

tzarc added breaking_change_2023q1 and removed breaking_change_2022q4 labels Nov 20, 2022

github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Nov 21, 2022

drashna approved these changes Nov 23, 2022

View reviewed changes

drashna requested a review from a team November 23, 2022 18:05

KarlK90 merged commit 4ae7525 into qmk:develop Nov 28, 2022

KarlK90 mentioned this pull request Nov 28, 2022

[Bug] Mod-tap keys on momentary layers don't work as expected #17281

Closed

fxkuehl deleted the bug17281 branch February 20, 2023 22:41

fxkuehl restored the bug17281 branch February 20, 2023 22:41

IsaacElenbaas mentioned this pull request Mar 12, 2023

Improve test invocation, fix Retro Shift bugs, and add Auto+Retro Shift test cases #15889

Merged

14 tasks

sigprof mentioned this pull request Mar 27, 2023

[Bug] Multiple OSM modifiers don't work on a layer activated with layer-tap and HOLD_ON_OTHER_KEY_PRESS #20269

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug17281 - Retain momentary layers until the end of tapping #17282

Bug17281 - Retain momentary layers until the end of tapping #17282

fxkuehl commented Jun 1, 2022

drashna commented Jun 2, 2022

fxkuehl commented Jun 2, 2022 •

edited

Loading

drashna commented Jun 2, 2022

precondition commented Jun 3, 2022 •

edited

Loading

precondition left a comment

fxkuehl commented Jun 6, 2022

bschwehn commented Aug 12, 2022

KarlK90 commented Sep 15, 2022

precondition commented Sep 18, 2022

KarlK90 commented Sep 18, 2022

KarlK90 left a comment •

edited

Loading

precondition Oct 5, 2022

fxkuehl Oct 5, 2022

github-actions bot commented Nov 20, 2022

fxkuehl commented Nov 20, 2022

KarlK90 commented Nov 23, 2022

KarlK90 commented Nov 24, 2022

Bug17281 - Retain momentary layers until the end of tapping #17282

Bug17281 - Retain momentary layers until the end of tapping #17282

Conversation

fxkuehl commented Jun 1, 2022

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

drashna commented Jun 2, 2022

fxkuehl commented Jun 2, 2022 • edited Loading

drashna commented Jun 2, 2022

precondition commented Jun 3, 2022 • edited Loading

precondition left a comment

Choose a reason for hiding this comment

fxkuehl commented Jun 6, 2022

bschwehn commented Aug 12, 2022

KarlK90 commented Sep 15, 2022

precondition commented Sep 18, 2022

KarlK90 commented Sep 18, 2022

KarlK90 left a comment • edited Loading

Choose a reason for hiding this comment

precondition Oct 5, 2022

Choose a reason for hiding this comment

fxkuehl Oct 5, 2022

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2022

fxkuehl commented Nov 20, 2022

KarlK90 commented Nov 23, 2022

KarlK90 commented Nov 24, 2022

fxkuehl commented Jun 2, 2022 •

edited

Loading

precondition commented Jun 3, 2022 •

edited

Loading

KarlK90 left a comment •

edited

Loading