[Core] Add Repeat Key ("repeat last key") as a core feature. #19700
Conversation
This commit adds Repeat Key as a core feature, including documentation and tests. The implementation is based on the userspace implementation <https://getreuer.info/posts/keyboards/repeat-key>. Differences in this implementation: * When generating repeated key events, my userspace implementation temporarily sets the layer state to what it was when the key was originally pressed. This implementation instead leverages the .keycode field in the keyrecord_t so that the temporary layer state switching isn't needed. * Thanks to the previous point, this implementation is able to add a `set_repeat_key_keycode(kc)` API. * This implementation removes repeat_key_register() and repeat_key_unregister(). I realize these APIs raise more trouble than they're probably worth, since it's easy to accidentally make a key that uselessly attempts to repeat itself. * I omit the "Reverse Repeat Key." On the one hand, I haven't yet found a nice way to supply a default for the "reverse key pairs table." On the other hand, it is fairly doable given Repeat Key's `get_repeat_key_keycode()` to implement a Reverse Repeat Key in userspace.
|
Here is an outline for navigating the code changes in this PR. Setup
Sequence of events for Repeat Key
Reverse RepeatReverse Repeat works the same way except for the keycode. When Reverse |
|
|
Thanks, @drashna! I updated test_common/keycode_table.cpp. FWIW, I just learned there is a handy script util/regen.sh that runs all keycode regenerators. |
|
Ah, the test function PR was merged. I wasn't sure if it was or not yet. But yeah, that works for that. That said, there are some merge conflicts here, as well as keycode value collisions due to the tri layer PR getting merged first. |
|
Done, merged with the develop branch. It's interesting to see the tri layer changes. |
|
I am currently running this branch and using both the repeat key and reverse repeat key in my layout. Thus far both have worked exactly as I expected them to. I have no comments on the code, but I do question the naming of the Reverse Repeat functionality. This key can be fully dynamic based on the last key press and need not be constrained to logically reversing that keypress. In my keymap, I think of it as the "magic" key, and have built a dictionary that eliminates the worst single finger bigrams that would otherwise exist on the layout. I suggest that the keycode be something like I think it would be worth adding some documentation that highlights this use-case, even if it is still called Reverse Repeat. |
|
@Ikcelaks thanks for checking out the PR and this excellent point! You are right, Reverse Repeat can be used for much more than "reversing", and this is well worth clarifying. Leveraging it to remove SFBs is an awesome idea, you inspire me to experiment with that. I renamed Reverse Repeat to Alternate Repeat (and " |
|
To follow up, the recent revisions to @Ikcelaks thank you for that context on STRD1 and Magic Sturdy! I found your Magic Sturdy writeup, thanks for doing this. You have gained a user! I've been test driving Magic Sturdy since Friday. As luck would have it, I've been experimenting with a Sturdy mod that was only a few keyswaps away, so I'm getting up to speed quickly and excited to begin getting a "taste of the power" with the right index magic key. That "-ion" shortcut is very nice. |
|
I'm a GitHub noob and have a question. I made the changes that @sigprof requested, but the PR status still shows "Changes requested." Should I be clicking something to clear that, or is that up to reviewers to resolve?
In any case, this PR is ready to resume review. |
|
I see that this PR is tagged for breaking_change_2023q2. Does this mean that it's scheduled to make a particular release? Is there anything needed from a testing perspective that I can help with? I've been running my Moonlander on this PR branch for the past couple months, and it has worked splendidly. If there is concern about how this might interact with other features, I would be willing to test. The masses need access to this incredibly powerful feature. |
|
@Ikcelaks thank you so much for your support and offer to help! Testing interaction with other features would absolutely be appreciated. I also welcome your review comments if you have further thoughts on the code.
I described in my previous comment that the PR status shows "Changes requested". I have made those changes, but I am a GitHub noob and not certain what the process is from here. I think the next step is that @sigprof would at their discretion clear the "Changes requested" state in a re-review. Let me know if I'm missing something! Besides this, yes, @drashna applied the "breaking_change_2023q2" tag. My understanding is that this is tentative, since changes to core require at least two approvers to merge. So far, drashna has approved and requested review from other core QMK members. Thank you, drashna! |
Nope, don't need to do anything. Github ... is a bit odd in some places. The sidebar with reviews is more accurate, as the "changes requested" part doesn't actually get updated until a new review is added by that reviewer or is manually cleared. It's a bit odd.
It's a way for us to filter PRs, and give a bit higher priority to getting them reviewed, so it's more of a soft target for them. And yeah, generally want two reviews for PRs |
|
@drashna thanks for clearing that up! Good to hear the PR is on track, and I appreciate that the breaking_change_2023q2 tag improves review priority a bit. |
|
@Ikcelaks thank you for the testing and the heads up! I just did a merge. What was the scenario with Combos that interacted badly? I'd like to add a unit test for that. |
The last key tracking in Repeat Key's implementation is useful for things far removed from Repeat Key. Referring to APIs with "repeat_key" in the name is out of place and potentially confusing. It also occurred to me that future QMK features might want to share this last key logic, for instance if the Adaptive Keys PR qmk#14034 is revived, in which case it would help to name the APIs relating to the last key more generically and separately from Repeat Key. This commit renames * get_repeat_key_eligible_user() -> remember_last_key_user() * get_repeat_key_keycode() -> get_last_keycode() * set_repeat_key_keycode() -> set_last_keycode() * get_repeat_key_mods() -> get_last_mods() * set_repeat_key_mods() -> set_last_mods() and segments last key logic to a separate process_last_key() handler.
There is no bad interaction. I was just encountering the independent regression that is now fixed. I have verified that my combos are now working as expected. I like the new function names. In particular |
The last key tracking in Repeat Key's implementation is useful for things far removed from Repeat Key. Referring to APIs with "repeat_key" in the name is out of place and potentially confusing. This commit renames * get_repeat_key_eligible_user() -> remember_last_key_user() * get_repeat_key_keycode() -> get_last_keycode() * set_repeat_key_keycode() -> set_last_keycode() * get_repeat_key_mods() -> get_last_mods() * set_repeat_key_mods() -> set_last_mods() This matches the API names in the upcoming Repeat Key implementation in QMK core qmk/qmk_firmware#19700
Co-authored-by: casuanoob <[email protected]> Co-authored-by: Sergey Vlasov <[email protected]>
Co-authored-by: casuanoob <[email protected]> Co-authored-by: Sergey Vlasov <[email protected]>
This PR implements Repeat Key as a core feature. I previously implemented a userspace Repeat Key implementation that this one is based on.
Description
The Repeat Key performs the action of the last pressed key. There are two ergonomically-grounded motivations for Repeat Key:
As pointed out by NotGate, it is attractive from the point of view that double-tapped letters are like same-finger bigrams (SFBs): "The repeat key fits the theme of reducing SFBs by as much as possible and has the bonus of including the 10th finger. Words like follow become 'fol[rep]ow' so your flow is never broken by the speed of a single finger."
It is useful for hotkey chords, like repeating Ctrl+Shift+Right to select by word or repeating Ctrl+PageUp / Ctrl+PageDown to switch browser tabs.
On that second point, hotkeys are quite often in pairs, navigating or acting in one direction or the reverse. This feature also includes a Reverse Repeat Key for such situations. The Reverse Repeat Key performs the "reverse" of the last key, if a reverse is defined. When the last key wast Ctrl+Shift+Right, the Reverse Repeat Key performs Ctrl+Shift+Left. Together with the Repeat Key, this enables convenient selection by words in either direction.
The implementation interoperates well with other features. It is essentially a one-key version of how dynamic macros work: pressing the Repeat Key (or Reverse Repeat Key) generates a
keyrecord_tand passes it toprocess_record(). This interoperates predictably with all QMK features that I have tested it with. This includes, for example, macros with custom handlers inprocess_record_user(): tapping the macro key and then Repeat Key invokes the macro twice.The implementation is space efficient. On my keymap, it adds 686 bytes to the firmware size. If Reverse Repeat Key is disabled (there is a
NO_REVERSE_REPEAT_KEYoption), the cost reduces to 350 bytes.Please see the documentation
docs/feature_repeat_key.mdfor further description.Types of Changes
Checklist