OSMs on OSL stopped resetting default layer

[bencollerson]

Describe the Bug

After rebasing my qmk branch on the current master (v 0.23.0) I noticed my "callum style" one shot mods (mods on a different layer) were no longer resetting the active layer back to the default. I tracked this new behaviour down to this patch:

e0eb90aba1 [Mon Sep 25 12:53:12 2023] Fix OSM on a OSL activated layer (#20410) [GitHub

Reverting this patch restores the previous behaviour of one shot mods and one shot layers which I was relying on. Maybe there should be some sort of option to allow for both types of behaviour?

Keyboard Used

contra

Link to product page (if applicable)

No response

Operating System

Debian linux

qmk doctor Output

$ qmk doctor
Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.1
Ψ QMK home: /home/benc/work/qmk
Ψ Detected Linux (Debian GNU/Linux trixie/sid).
Ψ Userspace enabled: False
Ψ Git branch: HEAD
Ψ Repo version: 0.23.0
Ψ - Latest HEAD: 2023-11-29 10:38:45 +1000 (529cde34d1) -- Revert "Fix OSM on a OSL activated layer (#20410)"
Ψ - Latest upstream/master: 2023-11-29 01:07:21 +1100 (049e964e61) -- Attempt to fix configurator. (#22555)
Ψ - Latest upstream/develop: 2023-11-28 14:07:50 +0000 (a2c745cae8) -- Merge remote-tracking branch 'origin/master' into develop
Ψ - Common ancestor with upstream/master: 2023-11-29 01:07:21 +1100 (049e964e61) -- Attempt to fix configurator. (#22555)
Ψ - Common ancestor with upstream/develop: 2023-11-29 01:07:21 +1100 (049e964e61) -- Attempt to fix configurator. (#22555)
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 12.3.1
Ψ Found avr-gcc version 7.3.0
Ψ Found avrdude version 7.1
Ψ Found dfu-programmer version 0.6.1
Ψ Found dfu-util version 0.11
Ψ Submodules are up to date.
Ψ Submodule status:
Ψ - lib/chibios: 2023-04-15 13:48:04 +0000 --  (11edb1610)
Ψ - lib/chibios-contrib: 2023-07-17 11:39:05 +0200 --  (da78eb37)
Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 --  (e2239ee6)
Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 --  (549b97320)
Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 --  (819dbc1)
Ψ - lib/printf: 2022-06-29 23:59:58 +0300 --  (c2e3b4e)
Ψ - lib/pico-sdk: 2023-02-12 20:19:37 +0100 --  (a3398d8)
Ψ - lib/lvgl: 2022-04-11 04:44:53 -0600 --  (e19410f)
Ψ QMK is ready to go

Is AutoHotKey / Karabiner installed

• AutoHotKey (Windows)
• Karabiner (macOS)

Other keyboard-related software installed

No response

Additional Context

For reference the rebased qmk code with revert that fixes the issue I was having is here:

https://github.com/bencollerson/qmk_firmware/tree/fixedrebase

[sigprof]

You can reimplement the behavior of OSM() keys in process_record_user(), mostly duplicating this code:

qmk_firmware/quantum/action.c

Lines 425 to 485 in 4228f55

	case MODS_ONESHOT:
	// Oneshot modifier
	if (!keymap_config.oneshot_enable) {
	if (event.pressed) {
	if (mods) {
	if (IS_MODIFIER_KEYCODE(action.key.code) || action.key.code == KC_NO) {
	// e.g. LSFT(KC_LGUI): we don't want the LSFT to be weak as it would make it useless.
	// This also makes LSFT(KC_LGUI) behave exactly the same as LGUI(KC_LSFT).
	// Same applies for some keys like KC_MEH which are declared as MEH(KC_NO).
	add_mods(mods);
	} else {
	add_weak_mods(mods);
	}
	send_keyboard_report();
	}
	register_code(action.key.code);
	} else {
	unregister_code(action.key.code);
	if (mods) {
	if (IS_MODIFIER_KEYCODE(action.key.code) || action.key.code == KC_NO) {
	del_mods(mods);
	} else {
	del_weak_mods(mods);
	}
	send_keyboard_report();
	}
	}
	} else {
	if (event.pressed) {
	if (tap_count == 0) {
	// Not a tap, but a hold: register the held mod
	ac_dprintf("MODS_TAP: Oneshot: 0\n");
	register_mods(mods);
	} else if (tap_count == 1) {
	ac_dprintf("MODS_TAP: Oneshot: start\n");
	add_oneshot_mods(mods);
	# if defined(ONESHOT_TAP_TOGGLE) && ONESHOT_TAP_TOGGLE > 1
	} else if (tap_count == ONESHOT_TAP_TOGGLE) {
	ac_dprintf("MODS_TAP: Toggling oneshot");
	register_mods(mods);
	del_oneshot_mods(mods);
	add_oneshot_locked_mods(mods);
	# endif
	}
	} else {
	if (tap_count == 0) {
	// Release hold: unregister the held mod and its variants
	unregister_mods(mods);
	del_oneshot_mods(mods);
	del_oneshot_locked_mods(mods);
	# if defined(ONESHOT_TAP_TOGGLE) && ONESHOT_TAP_TOGGLE > 1
	} else if (tap_count == 1 && (mods & get_mods())) {
	unregister_mods(mods);
	del_oneshot_mods(mods);
	del_oneshot_locked_mods(mods);
	# endif
	}
	}
	}
	break;
	# endif

If your reimplementation returns false from process_record_user(), the one shot layer will be reset after pressing those keys:

qmk_firmware/quantum/action.c

Lines 274 to 281 in 4228f55

	if (!process_record_quantum(record)) {
	#ifndef NO_ACTION_ONESHOT
	if (is_oneshot_layer_active() && record->event.pressed && keymap_config.oneshot_enable) {
	clear_oneshot_layer_state(ONESHOT_OTHER_KEY_PRESSED);
	}
	#endif
	return;
	}

(Actually you may just try to call clear_oneshot_layer_state(ONESHOT_OTHER_KEY_PRESSED) from process_record_user(), and then return true to use the default implementation — this has some chance to work.)

It could be possible to resurrect #9538 for a more general solution, although I don't really like the way how a tri-state value is implemented there.

[bencollerson]

Yeah I could do that, but I already have a one line fix by reverting the change that broke my layout.

[KoFish]

void post_process_record_user(uint16_t keycode, keyrecord_t *record) {
    if (IS_QK_ONE_SHOT_MOD(keycode) && is_oneshot_layer_active() && record->event.pressed) {
        clear_oneshot_layer_state(ONESHOT_OTHER_KEY_PRESSED);
    }
    return;
}

I also had this problem and actually agree that the current behavior of QMK is how it should be... for my layout it doesn't work tho, so I added this fix to my userspace, and it seems to be working perfectly.

[bencollerson]

Okay maybe that is a better solution. Will test it out.

[bencollerson]

Not sure I agree with this change that broke OSMs on OSLs. Maybe I just don't understand the reasoning. But KoFish's solution above works to make OSLs function in a way that makes sense to me.

[NapOli1084]

IMHO the change didn't break OSMs on OSLs, it fixed it. It was inconsistent that all mods don't reset the OSL except with OSM. And that prevented having a OSL-activated layer containing OSM and use it to shift a key on the same layer.

[bencollerson]

All good. I will close this issue as KoFish's userspace solution above restores the old OSM/OSL behaviour for people like me who use it.