[Bug] Swap-hands breaks OSL one shot keys

[ilikeheaps]

SH_T(KC_SPC) seems to cancel one shot layer.

"r / 2"                                -> r
OSM(MOD_LSFT) then OSL(1) then "r / 2" -> @
SH_T(KC_SPC) +  "r / 2"                -> i
SH_T(KC_SPC) +  OSL(1) + "r / 2"       -> 9
OSL(1) then SH_T(KC_SPC) +  "r / 2"    -> r ***expected 9***
toggle tap OSL(1); OSL(1) + "r / 2"    -> 9

[ilikeheaps]

Perhaps a fix similar to #6943 (comment) would work, I might try it if I find time.

[ilikeheaps]

Perhaps a fix similar to #6943 (comment) would work, I might try it if I find time.

Seems like code covers swap-hand but misses this specific case of "tap-swap".

#    ifdef SWAP_HANDS_ENABLE
        && !(action.kind.id == ACT_SWAP_HANDS && action.swap.code == OP_SH_ONESHOT)
#    endif

[sigprof]

action.kind.id == ACT_SWAP_HANDS && action.swap.code == OP_SH_ONESHOT should match QK_SWAP_HANDS_ONE_SHOT (SH_OS), not SH_T(kc). So that part of code keeps the one shot layer active only for SH_OS, and SH_T(kc) is not treated as a special case anywhere.

Maybe I should revive #9538, so that various user preferences in that area could be handled properly; one problem is that the API does not look good.

[ilikeheaps]

Attempted a fix without understanding the code and guess what, seems like it broke something else. It might be more useful to me right now anyway.

diff --git a/quantum/action.c b/quantum/action.c
index 29822c39e9..3ad38b983a 100644
--- a/quantum/action.c
+++ b/quantum/action.c
@@ -378,7 +378,9 @@ void process_action(keyrecord_t *record, action_t action) {
 #    endif
                                               ))
 #    ifdef SWAP_HANDS_ENABLE
-        && !(action.kind.id == ACT_SWAP_HANDS && action.swap.code == OP_SH_ONESHOT)
+        && !(action.kind.id == ACT_SWAP_HANDS && (action.swap.code == OP_SH_ONESHOT
+                                                  || action.swap.code < 0xF0 // swap-hand tap key
+                                                  ))
 #    endif
         && keymap_config.oneshot_enable) {
         clear_oneshot_layer_state(ONESHOT_OTHER_KEY_PRESSED);
-- 

Now:

"r / 2"                                   -> r
OSM(MOD_LSFT) then OSL(1) then "r / 2"    -> @
SH_T(KC_SPC) +  "r / 2"                   -> i
SH_T(KC_SPC) +  OSL(1) + "r / 2"          -> 9
OSL(1) then SH_T(KC_SPC) +  "r / 2"       -> 9
toggle tap OSL(1); SH_T(KC_SPC) + "r / 2" -> 9

OSM(MOD_LSFT) then SH_T(KC_SPC) +  "r / 2"    -> I
OSM(MOD_LSFT) then quickly SH_T(KC_SPC) +  "r / 2" -> <SPC>, r ***expected I***

[ilikeheaps]

action.kind.id == ACT_SWAP_HANDS && action.swap.code == OP_SH_ONESHOT should match QK_SWAP_HANDS_ONE_SHOT (SH_OS), not SH_T(kc). So that part of code keeps the one shot layer active only for SH_OS, and SH_T(kc) is not treated as a special case anywhere.

I suppose the issue with handling SH_T here is that we can't yet know whether it will become a "modifier" or a tap key?

[sigprof]

record->tap.count is available in process_action() (already cached in tap_count), so you can determine whether the action is a tap or a hold and act accordingly (there is a check like that in the mod tap handling part above).

[ilikeheaps]

Thanks, now tapping SH_T properly releases one shot layers/modifiers. I still have some issues, I guess due to delay in OSM (#1247 ?).

OSM(MOD_LSFT), wait ~0.5s, then SH_T(KC_SPC) +  "r / 2"    -> I
OSM(MOD_LSFT) then quickly SH_T(KC_SPC) +  "r / 2" -> <SPC>, r ***expected I***

(same issue with OSL)

[sigprof]

That old issue looks unrelated, and from your report it seems that in the SH_T(KC_SPC) + "r / 2" your SH_T(KC_SPC) press gets handled like a tap instead of a hold — so either you need to do some tuning for your tap/hold decision modes, or maybe the tap detection gets disturbed by the previous OSM(MOD_LSFT) press (may it be the case that OSM(MOD_LSFT) is not yet released before SH_T(KC_SPC) is pressed?).

[ilikeheaps]

While analyzing the logs I found out (a part of?) the issue happens on master regardless of one shot feature: SH_T(KC_SPC) + "r / 2" produces i or i depending on timing. Posted that in #24410

[ilikeheaps]

Code I wrote above along with a (one-line) fix mentioned in #24420 resolved my issues (there remains an issue with PERMISSIVE_HOLD, also in that draft, but I'm not using that mode anymore). If someone wants to check the changes I would tidy them up (and I could substantiate how it increases the utility of OSM and SH for one handed use), otherwise this may be closed.