feat(behaviors): sticky keys no longer sticky after being held #1788

nguyendown · 2023-05-01T09:19:34Z

Awesome suggestion from RexxStone.
https://discord.com/channels/719497620560543766/1112676462089207828/1112676462089207828

A tap will stick.

      press            release
        ^                 ^
========|-----------------|===========================
          no-sticky-after-hold-ms
        <------------------------->

                            release-after-ms
                          <------------------>
========|-----------------|------------------|========
        v                                    v
     activate                            deactivate

A hold won't stick.

      press                        release
        ^                             ^
========|-----------------------------|===============
          no-sticky-after-hold-ms
        <------------------------->
========|-----------------------------|===============
        v                             v
     activate                     deactivate

Sticky keys will act like normal keys if they are held for longer than a configurable amount of time. I am deciding on a name for this property. Please help me.

no-sticky-after-hold-ms
~~max-hold-ms~~
~~deactivate-from-hold-ms~~

Currently, the property is optional and won't break existing configs. The default value is set to be the same as release-after-ms.

petejohanson · 2023-05-20T08:14:19Z

app/src/behaviors/behavior_sticky_key.c

    // adjust timer in case this behavior was queued by a hold-tap
-    int32_t ms_left = sticky_key->release_at - k_uptime_get();


I don't see how this is accounting for the comment above about adjusting for being held.. The timestamp in the event might be "older" than the actual time if the release was captured then re-released.

Did you test this in combination of hold taps?

I included this change by mistake. I wanted to test if it would break.

Did you test this in combination of hold taps?

I planned to, but I haven't had the chance to test it yet.

theol0403 · 2023-05-29T17:53:28Z

app/src/behaviors/behavior_sticky_key.c

@@ -210,14 +212,6 @@ static int sticky_key_keycode_state_changed_listener(const zmk_event_t *eh) {
            continue;
        }

-        // If this event was queued, the timer may be triggered late or not at all.


What was your reasoning for deleting this? I believe it stands as a failsafe for an edge case for a laggy timer, but I'm not 100% sure exactly when it is being used.

release_at is calculated upon key press. This failsafe deactivates sticky keys while they are being held. The unit test 13-active-until-release covers this.

I have been using this for a month now without noticing any issues. Perhaps my keymap isn't complicated enough to cause any problems?

There is certainly a way to keep the failsafe but I haven't thought much about it. Personally, I prefer not to rely on failsafes and believe that unexpected behaviors should be fixed and verified through testing. (Maybe I'm just being lazy.) Let me know what you think.

For sure... I don't know enough to say for sure why the failsafe is there, but I think it would be wise to really understand why it was needed in the first place and whether it is safe to remove.

theol0403

This implementation looks good to me, and considers everything discussed in #1636

theol0403 · 2023-05-29T17:59:16Z

app/src/behaviors/behavior_sticky_key.c

    // adjust timer in case this behavior was queued by a hold-tap
    int32_t ms_left = sticky_key->release_at - k_uptime_get();
-    if (ms_left > 0) {
-        k_work_schedule(&sticky_key->release_timer, K_MSEC(ms_left));
+    if (ms_left < 0) {


Small cleanup

Suggested change

if (ms_left < 0) {

k_work_schedule(&sticky_key->release_timer, K_MSEC(ms_left > 0 ? ms_left : 0));

Compilers probably optimize this already. Even if they do not, it's not worth sacrificing readability here, considering that the code is not run often.

theol0403 · 2023-06-30T21:05:11Z

Personally, I preferred the PR without the config option. I think this change is a sane default and I can't imagine any usecase for the timeout after release. I think the config option adds redundant complexity.

Edit: I read the usecase about using it instead of a tap hold for "don't hold sticky if held". While I think this is a good feature, I think that the tap-hold approach is superior, as a) it composes existing features without introducing new ones b) it handles edge cases including that happens if another key is pressed during held, hold-while-undecided, etc.

nguyendown · 2023-07-02T04:29:04Z

usecase for the timeout after release

My focus is on no-sticky-after-hold. Whether the release (deactivate) timer starts on binding press or binding release, it is a separate issue. The reason I changed it in d1a0027 is that I could make use of release_at to solve two issues at once.

tap-hold approach

Sticky key needs a standalone feature and does not rely on any workarounds or other behaviors. Besides, keymaps could be much more simple.

&sk {
    no-sticky-after-hold-ms = <200>;
};

And I don't have to read through the Hold-Tap documentation or understand ten different hold-tap flavors. (I did read it but am still confused btw.)

kvietcong · 2023-11-20T02:56:11Z

Are there any blockers in getting this merged? I love this feature but it seems to be quite behind master at this point.

Sticky keys that are pressed down for longer than `no-sticky-after-hold-ms` will immediately deactivate upon release. If the property `no-sticky-after-hold-ms` does not exist, the value of `release-after-ms` will be used as a fallback.

kvietcong · 2024-09-05T06:24:30Z

Sorry for another ping, but is this feature still valid to push forward?

nguyendown mentioned this pull request May 1, 2023

bugfix: sticky keys held for longer than release-after-ms get stuck #1636

Open

11 tasks

petejohanson reviewed May 20, 2023

View reviewed changes

theol0403 reviewed May 29, 2023

View reviewed changes

theol0403 approved these changes May 29, 2023

View reviewed changes

caksoylar added the behaviors label Jun 10, 2023

nguyendown changed the title ~~fix(behaviors): deactivate sticky keys immediately after being held~~ feat(behaviors): sticky keys no longer sticky after being held Jun 30, 2023

nguyendown force-pushed the behaviors/sticky-key-deactivate-after-hold branch from b8f52da to 95185e3 Compare November 22, 2023 08:19

nguyendown marked this pull request as ready for review November 22, 2023 08:23

nguyendown requested a review from a team as a code owner November 22, 2023 08:23

nguyendown marked this pull request as draft August 4, 2024 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(behaviors): sticky keys no longer sticky after being held #1788

feat(behaviors): sticky keys no longer sticky after being held #1788

nguyendown commented May 1, 2023 •

edited

Loading

petejohanson May 20, 2023

nguyendown May 24, 2023

theol0403 May 29, 2023

nguyendown Jun 2, 2023

theol0403 Jun 15, 2023

theol0403 left a comment

theol0403 May 29, 2023

nguyendown Jun 2, 2023

theol0403 commented Jun 30, 2023 •

edited

Loading

nguyendown commented Jul 2, 2023

kvietcong commented Nov 20, 2023

kvietcong commented Sep 5, 2024

		// adjust timer in case this behavior was queued by a hold-tap
		int32_t ms_left = sticky_key->release_at - k_uptime_get();

	if (ms_left < 0) {
	k_work_schedule(&sticky_key->release_timer, K_MSEC(ms_left > 0 ? ms_left : 0));

feat(behaviors): sticky keys no longer sticky after being held #1788

Are you sure you want to change the base?

feat(behaviors): sticky keys no longer sticky after being held #1788

Conversation

nguyendown commented May 1, 2023 • edited Loading

petejohanson May 20, 2023

Choose a reason for hiding this comment

nguyendown May 24, 2023

Choose a reason for hiding this comment

theol0403 May 29, 2023

Choose a reason for hiding this comment

nguyendown Jun 2, 2023

Choose a reason for hiding this comment

theol0403 Jun 15, 2023

Choose a reason for hiding this comment

theol0403 left a comment

Choose a reason for hiding this comment

theol0403 May 29, 2023

Choose a reason for hiding this comment

nguyendown Jun 2, 2023

Choose a reason for hiding this comment

theol0403 commented Jun 30, 2023 • edited Loading

nguyendown commented Jul 2, 2023

kvietcong commented Nov 20, 2023

kvietcong commented Sep 5, 2024

nguyendown commented May 1, 2023 •

edited

Loading

theol0403 commented Jun 30, 2023 •

edited

Loading