zmkfirmware / zmk

ZMK Firmware Repository
https://zmk.dev/
MIT License
2.67k stars 2.74k forks source link

fix(split): Make locality work for nested behaviors #2409

Closed caksoylar closed 1 week ago

caksoylar commented 1 month ago

This is another attempt to solve #1494. My understanding is that #1630 only covers the case of macros invoking global behaviors. I incorporated the changes in it and attempted to address the existing comments.

In addition to the other PR, this should cover sticky keys, hold-taps, mod-morphs and work with source+global locality behaviors. I tested the reset behaviors with hold-taps and tap dances so far.

I am not familiar with these parts of the code; so far I stuck an extra field source wherever I saw a position and passed it around. Specifically, it is a new field in zmk_behavior_binding_event. Any recommendations are welcome on how better to do it.

TODO:

caksoylar commented 1 month ago

This is ready for review: I tested all the combinations I can. (And fixed one bug where I learned the purpose of the struct copy the hard way.)

One thing missing is sensor rotate since I don't have hardware with encoders but it is using the behavior queue just like the macros, so I assume it should be fine. I'll ask for testers on Discord just in case.

Nick-Munnich commented 1 month ago

Missing a docs adjustment to the new split keyboards page

caksoylar commented 1 month ago

@Nick-Munnich removed the note, thanks!

One thing missing is sensor rotate since I don't have hardware with encoders but it is using the behavior queue just like the macros, so I assume it should be fine.

This was naive, I looked into it and passing through the source information in the sensor events is not trivial and I don't feel confident enough to do it given I can't even test them. In https://github.com/zmkfirmware/zmk/pull/2409/commits/4e12392d918fafcc90fdff6195198f7cd9028e0a I hardcoded them to trigger on the central; compared to status quo it should fix using global behaviors like &rgb_ug, at least. I think making source local ones work can be tackled in the future.

caksoylar commented 1 month ago

I got confirmation from a user on Discord that encoders also work with a global behavior now. To sum up, this is the current state after this PR, according to my testing and others'.

Working with global and source-local behaviors:

Working with global behaviors:

I am not planning to tackle above two for source-local in this PR (see below) so it is complete from my POV.

[^1]: Making source local work with combos requires deciding the locality of combos from the set of key positions that trigger it, some discussion here [^2]: The code change required for this is separate and since I don't have the hardware to iterate on, I think it'd be better to tackle this separately

caksoylar commented 3 weeks ago

Went over the review items, squashed commits to be a bit saner and gave it a drive on my test split to check the previously tested items (no encoder, but everything else).