Fix Caps Word not working with Auto Shift and Combo feature #17240
Conversation
Processing Caps Word with only press events results in incompatibility with the Combo feature because not only some results of the combos but also some keys, which are part of the combos, make input with release events.
|
@getreuer just a heads up. Could you add additional tests for caps words, to verify that other features are working correctly? If you're not able to do so, let me know. But ideally, the unit tests should be expanded to verify that this is working correctly. |
| @@ -83,7 +83,13 @@ bool process_caps_word(uint16_t keycode, keyrecord_t* record) { | |||
| #endif // CAPS_WORD_IDLE_TIMEOUT > 0 | |||
|
|
|||
| // From here on, we only take action on press events. | |||
| if (!record->event.pressed) { | |||
| // The exceptions are the combo results and the chord keys, | |||
There was a problem hiding this comment.
At a glance, I thought that there was just kind of event generated from the combos implementation, where it calls process_record(), but I'm no expert. Are "combo result" and "chord key" synonymous terms, or is there a difference?
There was a problem hiding this comment.
I'm so sorry, I misclicked the "Resolve conversation" button for another conversation... Is there any way to undo resolving?
Edit: nevermind, I unresolved that.
| // which send keypress with a release event. | ||
| if (!record->event.pressed | ||
| #ifdef COMBO_ENABLE | ||
| && !caps_word_press_user(keycode) |
There was a problem hiding this comment.
I'm puzzled with how this logic mismatches the comment above it: "The exceptions are the combo results and the chord keys". As it is, the code is more like "We only take action on press events OR release events where caps_word_press_user() is true," which seems a lot broader than needed.
To focus the change in behavior to combos, could something like this work?
if (!record->event.pressed || IS_COMBOEVENT(record->event))
There was a problem hiding this comment.
Also this workaround looks really strange — the combo code should generate the same press and release events as would be generated for normal keys, and handling the release event here would result in setting weak mods on the release event too, which does not look correct. So I wonder what is the real problem here. Maybe the real bug is in the combo code, or maybe some other code calling clear_weak_mods() at some inappropriate time (normally clear_weak_mods() is called by action_exec() on press events before calling action_tapping_process() or process_record(), but it looks like that dump_key_buffer() in the combo code does that in the opposite order).
There was a problem hiding this comment.
@getreuer I refered this section of the docs for the terms "combo result" and "chord key".
For instance,
// name result chord keys
COMB(AB_MINS, KC_MINS, KC_A, KC_B)when this combo is defined, KC_MINS will be the "combo result" and, KC_A and KC_B will be the "chord keys".
Here the problem is, not only KC_MINS but also KC_A and KC_B won't shifted even after turning on Caps Word.
if (!record->event.pressed || IS_COMBOEVENT(record->event))
In order to work, the above code should be:
if (!record->event.pressed && !IS_COMBOEVENT(record->event))But this code won't completely fix the problem, because IS_COMBOEVENT() covers only "combo result" case.
By the way, I didn't know about IS_COMBOEVENT() and it seems to be better to use this macro (or is it a function?) if possible.
There was a problem hiding this comment.
the combo code should generate the same press and release events as would be generated for normal keys, and handling the release event here would result in setting weak mods on the release event too, which does not look correct.
I thought the same. What I found on debugging the current code -- before 5ca78fd -- is that when one of the "chord keys" is tapped, a keypress is actually passed through the caps_word_press_user() and true is returned, but what is actually typed is not a shifted character. That's why I guessed that they "send keypress with a release event", and this change actually fixed the issue.
There was a problem hiding this comment.
Maybe the real bug is in the combo code, or maybe some other code calling clear_weak_mods() at some inappropriate time (normally clear_weak_mods() is called by action_exec() on press events before calling action_tapping_process() or process_record(), but it looks like that dump_key_buffer() in the combo code does that in the opposite order).
Some notable behavior about it is that, when some Mod-Tap keys are part of the "chord keys", they are shifted flawlessly with Caps Word on, even if you do not redefine caps_word_press_user(). I hope there's no bigger problem than you said.
|
@drashna Thanks for the heads up. Yes, I'm happy to add additional unit tests for this PR and also the fix for Unicode Map #17247. @qyurila Thanks a bunch for bringing this up and the fixes. I'd like to check with you what exactly would repro these bugs so that the new tests test the right things.
|
|
I should have described it better. I'm sorry. What I fixed are about these scenarios (sorry for duplicating my comments on conversations) :
|
|
PR #17247 fixes Caps Word with Unicode Map. It does not address combos, but it does incidentally fix the problem with Caps Word with Auto Shift and Retro Shift noted here by reordering where the It looks like Caps Word + combos will need some more discussion. I suggest in the meantime to first merge #17284 to get those easy fixes in. |
|
In commit getreuer@bfddc1e, I wrote up a unit of using Caps Word together with Combos. The test checks that combo chord keys and result keys work as they should for several combos, including overlapping combos and combos that chord tap-hold keys. To check thoroughly, each scenario is repeated to press the combo chord keys with a few different orders and timings. Surprisingly, all tests of this kind that I have tried pass (on master and develop as of 2022-06-26). I was expecting rather that something in these tests would fail, and would be a good point to look further into this. Maybe the underlying problem has already been fixed (that would be cool), or the real problem is a scenario that hasn't been covered with this test? @qyurila could you please look over this test to see the combos it is checking? Let me know if there's a particular combo definition that should be tested to trigger the bug.
And see also the test's configuration in these files: |
|
@getreuer Thanks for your effort! It seems like the unit test covers all of the cases I encountered. Unfortunately, the problem still exists. In conclusion, the test fails if you add this line on AUTO_SHIFT_ENABLE = yesSo ultimately, It seems that the combo key problem occurs only when the chord keys are auto-shifted keys. (I probably mistook the condition when tweaking my keymap files. I apologize 🙏 ) |
|
Thanks @qyurila for pointing out to set After some printf-debugging, I found the root cause for the bug is that Combos calls For the above
A similar sequence happens with a combo result key when the
From these outlines, it becomes clear why the bug happens with all three features Caps Word + Combos + Auto Shift used together and not with Caps Word + Combos or Caps Word + Auto Shift.
The bug can be fixed by not clearing the weak Left Shift mod. I submitted a PR #17549 to make this change. |
|
Closing since other PR supersedes this |
Description
Currently Caps Word feature doesn't work with Auto Shifted keys, result keys and chord keys (which are the part of Combo feature).
This PR fixes the bug by reordering event processing order, and allowing Caps Word process for user Caps Word keys' release event when Combo feature is enabled.
Types of Changes
Checklist