Skip to content
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

feat(display): Add modifiers widget #1556

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

caksoylar
Copy link
Contributor

@caksoylar caksoylar commented Nov 28, 2022

This is a barebones modifiers widget for showing the status of active modifiers on the bottom right of the status screen (thus it should not be enabled together with the WPM widget). It only tracks explicit modifiers (is showing implicit modifiers desirable?) but it seems to work well with sticky keys so it should be useful for tracking their state.

The widget can be enabled with CONFIG_ZMK_WIDGET_MODS_STATUS=y in your .conf file. You can use CONFIG_ZMK_WIDGET_MODS_STATUS_CHARACTERS to change the characters displayed for each modifier (one char each, default is "CSAG").

Since Zephyr 3.2 has some LVGL changes affecting widgets I'd happy to revisit once it is merged in as well. This has been completed.

Small demo:
mods

@duckyb
Copy link
Contributor

duckyb commented Nov 29, 2022

I tested the current version on real hardware and it's working as intended.

@caksoylar
Copy link
Contributor Author

Thanks for testing! I'll publish it to mark as ready for review then.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts. Thanks for working on this!

app/src/display/widgets/mods_status.c Outdated Show resolved Hide resolved
app/src/display/widgets/mods_status.c Outdated Show resolved Hide resolved
@petejohanson
Copy link
Contributor

Does it make sense to keep each mod in a static location instead of shifting in from the right?

@caksoylar
Copy link
Contributor Author

caksoylar commented Dec 13, 2022

Thanks for the review!

Does it make sense to keep each mod in a static location instead of shifting in from the right?

That was my initial plan but I didn't find a good way to implement it. The font is not monospaced so using a single label with placeholders like " " for inactive mods doesn't guarantee fixed positioning. We could have 4 separate labels and position them manually but the correct positioning could depend on font and display size which doesn't feel robust. If you have any ideas I'd be happy to hear them.

@petejohanson
Copy link
Contributor

Thanks for the review!

Does it make sense to keep each mod in a static location instead of shifting in from the right?

That was my initial plan but I didn't find a good way to implement it. The font is not monospaced so using a single label with placeholders like " " for inactive mods doesn't guarantee fixed positioning. We could have 4 separate labels and position them manually but the correct positioning could depend on font and display size which doesn't feel robust. If you have any ideas I'd be happy to hear them.

Hmm.... All painful points. I think to do it right will need to wait on https://docs.lvgl.io/master/layouts/flex.html?highlight=flexbox when we get the Zephyr 3.2 update in. Put each label as a child of a container object using flex layout... and it might just work?

@caksoylar
Copy link
Contributor Author

Thanks for the review!

Does it make sense to keep each mod in a static location instead of shifting in from the right?

That was my initial plan but I didn't find a good way to implement it. The font is not monospaced so using a single label with placeholders like " " for inactive mods doesn't guarantee fixed positioning. We could have 4 separate labels and position them manually but the correct positioning could depend on font and display size which doesn't feel robust. If you have any ideas I'd be happy to hear them.

Hmm.... All painful points. I think to do it right will need to wait on https://docs.lvgl.io/master/layouts/flex.html?highlight=flexbox when we get the Zephyr 3.2 update in. Put each label as a child of a container object using flex layout... and it might just work?

Thanks for pointing that out -- I'll definitely experiment with that when I get a chance.

@idesignstuff
Copy link

I tested the current version on real hardware and it's working as intended.

Where can I find directions for testing a pr like this on my hardware?

@duckyb
Copy link
Contributor

duckyb commented Feb 12, 2023

I tested the current version on real hardware and it's working as intended.

Where can I find directions for testing a pr like this on my hardware?

Edit your west.yml like so: https://github.com/duckyb/zmk-urchin/blob/b97f8c669eb78be670b0254605888b3322f76639/config/west.yml

@idesignstuff
Copy link

idesignstuff commented Feb 12, 2023 via email

@idesignstuff
Copy link

So, is it more complicated to bring in more than one of these? I've already got one PR for split encoders. Can this one be added, or is it one at a time?

`FATAL ERROR: Malformed manifest file: /__w/Klein-zmk/Klein-zmk/config/west.yml
Schema file: /usr/local/lib/python3.8/dist-packages/west/manifest-schema.yml
Hint: manifest:
remotes:
# zmk official
- name: zmkfirmware
url-base: https://github.com/zmkfirmware
# for dual encoders
- name: infused-kim
url-base: https://github.com/infused-kim
# bravekarma
- name: caksoylar
url-base: https://github.com/caksoylar
projects:

  • name: zmk
    remote: infused-kim
    revision: my-changes/split-encoder
    import: app/west.yml
    - name: zmk
    remote: zmkfirmware
    revision: main
    import: app/west.yml
    self:
    path: config`

@duckyb
Copy link
Contributor

duckyb commented Feb 12, 2023

So, is it more complicated to bring in more than one of these? I've already got one PR for split encoders. Can this one be added, or is it one at a time?

`FATAL ERROR: Malformed manifest file: /__w/Klein-zmk/Klein-zmk/config/west.yml Schema file: /usr/local/lib/python3.8/dist-packages/west/manifest-schema.yml Hint: manifest: remotes: # zmk official - name: zmkfirmware url-base: https://github.com/zmkfirmware # for dual encoders - name: infused-kim url-base: https://github.com/infused-kim # bravekarma - name: caksoylar url-base: https://github.com/caksoylar projects:

* name: zmk
  remote: infused-kim
  revision: my-changes/split-encoder
  import: app/west.yml
  - name: zmk
  remote: zmkfirmware
  revision: main
  import: app/west.yml
  self:
  path: config`

Make you own fork with all the changes you want and add it to west.yml

@infused-kim
Copy link
Contributor

Great feature, thank you. One thing to consider: Users may also want to display caps lock and caps word in the same widget.

To accommodate that, you might need more than one character and separator after all.

@duckyb
Copy link
Contributor

duckyb commented Mar 7, 2023

Great feature, thank you. One thing to consider: Users may also want to display caps lock and caps word in the same widget.

To accommodate that, you might need more than one character and separator after all.

Is caps word a native modifier? If it isn't I don't think it would make sense to add it, however good point about caps lock, and also num lock could be an interesting addition.

@infused-kim
Copy link
Contributor

Is caps word a native modifier? If it isn't I don't think it would make sense to add it, however good point about caps lock, and also num lock could be an interesting addition.

The current implementation uses shift under the hood, but hopefully #1485 will be merged soon which will implement caps word by toggling capslock.

Despite that, I still think there is value in seeing whether capslock or capsword is enabled and I think it's better to have it all in one widget than to force the user to use one for modifiers and one for locks.

@caksoylar
Copy link
Contributor Author

Those two would indeed be very useful to have. Caps lock and other locks would ideally need #999 to get the status of the lock from the computer properly. Caps word is internal to the keyboard so that's easier but it doesn't expose an event to subscribe to or a function to check its status, as far as I can tell? I think it would be good if the basic modifier functionality gets merged first, then can be extended to these over time.

When I get a chance to update this, my plan in order of priority is:

  • Add a Kconfig to customize modifier letters
  • Adapt to Zephyr 3.2 when it gets merged
  • Experiment with flex layouts in the new LVGL to fix positioning

@caksoylar caksoylar force-pushed the mods-widget branch 2 times, most recently from b9ea8b8 to a452039 Compare May 12, 2023 20:33
@caksoylar caksoylar mentioned this pull request May 12, 2023
@caksoylar caksoylar force-pushed the mods-widget branch 3 times, most recently from 49cc132 to 9924e68 Compare May 12, 2023 21:48
@mike1808
Copy link

@caksoylar sorry for the ping. Are you planning to bringing this to finish line with the current capabilities?

@caksoylar
Copy link
Contributor Author

Hey, thanks for the reminder. I will rebase this in the next day or two and test locally, I think it should be working OK. I actually implemented the config option and ported to Zephyr 3.2 sometime ago. Still haven't checked out "Experiment with flex layouts in the new LVGL to fix positioning" item, I'll do that if I find some time.

@caksoylar caksoylar requested a review from a team as a code owner November 23, 2023 07:23
@caksoylar caksoylar removed the request for review from a team November 23, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
displays enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants