Skip to content

Ignore space cadet key release when caps word is active #21721

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

Merged
merged 8 commits into from
Jan 9, 2024
Merged

Ignore space cadet key release when caps word is active #21721

merged 8 commits into from
Jan 9, 2024

Conversation

p3l6
Copy link
Contributor

@p3l6 p3l6 commented Aug 9, 2023

Description

If using BOTH_SHIFTS_TURNS_ON_CAPS_WORD and Space cadet, activating Caps word before the tapping term expires produces either an unwanted 0 or 9, depending on which shift was pressed first. A deeper explanation of the cause can be found in the linked issue below.

In this change, when shift if released, I check to see if caps word is enabled, and then perform the same space cadet code branch as if the tapping term had expired. That is to say, releasing the modifier without typing the parenthesis. This works because caps word is processed first, so when entering caps word mode, the flag will already be set when processing space cadet. In the case that a shift key is tapped to exit caps word, the flag will be disabled before space cadet processes, and the key will be processed normally.

Types of Changes

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

Issues Fixed or Closed by This PR

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

@github-actions github-actions bot added the core label Aug 9, 2023
@getreuer
Copy link
Contributor

getreuer commented Aug 9, 2023

@p3l6 thank you for contributing this fix!

My impression is this fix makes sense. It is delicate logic how Caps Word and Space Cadet features interoperate. To avoid regression where some future change breaks this behavior again, it would be a good idea to add a unit test to cover BOTH_SHIFTS_TURNS_ON_CAPS_WORD + Space Cadet.

In case you are new to testing in QMK, here are a few notes. I suggest:

  1. Add a test case covering this change in tests/caps_word/test_caps_word.cpp.
  2. Test it by running make test:caps_word.

Tests written using the GoogleTest framework. Take a look at the existing tests in the same file examples of how GoogleTest is used in QMK to simulate and test key events.

Comment on lines 96 to 100
#ifdef BOTH_SHIFTS_TURNS_ON_CAPS_WORD
if (sc_last == holdMod && timer_elapsed(sc_timer) < GET_TAPPING_TERM(sc_keycode, record) && !is_caps_word_on()) {
#else
if (sc_last == holdMod && timer_elapsed(sc_timer) < GET_TAPPING_TERM(sc_keycode, record)) {
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This change effectively blocks taps of all Space Cadet keys while Caps Word is active. Because Space Cadet keys are customizable and can be used also with other modifiers than just Shift, this restriction may not be desired.

An alternative way to fix the problem might be:

  • Add a function to reset the Space Cadet state (the same thing happens when any other key is pressed while a Space Cadet key is held down):
void reset_space_cadet(void) {
    sc_last = 0;
}
  • Call reset_space_cadet() from the Caps Word code when the press of both shifts is detected.

This way only the last tap of Space Cadet Shift which activates Caps Word would be forcibly converted to a hold, and other usage of Space Cadet keys during Caps Word would remain undisturbed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this additional context and write-up, I've re-worked the change in the way you suggested.

@p3l6
Copy link
Contributor Author

p3l6 commented Aug 11, 2023

@getreuer When I went to add a new test case, I noticed that the existing cases already covered this combination, but had an exception to wait for the Tapping term to expire to work around the issue. I think removing tapping term in the existing tests could be sufficient, but let me know if you think an explicitly named case would still be good.

I checked that

  • Adjusting the test without the fix does fail due to the extra 0 produced
  • Tests pass with the fix

@sigprof
Copy link
Contributor

sigprof commented Aug 11, 2023

Another feature which probably won't work together with Space Cadet is CAPS_WORD_INVERT_ON_SHIFT (the code for that feature actually intercepts KC_LSFT, KC_RSFT and the MT() and OSM() keycodes for the same modifiers and handles the modifier change in a special way instead of registering the modifier as usual). Not sure how to fix that properly though without introducing some more assumptions about the space cadet configuration (in theory it is possible to configure QK_SPACE_CADET_LEFT_SHIFT_PARENTHESIS_OPEN and QK_SPACE_CADET_RIGHT_SHIFT_PARENTHESIS_CLOSE to use a completely different modifier, and detecting whether the intended space cadet action is actually a tap or a hold is not possible outside of the space cadet code).

@p3l6
Copy link
Contributor Author

p3l6 commented Oct 11, 2023

I am also not familiar enough (or at all) with CAPS_WORD_INVERT_ON_SHIFT to want to attempt any similar fix myself.

Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Nov 26, 2023
@drashna drashna requested a review from a team December 6, 2023 06:50
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Dec 7, 2023
@tzarc tzarc merged commit 1f6dfd1 into qmk:develop Jan 9, 2024
nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024
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.

5 participants