-
-
Notifications
You must be signed in to change notification settings - Fork 79
Show Layer-Tap/Mod-Tap information on keys #135
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for zmk-studio ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
This looks very similar to https://discordapp.com/channels/719497620560543766/722441520741089317/1293320175482966037, which I quite like the look of. If you're able to adjust it to match more closely, that would be great. Does this work with all hold-taps? If not, it should be extended as such. |
| behavior: string, | ||
| binding: KeyBinding, | ||
| ): JSX.Element | JSX.Element[] => { | ||
| if (behavior === "Layer-Tap") { |
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.
Those if should probably be improved, not sure if we should add the "behavior type" and/or binding-cells. Could maybe be a followup?
86d2c86 to
e214945
Compare
|
I agree that it would be nice to see the layer and mod tap info on the keys, but I think we should start out with a simpler visualisation like This is personal preference though. I feel its simpler to review, and improve over time, when making as few changes as possible. Also the linting changes you (or your editor) has done, makes it more difficult to review, and should be done in a seperate PR. The actual important changes are confined to 1 or 2 files. |
petejohanson
left a comment
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 the PR! The main concern for me is the trigger/logic used to determine when to render these differently.
| ]; | ||
| } | ||
|
|
||
| if (behavior === "Mod-Tap") { |
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.
We really can't do this. We need to be inspecting the metadata information to determine what to do here. Because:
- Users might have multiple mod taps with different names,
- Users might translate the display names to their language in the devicetree.
- General flexibility for custom behavior support in studio.
So... this is great to work on... and it must be metadata driven. For instance, we might choose this rendering style if both param1 and param2 are of parameter metadata type "HID usage".
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.
What do you think about the latest changes? Like that we don't depend on the name. It's not perfect but it's more generic.
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.
ping @petejohanson
| const param1IsHidUsage = isHidUsageParameter(binding.param1, behavior.metadata, layerIds); | ||
| const param2IsHidUsage = isHidUsageParameter(binding.param2, behavior.metadata, layerIds); | ||
| const param1IsLayerId = isLayerIdParameter(binding.param1, behavior.metadata, layerIds); | ||
| const param2IsLayerId = isLayerIdParameter(binding.param2, behavior.metadata, layerIds); |
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.
Chiming in because I think this is a cool change and I hope it hits studio main soon:
This variable here param2IsLayerId is never used, so CI is failing.
|
Any updates on this? |
|
I wish sorry, I pinged the devs a few times on Discord, I don't know what to do to get it reviewed again/merged. I lost motivation. |
|
Last I checked, Pete is planning on doing more with Studio after we bump Zephyr to 4.1. |
|
Is there still hope to help me review/merge this MR? |
|
I believe this PR as-is is seen as too specific for the current state of Studio. See https://discord.com/channels/719497620560543766/719909884769992755/1435008086329917521 for a recent discussion we had - I believe that a generic "display both params of behaviour with two params" needs to come first, then figure out how to make that look good, then do edge cases for things like layer-tap. I do think this PR could be modified to cover the more general case, but I would understand (yet feel saddened if you feel too demotivated to do said work. |
So this is basically what @danielsvane suggested above. If this change would be green lit, I'd be happy to take a stab at it |
|
I'd love to see improvements here, yes, that align with the metadata approach so we can have a sane display for any possible parameter combination, with enhanced/imporved display for specific pairs of parameter types. Important note: The calculation of the second parameter type can depend on the given first parameter value, so the calculation for what that second type is can be non-trivial. We do that right now for the behavior parameter picker here, if you need to see that in action: https://github.com/zmkfirmware/zmk-studio/blob/main/src/behaviors/BehaviorParametersPicker.tsx |
|
Ok, thank you guys for the quick answers, I'll try to update this PR one of those days and make it more simple. |
|
Still a WIP but it's now closer to the behavior param picker
What would be good, I think, is to send the behavior name in the protocol. |
awkannan
left a comment
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.
I think this is a great start!
| const matchingSet = findMatchingParameterSet(binding.param1, behavior.metadata, layerIds); | ||
|
|
||
| // If both parameters are zero | ||
| if (binding.param1 === 0 && binding.param2 === 0) { |
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.
this could be a valid configuration for some behaviors - so I don't think we can return an empty div here
|
|
||
| // Both parameters present and should be displayed | ||
| if (param1Display && param2Display) { | ||
| return [ |
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.
The ordering here is tricky and I think could be up for discussion, but imo is fine for now
| param1?: number, | ||
| param2?: number | ||
| ): boolean { | ||
| if ( |
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.
I'd add a few comments about what each check does.
For example - I'm personally a bit lost as to why we are doing a param1 === 0 check here.
For some behaviors, like bluetooth, param1 could be 0 to indicate clear current profile
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.
I agree with you, but I didn't touch the logic, this function was moved. I reverted it to simplify the diff.
| } | ||
|
|
||
| // Get displays for both parameters | ||
| const param1Display = binding.param1 !== 0 ? |
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.
0 is a valid value for a param, i don't think this should be gated behind that
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.
fixed
| getParameterDisplay(binding.param1, behavior.metadata.flatMap(m => m.param1), layers) : | ||
| null; | ||
|
|
||
| const param2Display = binding.param2 !== 0 && matchingSet ? |
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.
same as above
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.
fixed
|
If we go that route @awkannan I think it would be better to open a separate PR that only change the design. Right now we show this info when we have the cursor "on hover". I would gladly take care of the implementation, but to avoid back-and-forth it would help to have a specific design/idea/goal in mind before I start. And thanks for your review! I'll check this soon |
|
Agreed - I think that should be a separate PR! |
|
This would close #147 which would be great 😍 |
|
Since #141 was merged, there are some merge conflicts now. |
c0f3ce6 to
7b9bf07
Compare
69e4140 to
8926887
Compare








Show "Layer-Tap" information on the keys, before it was empty, we now see which layer will be enabled on hold and which key will be send on tap.
Instead of the layer logo I was hesitant to add "Hold" to show that this behavior is triggered when it's hold, but it could be done as a future improvement.
Before:
After: