Fix mod-morph

[urob]

This is an updated version of #1114. It builds on @aumuell's masked-mods fix 241f82e. Notable differences with respect to #1114 are:

• fixes issue where modifiers in the binding are ignored by allowing masked-mods to be overwritten by implicit mods
• fixes issue where masked mods does not work when the behavior is bind to a hold-tap
• fixes issue where masked mods are unmasked before key release
• added unit tests
• added documentation
• new default: modifiers won't be passed along with the morphed binding (fixes when using mod-morph to make shift-backspace work as delete, the shift key is not 'swallowed' #683 without the need to specify masked-mods)

Important change: the config option has been changed from masked_mods to keep-mods

[urob]

Quick ping @petejohanson: I think this is ready for review if you get the chance. Below a quick summary of the technical implementation to, hopefully, facilitate the review process.

At its core, the PR does a bit of refactoring of how mods are handled in hid.c. In addition to explicit_modifiers, it adds two new persistent state variables: implicit_modifiers and masked_modifiers.

Any change in modifiers; i.e., any call to one of the following functions:

• zmk_hid_register_mod()
• zmk_hid_unregister_mod()
• zmk_hid_implicit_modifiers_press()
• zmk_hid_implicit_modifiers_release()
• zmk_hid_masked_modifiers_set()
• zmk_hid_masked_modifiers_clear()

updates the corresponding state variable and then calls SET_MODIFIERS(). The SET_MODIFIERS() macro in turn sets the modifiers as follows:

keyboard_report.body.modifiers = (mods & ~masked_modifiers) | implicit_modifiers;

Using the refactored hid functions, behavior_mod_morph.c updates the state of masked_modifiers when a mod-morph is triggered (by default to the mods that trigger it). And after the mod-morph is done, it clears masked_modifiers.

[petejohanson]

Code changes all seem reasonable, just the one point of discussion around the default behavior being changed by this PR.

[petejohanson]

This fallback makes sense, in theory, but changes existing default mod morph behavior....

[petejohanson]

Doesn't this mean the default behavior for mod morph is changed as part of this PR?

Do we think there's any reason to be concerned by that?

[urob]

My reading was that most considered the old default a bug (see #683). Personally, I could go either way. But I thought that the proposed default may simplify life for the typical use-case of mod-morph?

[petejohanson]

Plenty of voices there that would seem to suggest a new default behavior is fine... Wonder how many folks use it today is as and it works well?

[urob]

Also, one can always enforce that a mod gets sent along by adding it to the binding as implicit mod. Hopefully that'll be intuitive enough so that most people won't even need to adjust the masked-mods property?

[petejohanson]

Yeah, I am leaning towards this being an "in the greater good" breaking change in behavior, given the fact the original versions sending along the mods was "odd" in the first place.

@joelspadin @Nicell @okke-formsma Thoughts?

[snoyer]

Re: masked-mods vs keep-mods, there's a minor restriction associated with the latter in that one wouldn't be able to mask any mods other than the ones triggering the mod-morph. Not sure how much that's a restriction in practice, but it may limit some creative uses?

That's a good call out. I think potentially useful to support that for cases we're not imaging yet for this.

I hadn't thought that far.
My issue was that masked-mods looked like an implementation detail leaking into the API for no good reason.
"Future use-cases" could be reason enough to go with it; and with the with the new default I assume it wouldn't show up in the vast majority of cases anyway.

[caksoylar]

I agree with snoyer, in addition if we are changing the default then it's also fair to break not-yet-imagined use cases that masked-mods might allow. Making it more intuitive for users should take priority here IMO.

[petejohanson]

Thinking about this more... I think I'm now leaning towards keep-mods for this. If we really need to filter other mods not specified in the morph later, we can solve that another way.

[urob]

The main use I could imagine is to set up a "dummy-mod-morph" which doesn't actually do anything other than masking mods. Currently, this is the only way to access the backend hid-functions zmk_hid_masked_modifiers_set and zmk_hid_masked_modifiers_clear from the user side. But I agree that if there is demand for that, it would probably better to create a new "negative-mods" behavior that more directly exposes the masked_mods functionality.

I just pushed a new commit that switches from masked-mods to keep-mods. I tried out a couple of edge cases and it all works well on my side.

[joelspadin]

Having a behavior to set/clear masked mods would also be useful for sending strings, as you could lock out mods before sending the string and restore them after. That said, if the string is too long and it overflows the behavior queue, the restore behavior would never be sent and you wouldn't be able to use those mods anymore, so that wouldn't be great. Maybe instead we could add an optional mask property to specific behaviors, so for example you could add a &kp variant that masks mods and use that instead of &kp when sending strings.

[okke-formsma]

lgtm

[caksoylar]

Looks good to me from a docs perspective.

[petejohanson]

Thanks for all the work on this!