Magic up action codes to respect wizardry

[drashna]

Description

Fixes up some of the quantum handlers to properly respect magic status.

• Note: This is likely a breaking change since it will change existing behavior.

Types of Changes

• [x] Core
• [x] Bugfix
• [x] Enhancement/optimization

Issues Fixed or Closed by This PR

• Chat on discord

Checklist

• [x] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [x] I have read the CONTRIBUTING document.
• [x] I have added tests to cover my changes.
• [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[sigprof]

After some more thinking I'm not even sure that we are moving in the right direction here.

Most likely the intended purpose of this part of the bootmagic configuration is to swap or disable the keys based on their physical location, and not change the function of keycodes explicitly assigned in the keymap. E.g., if keycode_config.no_gui is enabled, it should stop the physical GUI keys from functioning, but if the keymap contains LGUI(something) somewhere, I'm not sure that stripping the LGUI modifier from that mapping is intended. Similarly, when swapping of GUI and Alt keys is enabled, it should probably affect just the action of those physical keys, and not keymap entries with those modifiers that may be assigned to some other keys on some layers.

Unfortunately, I don't see a good way to make this function behave sanely without breaking some use cases. Especially the QK_MODS handling change looks problematic.

[drashna]

@sigprof that's fine. I think it may still be a good idea to include, but it may be worth adding settings to control the behavior (like "strict bootmagic processing", or some such).

However, I think that introducing this behavior is correct. It may have been intended for physical locations initially, but .... it produces inconsistent behavior that may not be expected, in general.

I would prefer to have a consistent behavior between all of the keycodes, rather than the alternative.

However, I do like the discussion here, and I may see about adding a define to change the handling.

[skullydazed]

However, I think that introducing this behavior is correct. It may have been intended for physical locations initially, but .... it produces inconsistent behavior that may not be expected, in general.

I'm not sure I agree with your last sentence. I think changing the current behavior would result in more confusion and surprise. I think by default people think of the keymap and what happens in code as two separate things.

I can see the use case for enabling this new behavior with a define, but for the moment my thinking is that the default should behavior should be the current behavior.

[drashna]

I can see the use case for enabling this new behavior with a define, but for the moment my thinking is that the default should behavior should be the current behavior.

That was actually discussed on discord. Though, I wasn't sure if that was wha we wanted or not.

Also, the fact that I started this PR at 5am, due to insomnia means that it was a fully thought out change, and more of a minimum viable change.

I'll see about adding toggle-able/definable check here, soon.

[drashna]

https://discord.com/channels/440868230475677696/473506116718952450/1002342277156438076

Regardless, all I've done is put the key code CG_TOGG in a regular key position on a layer. I activate the layer and then strike that key. I then return to my default layer and press (what should now be) Ctrl-Tab, only to find that the behavior for Gui-Tab triggers. Am I misunderstanding how this toggle is supposed to work?

May be better to remove the define.

[sigprof]

So it seems that some people really want this feature:

• #18349
• #18353

The suggested implementation in #18353 does things in a different way — it does not touch action_for_keycode() and instead performs all remapping inside keycode_config(); this actually can be more useful, but also performs some parsing of quantum keycodes twice. It could even be changed to apply mod_config() too instead of doing that in action_for_keycode(), so that it would be a complete remapping function for quantum keycodes (although we should test corner cases like MT(MOD_LGUI, kc) with no_gui, which would end up removing the only modifier from MT).

The remaining issue (regardless of which way to implement the feature is chosen) is where exactly the mapping should be applied. With both this implementation and #18353 the mapping would be applied to anything that ends up in process_record_handler(), which includes not only the keycodes placed in the keymap or encoder map, but also the output keycodes of combos, but not to the keycodes emitted by some other features:

• space cadet
• key overrides (that case is especially complicated, because the matching of modifiers would be affected by the magic remappings previously applied to those modifiers; probably can't to anything about that)
• tap dance (the predefined variants that just send the specified keycodes)

[osamuaoki]

I agree to take care corner cases as much as possible. At the same time, I recognize importance of keeping the code logic as simple as possible to keep it maintainable.

I think good compromise can be realized if uint16_t keycode_config(uint16_t keycode) is defined as __attribute__((weak)). (Whoever requires old behavior can define this function in their code in keyboards/... by copying the old code.)

More over, if bool process_magic(uint16_t keycode, keyrecord_t *record) is also defined as __attribute__((weak)), any more special magic mechanism addition doesn't require upstream involvement.

FYI: I may be wrong but extern keymap_config_t keymap_config; in keycode_config.c looks redundant since it is defined in keycode_config.h and keycode_config.c includes keycode_config.h. There are many similar extern declaration lines in the QMK source so these may have some reasons behind them.

[drashna]

FYI: I may be wrong but extern keymap_config_t keymap_config; in keycode_config.c looks redundant since it is defined in keycode_config.h and keycode_config.c includes keycode_config.h. There are many similar extern declaration lines in the QMK source so these may have some reasons behind them.

This is likely due to some legacy code/behavior. The same extern was all over the place in keymaps, etc. It's something that has been cleaned up in increments, so this is one of the spots where it has just been missed.

The actual declaration of the structure is in magic.c, which probably isn't the correct place for it. But for now, should be okay.

[osamuaoki]

The actual declaration of the structure is in magic.c, which probably isn't the correct place for it. But for now, should be okay.

Yes. I also found magic.c.

For me, magic is useful as a temporary change mechanism but since this involves bootmagic/magic.c, magic change is persistent over power cycle. (EEPROM is used to store keymap_config state instead of global variable in RAM.)

I created PR to add define KEYMAP_CONFIG_RAM to make magic change to be non-persistent. https://github.com/qmk/qmk_firmware/pull/18373

[lilgrassin]

Any updates on this? I've cherry-picked this PR into my local branch and have been running it on my Lulu with no problems since November, but it would be nice to get it merged officially.