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

Fix Caps Word capitalization when used with Combos + Auto Shift. #17549

Merged
merged 12 commits into from
Aug 13, 2022
Merged

Fix Caps Word capitalization when used with Combos + Auto Shift. #17549

merged 12 commits into from
Aug 13, 2022

Conversation

getreuer
Copy link
Contributor

@getreuer getreuer commented Jul 3, 2022

Caps Word + Combos + Auto Shift don't work together, as noted in PR #17240. This PR fixes it.

Description

When Auto Shift is enabled, Caps Word fails to capitalize combo "chord" keys and "result" keys.

The bug only happens where all three features Caps Word + Combos + Auto Shift are active. Caps Word works as intended on non-combo keys when Auto Shift is enabled or on combo keys when Auto Shift is disabled.

Additionally, unrelated to Combos, this PR incorporates zsa@42519ed, which fixes capitalization with rolled key presses when Caps Word + Auto Shift are both active. For instance, rolled keys "A down, B down, A up, B up" incorrectly produces "Ab" instead of "AB". The cause of this bug is similar, with Auto Shift clearing the weak Shift mod that Caps Word sets.

Example

Suppose a combo is defined such that pressing KC_A + KC_B types KC_X. Then,

  • (Chord key) If both Caps Word and Auto Shift are on, then tapping KC_A types "a", but Caps Word should capitalize it "A". Similarly for KC_B.
  • (Result key) Pressing the KC_A + KC_B chord while Caps Word is on types "x" but it should produce capital "X".

Investigation

After some printf-debugging, I found the reason for this is that Combos calls clear_weak_mods() within dump_key_buffer() after passing a chord key or result key event (code link), and this clears the weak Left Shift mod that Caps Word sets to capitalize the typed key.

For the above KC_A + KC_B = KC_X combo example, here is an outline of what happens when the chord key KC_A is tapped, with both Caps Word and Auto Shift turned on:

  1. Combo: gets the KC_A press event. The event is buffered and not passed to other handlers yet.
  2. Combo: gets the KC_A release event. The dump_key_buffer() function is called to pass along the tap on KC_A. The following all occurs within dump_key_buffer()...
  3. dump_key_buffer() calls action_tapping_process() to pass the buffered KC_A press event.
  4. Caps Word: gets the KC_A press event. The weak Left Shift mod is set to capitalize it.
  5. Auto Shift: gets the KC_A press event. The event is buffered and further handling is skipped.
  6. ⚠️ ROOT CAUSE HERE ⚠️ Execution returns from action_tapping_process(). dump_key_buffer() calls clear_weak_mods(), clearing the Left Shift mod that Caps Word had set.
  7. dump_key_buffer() calls action_tapping_process() a second time to pass the KC_A release event.
  8. Auto Shift: gets the KC_A release event and types lowercase "a".

A similar sequence happens with a combo result key when the KC_A + KC_B chord is tapped:

  1. Combo: gets the KC_A press event. This event is buffered.
  2. Combo: gets the KC_B press event. The dump_key_buffer() function is called to pass along the combo result key KC_X...
  3. dump_key_buffer() calls action_tapping_process(), passing a press event on KC_X.
  4. Caps Word: gets KC_X press event. The weak Left Shift mod is set to capitalize it.
  5. Auto Shift: gets KC_X press event and buffers it.
  6. ⚠️ ROOT CAUSE HERE ⚠️ Execution returns from action_tapping_process(). dump_key_buffer() calls clear_weak_mods(), clearing the Left Shift mod that Caps Word had set.
  7. dump_key_buffer() calls action_tapping_process(), passing a release event on KC_X.
  8. Auto Shift: gets KC_X release event and types lowercase "x".

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.

  • With Caps Word + Combos, but Auto Shift is disabled, the press event continues with default handling (and normally ultimately types a key) after Caps Word applies the weak Left Shift mod, instead of getting buffered in the Auto Shift handler.
  • With Caps Word + Auto Shift, and no Combos, the weak Left Shift mod that Caps Word sets during the press event survives until the release event, when Auto Shift types the key.

Fix

Change:

clear_weak_mods();

To:

del_weak_mods((is_caps_word_on() && get_autoshift_state())
              ? ~MOD_BIT(KC_LSFT) : 0xff);

Not clearing the weak Left Shift mod in dump_key_buffer() fixes the bug. It is still fine to clear the other weak mods. To make this change conservatively, we continue to clear all mods unless Caps Word and Auto Shift are both active.

Tests

This PR also includes a new "caps_word_combo" test to check Caps Word + Combos, with and without Auto Shift. These tests would fail currently, but pass with the above fix.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

getreuer added 6 commits June 26, 2022 10:52
Motivated by #17240, this commit
adds a unit test 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). Maybe the underlying problem has already
been fixed, or the real problem is a scenario that hasn't been covered
with this test?
@drashna drashna requested a review from a team July 3, 2022 17:17
@getreuer getreuer changed the title Fix/caps word combo Fix Caps Word capitalization when used together with Combos + Auto Shift. Jul 6, 2022
@getreuer getreuer changed the title Fix Caps Word capitalization when used together with Combos + Auto Shift. Fix Caps Word capitalization when used with Combos + Auto Shift. Jul 6, 2022
@drashna
Copy link
Member

drashna commented Jul 12, 2022

Not sure if you want to pile on here or add to a separate PR (since probably more tests)...

But caps words messes with autoshift, and autoshift messes with caps words, too.

This fixes it:
zsa@42519ed

@getreuer
Copy link
Contributor Author

@drashna thanks a bunch for sharing that commit! I'm happy to include the corresponding change here.

A few weeks back, PR #17284 was merged to (supposedly) fix Caps Word + Auto Shift and add a unit test. But I could imagine something else slipping through in addition to the "Caps Word + Combos + Auto Shift" scenario addressed in this PR.

Is there a particular situation where Caps Word + Auto Shift still breaks that I could add as a unit test?

@drashna
Copy link
Member

drashna commented Jul 13, 2022

Yeah, that was with #17284 applied already. So a few things did get through still. Though, given the complexity and such, I can't say I'm surprised.

For instance:

When auto shift is on, if I roll keys, the second key doesn’t get capitalized by Caps Word.
For example, without auto shift if I hit caps word, I get: JKL
With auto shift, I get: JkL

Note that this is using the keycode to trigger caps word, none of the other options for activating caps word.

The changes are based on customer reports from ZSA, and internal staff. The commit above is confirmed to fix the issues with both. And I can confirm the behavior, as well.

If Caps Word and Auto Shift are both active, then currently a rolled
press like "A down, B down, A up, B up" will incorrectly produce "Ab".
This is because Auto Shift clears the weak shift mod when the A key
is released, which in the case of a roll is *after* Caps Word has set
the weak shift mod for B when it was pressed.

This commit incorporates
zsa@42519ed
which fixes it. An additional test `AutoShiftRolledShiftedKeys` is
added, which would previously fail but passes with this fix.
@getreuer
Copy link
Contributor Author

Thanks @drashna for that clear example! I incorporated your fix into this PR and added a test AutoShiftRolledShiftedKeys to check this rolled keys behavior.

@github-actions github-actions bot removed the keyboard label Aug 7, 2022
@drashna
Copy link
Member

drashna commented Aug 7, 2022

Thanks @drashna for that clear example! I incorporated your fix into this PR and added a test AutoShiftRolledShiftedKeys to check this rolled keys behavior.

Awesome!

Though, I'm still getting reports of it happening in rare occasions when typing very fast.

(with caps words and autoshift enabled:

Normal typing speed: cAPSWOrD cAPSWOrD cAPSWOrD cApSWOrD CAPWOrD
Slow typing speed: CAPSWORD CAPSWORD CAPSWORD CAPSWORD

However, I don't think this is a show stopper here. Mostly, wanted to let you know there is some sort of edge case here (it was much worse before the patch I mentioned.

@getreuer
Copy link
Contributor Author

getreuer commented Aug 7, 2022

Got it, thanks for the heads up about the fast typing.

Is it by chance an issue reported on Mac OS particularly? I wonder if the following is related: On my qmk-keymap repo, I've had a couple issues where "add_mods() + register_code()" worked for me on Linux, but a couple Mac OS users noticed the mod not getting applied until after the key. The solution was to split the mod and key into two keyboard reports by using register_mods() or adding an extra send_keyboard_report() call (getreuer/qmk-keymap#5).

@tzarc tzarc merged commit 6a0d90f into qmk:develop Aug 13, 2022
@drashna
Copy link
Member

drashna commented Aug 14, 2022

Primarily, yeah, on MacOS, but also Windows, and appears to be a bit inconsistent when it will happen. But the suggested changes seem to help, but not completely.

@getreuer getreuer deleted the fix/caps_word_combo branch August 22, 2022 02:32
uvera pushed a commit to uvera/qmk_firmware that referenced this pull request Dec 14, 2022
uvera pushed a commit to uvera/qmk_firmware that referenced this pull request Dec 14, 2022
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 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.

3 participants