Skip to content
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

On-release option for positional-hold-taps #1423

Merged
merged 4 commits into from
Mar 12, 2023

Conversation

urob
Copy link
Contributor

@urob urob commented Aug 6, 2022

This fixes #1363 by adding a hold-trigger-on-release option to positional hold-taps.

When hold-trigger-on-release is set to true, it delays the evaluation of hold-trigger-key-position until the next key's release. This is in contrast to the current behavior, which evaluates hold-trigger-key-positions upon the next key press. The current behavior is not always ideal, for example for homerow mods, because it prevents combining multiple modifiers on the same hand.

The new hold-trigger-on-release configuration allows combining multiple modifiers when held, while still forcing a tap-decision when the next key is tapped.

Note: Due to the delay of the position-check until key release, the property has no effect when using the hold-preferred flavor (which triggers mods when another key is pressed). The property is most useful when combined with the balanced or tap-preferred flavor.

Example configuration:

// 36 keys layout
#define LEFT_KEYS 0 1 2 3 4 10 11 12 13 14 20 21 22 23 24  // left-hand keys
#define RIGHT_KEYS 5 6 7 8 9 15 16 17 18 19 25 26 27 28 29  // right-hand keys

/ {
    behaviors {
        hrm_l: left_homerow_mods {
            compatible = "zmk,behavior-hold-tap";
            label = "HOMEROW_MODS_LEFT";
            #binding-cells = <2>;
            flavor = "balanced";
            tapping-term-ms = <200>;
            hold-trigger-key-positions = <RIGHT_KEYS>;
            hold-trigger-on-release;
            bindings = <&kp>, <&kp>;
        };

        hrm_r: right_homerow_mods {
            compatible = "zmk,behavior-hold-tap";
            label = "HOMEROW_MODS_RIGHT";
            #binding-cells = <2>;
            flavor = "balanced";
            tapping-term-ms = <200>;
            hold-trigger-key-positions = <LEFT_KEYS>;
            hold-trigger-on-release;
            bindings = <&kp>, <&kp>;
        };
    };
};

@jigfox
Copy link

jigfox commented Aug 9, 2022

Tank you for trying to solve the problem with chording mods and using hold-trigger-key-positions together. But sadly it isn't working for me. If I roll over 21 &rhm LALT I and 20 &rhm LGUI E, so a very fast 21 down, 20 down, 21 up, 20 up. The keyboard fires alt-e, but I want to get the letters ie. Although e (key 20) is not part of hold-trigger-key-positions in &rhm

Same goes for rolling 20 and 19, there I get an E instead en.

/ {
  behaviors {
    // 0   1   2   3   4   5                    6   7   8   9   10  11
    // 12  13  14  15  16  17                   18  19  20  21  22  23
    // 24  25  26  27  28  29  30  31   32  33  34  35  36  37  38  39
    //             40  41  42  43  44   45  46  47  48  49
    rhm: right_home_row_mod {
      compatible = "zmk,behavior-hold-tap";
      label = "RIGH_POSITIONAL_HOLD_TAP";
      #binding-cells = <2>;
      tapping-term-ms = <200>;
      bindings = <&kp>, <&kp>;
      hold-trigger-key-positions = <0 1 2 3 4 5 12 13 14 15 16 17 24 25 26 27 28 29 30 31 40 41 42 43 44>;
      hold-trigger-on-release;
    };
    lhm: left_home_row_mod {
      compatible = "zmk,behavior-hold-tap";
      label = "LEFT_POSITIONAL_HOLD_TAP";
      #binding-cells = <2>;
      tapping-term-ms = <200>;
      bindings = <&kp>, <&kp>;
      hold-trigger-key-positions = <6 7 8 9 10 11 18 19 20 21 22 23 32 33 34 35 36 37 38 39 45 46 47 48 49>;
      hold-trigger-on-release;
    };
  };
  keymap {
    compatible = "zmk,keymap";

    default_layer {
      bindings = <
	&kp TAB        &kp Q          &kp W        &kp F          &kp P         &kp B                                                     &kp J     &kp L         &kp U        &kp Y        &kp SEMI      &kp BSPC
	&mt LCTRL ESC  &lhm  LCTRL A  &lhm LALT R  &lhm LGUI S    &lhm LSHFT T  &kp G                                                     &kp M     &rhm LSHFT N  &rhm LGUI E  &rhm LALT I  &rhm LCTRL O  &mt RCTRL SQT
	&kp LSHFT      &kp Z          &kp X        &kp C          &kp D         &kp V        &kp LBKT  &kp GRAVE   &caps_word  &kp RBKT   &kp K     &kp H         &kp COMMA    &kp DOT      &kp FSLH      &kp RSHFT
	                                           &kp LG(LSHFT)  &kp LCMD      &mt LALT RET &kp SPACE &mo NAV     &mo SYM     &kp SPACE  &kp RALT  &kp RGUI      &kp HYP
        >;
    };
  };
};

@urob
Copy link
Contributor Author

urob commented Aug 10, 2022

Tank you for trying to solve the problem with chording mods and using hold-trigger-key-positions together. But sadly it isn't working for me. If I roll over 21 &rhm LALT I and 20 &rhm LGUI E, so a very fast 21 down, 20 down, 21 up, 20 up. The keyboard fires alt-e, but I want to get the letters ie. Although e (key 20) is not part of hold-trigger-key-positions in &rhm

Thanks for reporting. This is to be expected due to the default flavor being "hold-preferred", which triggers when another key has been pressed (bypassing the key-position check which is delayed until key release). I suggest using the "balanced" or "tap-preferred" flavor along with this feature.

@jigfox
Copy link

jigfox commented Aug 10, 2022

Thank you for your response, and thanks for your efforts, with balanced-flavor it is working now

Copy link

@jigfox jigfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working with this branch for one week now, and it is working perfectly fine

caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Oct 15, 2022
@caksoylar
Copy link
Contributor

I have been using this branch for a few weeks now as well, no problems with it, I would love to see it merged. It fixes the major problem I had with positional hold-taps which is not being able to combine multiple holds on the same side.

@urob
Copy link
Contributor Author

urob commented Oct 17, 2022

I have been using this branch for a few weeks now as well, no problems with it, I would love to see it merged. It fixes the major problem I had with positional hold-taps which is not being able to combine multiple holds on the same side.

Thanks! I have been using it as well without issues for quite a while now. @petejohanson from my side it's ready to be reviewed

caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Nov 1, 2022
johnm added a commit to johnm/zmk that referenced this pull request Nov 6, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Nov 7, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Nov 27, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 5, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 11, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 17, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 17, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 27, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Jan 22, 2023
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Jan 22, 2023
@ifreund
Copy link

ifreund commented Feb 10, 2023

I'm using this commit in my zmk fork and this works great for the balanced flavor. The status quo hold-trigger-key-position behavior is unusable for me with the balanced flavor at least and I was very happy to discover this branch existed.

The approach taken by this commit is quite elegant and straightforward, it would be nice to see it merged someday.

replicaJunction added a commit to replicaJunction/zmk that referenced this pull request Feb 24, 2023
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Feb 24, 2023
lugoues pushed a commit to lugoues/zmk that referenced this pull request Mar 9, 2023
lugoues pushed a commit to lugoues/zmk that referenced this pull request Mar 9, 2023
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nitpick on the updated conditional. This also should have a new test added to assert the updated additional behavior option and it's functionality.

Comment on lines 591 to 592
if (((!undecided_hold_tap->config->hold_trigger_on_release && ev->state) // key pressed
|| (undecided_hold_tap->config->hold_trigger_on_release && !ev->state)) // key released
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to undecided_hold_tap->config->hold_trigger_on_release != ev->state>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, done

@urob
Copy link
Contributor Author

urob commented Mar 11, 2023

One minor nitpick on the updated conditional. This also should have a new test added to assert the updated additional behavior option and it's functionality.

Done. I have tests for all 3 main flavors but only committed the one for balanced so far since the flag is meaningless for the other ones. But if you prefer I can upload them as well.

@petejohanson
Copy link
Contributor

@urob can you please rebase against latest main? Missed the netlify deploy failures earlier. Then I think this is ready to merge.

@urob urob force-pushed the positional-hold-tap-on-release branch from 69abe2e to 9d4b775 Compare March 12, 2023 15:45
@urob
Copy link
Contributor Author

urob commented Mar 12, 2023

@urob can you please rebase against latest main? Missed the netlify deploy failures earlier. Then I think this is ready to merge.

Done. I noted those netlify deploy failures but didn't know where they came from and what to do about (they weren't there when I first pushed this branch). But looks like they are resolved after the rebase.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@petejohanson petejohanson merged commit 6cb42a8 into zmkfirmware:main Mar 12, 2023
jakkan pushed a commit to jakkan/zmk-config that referenced this pull request Mar 13, 2023
yuanbin pushed a commit to yuanbin/zmk that referenced this pull request Jun 6, 2023
yuanbin pushed a commit to yuanbin/zmk that referenced this pull request Jun 6, 2023
yuanbin pushed a commit to yuanbin/zmk that referenced this pull request Jun 6, 2023
yuanbin added a commit to yuanbin/zmk that referenced this pull request Jun 13, 2023
yuanbin added a commit to yuanbin/zmk that referenced this pull request Jun 14, 2023
yuanbin added a commit to yuanbin/zmk that referenced this pull request Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion/Feature request: Better handling of chorded mods with positional hold-tap feature
5 participants