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

feat(behaviors): Adding require-prior-idle-ms for combos and hold-taps #1387

Merged
merged 8 commits into from
Oct 3, 2023

Conversation

andrewjrae
Copy link
Contributor

@andrewjrae andrewjrae commented Jul 17, 2022

Detaching the global-quick-tap functionality from quick-tap term, and implementing it for both combos and hold-taps under the new option require-prior-idle-ms.

This constitutes a breaking change for users keymaps in that global-quick-tap will no longer be an option. I'm not sure what the protocol is when doing such refactors, but I think it is beneficial in the long run for two reasons: This deprecates the global-quick-tap option, however it is backwards compatible in that if it is set it will copy the quick-tap-ms to the require-prior-idle-ms.

There are two reasons for these changes;

  1. This functionality can be added to combos under the new unified name require-prior-idle-ms.

  2. This allows users to set a lower term for the 'global-quick-tap' (now require-prior-idle-ms) functionality (typically ~100ms), and a higher term for the regular quick-tap (typically ~200ms). This means the regular quick-tap is easier to hit, while still allowing the benefits of disabling the hold-taps during fast typing.

PR Progress:

  • Refactor hold-taps to support require-prior-idle-ms rather than global-quick-tap
  • Add require-prior-idle-ms functionality to combos
  • Update docs

@andrewjrae
Copy link
Contributor Author

andrewjrae commented Jul 18, 2022

I've now pushed a change such that he old global-quick-tap option is still usable, and will simply copy the value of quick-tap-ms to min-prior-ms if set, as per @okke-formsma's suggestion on Discord. A better name for min-prior-ms has yet to be chosen however.

@andrewjrae andrewjrae changed the title feat(behaviors)!: Adding min-prior-ms for combos and hold-taps feat(behaviors): Adding min-prior-ms for combos and hold-taps Jul 18, 2022
@andrewjrae andrewjrae changed the title feat(behaviors): Adding min-prior-ms for combos and hold-taps feat(behaviors): Adding global-quick-tap-ms for combos and hold-taps Jul 18, 2022
urob added a commit to urob/zmk-config that referenced this pull request Jul 19, 2022
urob added a commit to urob/zmk-config that referenced this pull request Jul 19, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 21, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 31, 2022
Remove global-quick-tap from shift
urob added a commit to urob/zmk-config that referenced this pull request Aug 5, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Aug 8, 2022
Remove global-quick-tap from shift
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Aug 9, 2022
Remove global-quick-tap from shift
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Sep 1, 2022
Remove global-quick-tap from shift
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Sep 1, 2022
Remove global-quick-tap from shift
@caksoylar
Copy link
Contributor

Wanted to drop a note saying this has been working perfectly for me for both combos and hold-taps for the last couple of months of testing. If the idea makes sense to contributors, maybe we can add docs?

@andrewjrae
Copy link
Contributor Author

Yah I'm sorry I've let this go stale, I moved and started a new job hehe. If someone wants to write docs they can feel free to PR my branch, otherwise I should be able to get to it next week.

@okke-formsma
Copy link
Collaborator

code lgtm, some docs are needed before we can merge. I left a minor comment on a comment :)

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

urob commented Oct 17, 2022

Wanted to drop a note saying this has been working perfectly for me for both combos and hold-taps for the last couple of months of testing. If the idea makes sense to contributors, maybe we can add docs?

Same here, would be great to see this merged.

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
pipex added a commit to pipex/zmk-corne that referenced this pull request Sep 5, 2023
gibking pushed a commit to gibking/zmk that referenced this pull request Sep 13, 2023
gibking pushed a commit to gibking/zmk that referenced this pull request Sep 20, 2023
@dmchmk
Copy link

dmchmk commented Sep 20, 2023

hi guys, what is the current status of this merge? Looks like no one here's against it, can we expect it to be approved any time soon?

@andrewjrae
Copy link
Contributor Author

This was on pause for a bit while we decided on better naming. Many ideas were thrown around on discord but I'm leaning towards require-prior-idle-ms for both combos and hold taps. I just need to get around to actually renaming and then a final review from @petejohanson and or @caksoylar.

@infused-kim
Copy link
Contributor

I have been using this for a few months and it’s astonishing how well it works.

It makes homerow mods actually usable.

And I think this will give users a huge reason to pick zmk over qmk.

I believe zmk state of the firmware blog post is “due” soon. And it would probably be a great opportunity to advertise this feature and “Urob-style homerow mods” in it.

So, perhaps it would be a good idea to merge this before the state of the firmware post.

@andrewjrae andrewjrae requested a review from a team as a code owner September 24, 2023 13:45
@andrewjrae andrewjrae changed the title feat(behaviors): Adding global-quick-tap-ms for combos and hold-taps feat(behaviors): Adding require-prior-idle-ms for combos and hold-taps Sep 24, 2023
@andrewjrae
Copy link
Contributor Author

Okay, renaming done, should be ready for a final review @petejohanson @caksoylar.

andrewjrae and others added 6 commits September 24, 2023 10:11
Detaching the global-quick-tap functionality from the quick-tap term.
This makes way for two improvements:

1. This functionality can be added to combos under a unified name
   'global-quick-tap-ms'.

2. This allows users to set a lower term for the 'global-quick-tap'
   (typically ~100ms), and a higher term for the regular
   quick-tap (typically ~200ms)

This deprecates the global-quick-tap option, however if it is set, the
quick-tap-ms value will be copied to global-quick-tap-ms.
This brings the 'global-quick-tap' functionality to combos by filtering
out candidate combos that fell within their own quick tap term.

I also replaced `return 0` with `return ZMK_EV_EVENT_BUBBLE` where appropriate.
(I assume this was done in past as it is similar to errno returning, but
being that this is to signify an event type I find this more clear)
Renaming global-quick-tap-ms to require-prior-idle.
Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

Thanks for the update! A few nitpicks on docs, but otherwise LGTM docs-wise.

(Not doing a "Github approval" since that should come from a non-docs maintainer.)

docs/docs/behaviors/hold-tap.md Outdated Show resolved Hide resolved
docs/docs/behaviors/hold-tap.md Outdated Show resolved Hide resolved
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 as well, but otherwise LGTM.

app/dts/bindings/behaviors/zmk,behavior-hold-tap.yaml Outdated Show resolved Hide resolved
@andrewjrae
Copy link
Contributor Author

Okay, I realized my old sweep for global quick tap must have fallen short so I did a better search and caught a few more outliers. Please review the latest commit @petejohanson, I also ended up changing cradio.keymap since it was using the now deprecated global-quick-tap.

gibking pushed a commit to gibking/zmk that referenced this pull request Sep 29, 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.

Thanks for the work on this and your patience. LGTM!

@petejohanson petejohanson merged commit 11996ff into zmkfirmware:main Oct 3, 2023
51 checks passed
@andrewjrae andrewjrae deleted the min-prior-ms branch October 3, 2023 20:09
bark2 pushed a commit to bark2/Sofle_bluemicro840_ZMK that referenced this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants