-
-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
[Core] Add Chordal Hold, an "opposite hands rule" tap-hold option similar to Achordion, Bilateral Combinations. #24560
Open
getreuer
wants to merge
40
commits into
qmk:develop
Choose a base branch
from
getreuer:core/chordal_hold
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
fa857db
Chordal Hold: restrict what chords settle as hold
getreuer e0f648d
Chordal Hold: docs and further improvements
getreuer 352f4fa
Fix formatting.
getreuer ed53b7d
Doc rewording and minor edit.
getreuer 3fdbb07
Support Chordal Hold of multiple tap-hold keys.
getreuer 99d49ac
Fix formatting.
getreuer da8ccf0
Simplification and additional test.
getreuer 1606d67
Fix formatting.
getreuer bd7e54a
Tighten tests.
getreuer 6b55824
Add test two_mod_taps_same_hand_hold_til_timeout.
getreuer e924a0c
Revise handing of pairs of tap-hold keys.
getreuer fb6c2d8
Generate a default chordal_hold_layout.
getreuer 8f86425
Merge branch 'develop' into chordal_hold
getreuer 5b5ff41
Document chordal_hold_handedness().
getreuer 60e8288
Add license notice to new and branched files in PR.
getreuer 4e46c16
Merge branch 'develop' into chordal_hold
getreuer a525048
Add `tapping.chordal_hold` property for info.json.
getreuer 4f3f5b3
Update docs/reference_info_json.md
getreuer 234fb97
Merge branch 'develop' into chordal_hold
getreuer 2014205
Revise "hand" jsonschema.
getreuer 355f9f9
Chordal Hold: Improved layout handedness heuristic.
getreuer 788f4aa
Use Optional instead of `| None`.
getreuer da32973
Refactor to avoid lambdas.
getreuer ae9fa23
Merge branch 'develop' into chordal_hold
getreuer c4d9180
Remove trailing comma in chordal_hold_layout.
getreuer 503752a
Minor docs edits.
getreuer 5af4ae7
Merge branch 'develop' into chordal_hold
getreuer 2b1eff2
Revise to allow combining multiple same-hand mods.
getreuer bb7a3c3
Merge branch 'develop' into chordal_hold
getreuer 4c5f603
Fix formatting.
getreuer a2bbbfa
Merge branch 'develop' into chordal_hold
getreuer c0cf45c
Add a couple tests with LT keys.
getreuer 36fdeb3
Remove stale use of CHORDAL_HOLD_LAYOUT.
getreuer 2aacc99
Fix misspelling lastest -> latest
getreuer 11cc997
Merge branch 'develop' into chordal_hold
getreuer e6ecff4
Merge branch 'develop' into chordal_hold
getreuer ae88194
Handling tweak for LTs and tests.
getreuer d23fd44
Fix formatting.
getreuer f649bc4
More tests with LT keys.
getreuer 3cefbaf
Fix formatting.
getreuer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
21 changes: 21 additions & 0 deletions
21
tests/tap_hold_configurations/chordal_hold_and_hold_on_other_key_press/config.h
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* Copyright 2022 Vladislav Kucheriavykh | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License as published by | ||
* the Free Software Foundation, either version 2 of the License, or | ||
* (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
#pragma once | ||
|
||
#include "test_common.h" | ||
#define CHORDAL_HOLD | ||
#define HOLD_ON_OTHER_KEY_PRESS |
18 changes: 18 additions & 0 deletions
18
tests/tap_hold_configurations/chordal_hold_and_hold_on_other_key_press/test.mk
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Copyright 2022 Vladislav Kucheriavykh | ||
# | ||
# This program is free software: you can redistribute it and/or modify | ||
# it under the terms of the GNU General Public License as published by | ||
# the Free Software Foundation, either version 2 of the License, or | ||
# (at your option) any later version. | ||
# | ||
# This program is distributed in the hope that it will be useful, | ||
# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
# GNU General Public License for more details. | ||
# | ||
# You should have received a copy of the GNU General Public License | ||
# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
# -------------------------------------------------------------------------------- | ||
# Keep this file, even if it is empty, as a marker that this folder contains tests | ||
# -------------------------------------------------------------------------------- |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a limitation of the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Great comment. I was originally thinking multiple simultaneous tap-hold keys is out of scope of what Chordal Hold needs to consider, since in an input sequence like "
A
↓,B
↓" whereA
andB
are tap-hold keys,A
andB
can't be settled yet.But rethinking it, these keys will eventually settle, of course, depending on subsequent events. I've revised so that Chordal Hold considers input sequences involving multiple tap-hold keys and I'm happy how this is working out.
Details:
Consider an input sequence "
A
↓,B
↓,C
↓, ..." of all presses of tap-hold keys. If held until the tapping-term, then regardless of handedness, these keys should be settled as held as usual. It is important to preserve this behavior for home row mods so that it is possible to chord multiple mods, e.g. Ctrl + Shift, in one hand.Another case: consider if within the tapping term a key is released: "
A
↓,B
↓,C
↓,C
↑". Provided either Permissive Hold or Hold on Other Key Press is enabled, the tap-hold keysA
,B
precedingC
would then be settled as held. With Chordal Hold, perhaps depending on handedness some or all of the keys should be tapped instead.I've implemented a general heuristic (
waiting_buffer_find_chordal_hold_tap()
in the code) to decide this, based on evaluatingget_chordal_hold()
between successive pairs of keys. Some specific practical cases described for 3-key sequences, though the rule works for any number of keys:If
A
,B
,C
are all on the same hand (get_chordal_hold()
evaluates false betweenAB
andBC
), they are all considered tapped. This is effectively "typing streak" suppression of the hold function. This is cool!If
A
andB
are on one hand andC
on the other, then bothA
andB
are held.If
A
is on one hand andB
andC
on the other, then justA
is held.Suppose an input sequence of tap-hold presses followed by a press of a regular key
Z
, like "A
↓,B
↓,C
↓, ...,Z
↓". Then, all the same logic as in case 2 applies.The implementation is more invasive than where this PR started, which makes me nervous. The "state machine" is not easy to reason about or modify, though I'm starting to get a feel for it.
I think you understand this area of the code a lot better than I do. Please let me know whether what I describe here needs further explanation, or seems like a flawed design and/or could be implemented better. I've made an effort to make it clean, been testing Chordal Hold in daily use (typing with it as we speak), and significantly beefed up the unit tests. But surely there's room for improvement. Anyway, I'm excited how this is progressing! 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading and changing the
action_tapping.c
file after a long hiatus is non-trivial (to say the least) and I'm trying to get up to speed again to evaluate this PR.My user space implementation is similar to ZMK's "positional hold-tap" when the following are pressed sequentially within tapping term on the right side of a QWERTY layout:
The host received the following:
But Chordal implementation does not override the first key and will transmit both modifiers:
Will review the changes in more detail to figure this out.
I haven't considered the scenario of more than 2 simultaneous key presses, and it seems that specific use cases will have nuances. This PR might benefit from more end user tests to ensure robustness because support of this feature will land on the Discord server when merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comparison! Yes, that is intended that both mods be held. I just added a unit test to verify this case. This behavior is useful so that one hand can hold Alt + GUI, or some other set of mods, while the other hand types a hotkey or uses the mouse.
OTOH, I can imagine there are other ways to handle those use cases. When using behavior as in your first output, what is a good solution for sending hotkeys like Ctrl+Shift+V? Do you find such multi-mod hotkeys are rare enough in practice that it's not a big deal? or maybe switch over to a Callum-style mod scheme for such things, or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your solution includes the following user function:
In the event of "Ctrl+Shift+V" or any other (frequent) same hand modifier combination, users can use that function to enable specific same hand modifier activation. Layouts such as Colemak that places a lot of frequently used letters on home row and may benefit from a default same-hand tap as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it some try on typing training via https://www.keybr.com/ - rolls seem really fine and no strange delays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful! That's great to hear. Thank you for the testing =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this diff today. This actually is much smoother that Achordion mode which was adding annoying delays on the home row mod. Cannot wait for it to land upstream!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did use it now the last days full time and so far found no issues, at least for my use case with the home row mods this state fully replaces Achordion, thanks a lot. Hope it can be merged without any issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxzh @christoph-cullmann thank you both for the feedback! I agree, qualitatively this resembles and improves upon Achordion.