[Bug] Space Cadet Shift and Auto Shift interaction produces unwanted () characters

[lanslans]

Describe the Bug

Enable space cadet shift - left shift (, right shift ) Enable auto shift

Quickly type shift-letter, and release shift before releasing the letter.

When using left shift, the ( will be before the letter. When using right shift, the ) will be after the letter.

If you release shift after releasing the letter, the () will not print, and the keyboard works as you would want.

Keyboard Used

Leopold FC980m modded to run QMK off a bluepill

Link to product page (if applicable)

https://geekhack.org/index.php?topic=106758.0

Operating System

No response

qmk doctor Output

No response

Is AutoHotKey / Karabiner installed

• [ ] AutoHotKey (Windows)
• [ ] Karabiner (macOS)

Other keyboard-related software installed

No response

Additional Context

No response

[sigprof]

process_space_cadet() needs to get all key press events, so that it is able to distinguish between taps and holds of Space Cadet keys (if any other key is pressed while a Space Cadet key is held down, the Space Cadet key is handled as a modifier hold even it gets released before the tapping term expires — this implements mostly the same logic as HOLD_ON_OTHER_KEY_PRESS for MT()/LT()).

However, process_record_quantum() calls process_space_cadet() later than process_auto_shift(), and process_auto_shift() returns false for key press events of auto-shifted keys (so that it is able to disable the normal press action for those keys, and decide whether to send a normal or a shifted key at the key release time). Because of that, any subsequent process_*() handlers for auto-shifted keys are not invoked, and this includes process_space_cadet(), therefore the Space Cadet code does not detect that some other key has been pressed together with the Space Cadet key, and will execute the tap action for the Space Cadet key if it gets released earlier than the tapping term.

The difference between the two shift keys is because the Auto Shift code treats only the left Shift modifier specially — it assumes that MOD_BIT(KC_LSFT) has been set by the Auto Shift code itself, but treats MOD_BIT(KC_RSFT) as an extra modifier if AUTO_SHIFT_MODIFIERS is not defined, and in that case the Auto Shift code emits the unshifted key immediately on press, instead of emitting the appropriately shifted key on release.

One way to fix this in the core is to move the call to process_space_cadet() so that it happens before process_auto_shift(), then process_space_cadet() would be able to detect the press of any auto-shifted keys and disable its tap action appropriately. This might still fail in some other cases though, because any other feature that returns false from its process_*() function will cause the same problem, so process_space_cadet() may need to be moved even earlier (e.g., the key override code may also return false for some keys).

Replacing Space Cadet keys with MT() and a customized tap action should work too, but would introduce a delay between the physical key press and the modifier registration, which may not be desired. Using a tap dance would also result in some undesired delays (the tap dance code does not have a callback to handle key release immediately, so an isolated tap would be handled only after the tapping term had expired). Implementing a custom space-cadet-like behavior in process_record_user() could be an option (that function is called much earlier than process_space_cadet()).

[lanslans]

Do you know what changed to cause this issue?

The last time I built QMK firmware (December 2020), this wasn’t an issue.

[sigprof]

On QMK 0.11.x (0.11.0 was released on 2020 Nov 28) I would expect that this problem would appear for the left Shift key, but not for the right Shift; the Auto Shift implementation in 0.11.0 had this code:

#    ifndef AUTO_SHIFT_MODIFIERS
    if (get_mods() & (~MOD_BIT(KC_LSFT))) {
        return true;
    }
#    endif

However, maybe you were still using some 0.10.x version of QMK (0.10.0 was released on 2020 Aug 29); the Auto Shift implementation in that version had this code:

#    ifndef AUTO_SHIFT_MODIFIERS
                if (get_mods()) {
                    return true;
                }
#    endif

So if you were using 0.10.x, any Shift modifier that was registered by a Space Cadet key prevented the Auto Shift code from activating for the next keypress, therefore that keypress was handled normally and reached the Space Cadet code as intended. But some subsequent Auto Shift improvements changed that code:

• 9826 (included in 0.11.0) changed the non-AUTO_SHIFT_MODIFIERS condition to get_mods() & (~MOD_BIT(KC_LSFT)), apparently to support the newly added AUTO_SHIFT_REPEAT mode (in that case the left Shift modifier is already registered by the Auto Shift code).

• 12083 (0.12.29) restored the non-AUTO_SHIFT_MODIFIERS condition to be just get_mods() again, apparently because the previous change also broke the normal usage of left Shift with auto-shifted keys (no idea whether this change broke the AUTO_SHIFT_REPEAT mode).

• 11059 (included in 0.15.0) added the & (~MOD_BIT(KC_LSFT)) part again, and also replaced return true in that branch with the code that emitted the unshifted value and returned false (apparently to support custom unshifted values through autoshift_press_user()); this is probably the change that broke compatibility with Space Cadet completely.

[lanslans]

https://github.com/lanslans/qmk_firmware/commit/37d3fb37efd7eeaa473c0afdfa3c58e3ec4dc5fb

I fixed it here on my own repo.

I'm hesitant to make a pull request for this, because I don't know if it will screw things up for anyone else.

[IsaacElenbaas]

Auto Shift is horribly complicated now to work with Tap Holds and such (though some of that can be removed once I fix conflicts in my bugfix PR for it). For most features it doesn't work with, especially where the issues are related to which feature eats keys, I would say yeah - just move the other above Auto Shift if that works well enough.

Space Cadet is trivial to reimplement through custom auto shift keys if people use both and are having issues.