feat: Refine behavior_binding_event, nested trans #2848
Conversation
Adds some encoder tests for rotating, and basic layers. Mock Kscans don't seem to interact nicely, slightly on the hacky side. However, better to have hacky tests than no tests.
Nick-Munnich
added
core
|
What documentation changes do you have in mind, besides updating the new behavior guide? |
| } | ||
|
|
||
| // TODO: Pass the event in as pointers, rather than copying in the struct | ||
| // Make this change as part of a breaking release |
There was a problem hiding this comment.
Isn't this passing it as a pointer already?
There was a problem hiding this comment.
Here I'm referring to the behavior_keymap_binding_pressed etc functions. We're dereferencing the event struct before passing it in for backwards compatibility reasons. That way this isn't a breaking change for external behavior modules. I think that passing the struct in as a pointer would be slightly more efficient though, so doing it as part of a breaking change would be a good idea imo.
There was a problem hiding this comment.
Got it, so the comment applies to the function calls inside. Though I don't know enough to say whether passing a pointer rather than a struct gives a non-trivial performance gain. I'd be interested to learn if e.g. @joelspadin has thoughts on it.
app/include/zmk/keymap.h
Outdated
| @@ -73,8 +74,9 @@ int zmk_keymap_save_changes(void); | |||
| int zmk_keymap_discard_changes(void); | |||
| int zmk_keymap_reset_settings(void); | |||
|
|
|||
| int zmk_keymap_position_state_changed(uint8_t source, uint32_t position, bool pressed, | |||
| int64_t timestamp); | |||
| int zmk_keymap_raise_binding_event_at_layer_idx(zmk_keymap_layer_id_t layer_id, uint8_t source, | |||
There was a problem hiding this comment.
Probably should be ..._layer_id, to disambiguate between layer_id and layer_index?
There was a problem hiding this comment.
Considering the other conversationy bits I think renaming this to ..._layer_index might be better?
There was a problem hiding this comment.
I guess that function interface will be changed to use indices anyway, so it makes sense.
There was a problem hiding this comment.
Should be fixed now. Turns out I had half the solution in the code already. I think I may have fixed a bug that currently might exist in the process: We currently loop until LAYER_ID_TO_INDEX(default_layer). I think if someone reordered their layers to put the original default layer at the top of their layers and then tried to use &trans, they would have a bad time. Might be wrong, haven't tested this.
There was a problem hiding this comment.
Thanks for the review! Regarding documentation changes, it is pretty much just the new behavior guide that I was thinking of, at least for now. Some further changes in the future might result in a nested trans making for a nice example in e.g. hold-taps, but that shouldn't be a part of this PR imo.
| } | ||
|
|
||
| // TODO: Pass the event in as pointers, rather than copying in the struct | ||
| // Make this change as part of a breaking release |
There was a problem hiding this comment.
Here I'm referring to the behavior_keymap_binding_pressed etc functions. We're dereferencing the event struct before passing it in for backwards compatibility reasons. That way this isn't a breaking change for external behavior modules. I think that passing the struct in as a pointer would be slightly more efficient though, so doing it as part of a breaking change would be a good idea imo.
| * RGB behaviors) does not modify it, e.g. if the behavior is stored in a combo_cfg | ||
| */ | ||
| struct zmk_behavior_binding binding = *(event->binding); | ||
| int err = behavior_keymap_binding_convert_central_state_dependent_params(&binding, *event); |
There was a problem hiding this comment.
I believe that L111 is making the copy, and then L112 is passing in a pointer to the copy so that the copy can be modified appropriately. We're then passing the adjusted behavior in as the first argument to the other functions, with the event having the behavior prior to any sort of adjustment.
Nick-Munnich
force-pushed
the
behavior-event
branch
2 times, most recently
from
d812522 to
4079dca
Compare
Nick-Munnich
force-pushed
the
behavior-event
branch
from
4079dca to
086cc93
Compare
This PR essentially does multiple things in one commit. These are all quite linked to one another, I wasn't quite able to untangle them from each other.
sensors.c.zmk_behavior_binding_eventso that it is actually a zmk event, i.e. is raised and managed byevent_manager.&transinside of other behaviors, getting rid ofZMK_BEHAVIOR_TRANSPARENTin the process.Taking the explanation from #573:
We currently have the following events
What is missing from this event flow are behavior bindings. This means they can not be observed like other events.
This refactor/feat will allow for much more advanced and elegant handling in the future. My goal is to use this to rework combos a bit further. Nesting
&transis also a benefit that should not be underestimated, it will make things like home-row mods much easier to set up. The sensor refactor should allow further work on that part to be simpler as well.Needs documentation and lots of testing, particularly around the encoder bits as the tests we have are very rudimentary. There are likely some stylistic bits that could use tidying up as well.
Closes: #573, #532, #2368
PR check-list