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.

Sign up for GitHub

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

Fix Caps Word not working with Auto Shift and Combo feature #17240

Closed
wants to merge 2 commits into from
Closed

Fix Caps Word not working with Auto Shift and Combo feature #17240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a13ffeb
Fix Caps Word to work properly with Auto Shift
qyurila May 29, 2022
5ca78fd
Fix Caps Word to work properly with Combo feature
qyurila May 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion quantum/process_keycode/process_caps_word.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

@qyurila qyurila May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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))

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Author

@qyurila qyurila May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@sigprof

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.

Copy link
Author

Choose a reason for hiding this comment

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

@sigprof

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.

#endif // COMBO_ENABLE
) {
return true;
}

Expand Down
6 changes: 3 additions & 3 deletions quantum/quantum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,9 @@ bool process_record_quantum(keyrecord_t *record) {
#ifdef PRINTING_ENABLE
process_printer(keycode, record) &&
#endif
#ifdef CAPS_WORD_ENABLE
process_caps_word(keycode, record) &&
#endif
#ifdef AUTO_SHIFT_ENABLE
process_auto_shift(keycode, record) &&
#endif
Expand All @@ -307,9 +310,6 @@ bool process_record_quantum(keyrecord_t *record) {
#ifdef TERMINAL_ENABLE
process_terminal(keycode, record) &&
#endif
#ifdef CAPS_WORD_ENABLE
process_caps_word(keycode, record) &&
#endif
#ifdef SPACE_CADET_ENABLE
process_space_cadet(keycode, record) &&
#endif
Expand Down