-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(split): Make locality work for nested behaviors #2409
Conversation
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. |
Missing a docs adjustment to the new split keyboards page |
80e5708
to
c17579d
Compare
@Nick-Munnich removed the note, thanks!
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 4e12392 I hardcoded them to trigger on the central; compared to status quo it should fix using global behaviors like |
c17579d
to
6227bfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments after a first pass. Thanks!
bdc2610
to
0b50828
Compare
c2564d2
to
2bc4f23
Compare
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. Footnotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking with this! A few thoughts on the implementation.
4ce0453
to
8013892
Compare
Co-authored-by: Tokazio <[email protected]>
TODO: Currently the source is hardcoded to central for source local behaviors
8013892
to
22e2cb5
Compare
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking with this!
It can be enabled since locality now works with nested behaviours: zmkfirmware/zmk#2409
It can be enabled since locality now works with nested behaviours: zmkfirmware/zmk#2409
It can be enabled since locality now works with nested behaviours: zmkfirmware/zmk#2409
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 aposition
and passed it around. Specifically, it is a new field inzmk_behavior_binding_event
. Any recommendations are welcome on how better to do it.TODO: