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): Set mono theme and add invert config #1754

Merged
merged 3 commits into from
May 22, 2023

Conversation

caksoylar
Copy link
Contributor

@caksoylar caksoylar commented Apr 14, 2023

Update

This PR sets CONFIG_LV_USE_THEME_MONO by default when ZMK_DISPLAY is enabled, so that small font selection used in the built-in status screen widgets is no longer ineffective.

It also adds a new Kconfig ZMK_DISPLAY_INVERT to invert the colors used in the display from black-on-white to white-on-black, by setting the appropriate value in mono theme initialization.

Original

LVGL mono theme was getting selected by ZMK_DISPLAY but not actually used outside of the corneish_zen configuration. This change sets it by default if OLEDs or ls0xx memory displays like nice!view are used. This change could use some testing to make sure it doesn't break on OLEDs, nice!views, Adafruit memory displays etc. (Unfortunately I don't have any at the moment, tested on native_posix+SDL setup.)

Given that the theme is set correctly, we can also add an invert option by setting the dark_bg parameter of lv_theme_mono_init accordingly. The config is currently named CONFIG_ZMK_DISPLAY_INVERT but maybe it should be CONFIG_ZMK_MONO_THEME_INVERT or something else? Also it is possible that there is a better way to make this change.

@caksoylar
Copy link
Contributor Author

caksoylar commented Apr 14, 2023

One question: Should we default to LV_USE_THEME_MONO unconditionally? Right now it seems to be necessary for setting the small font via CONFIG_ZMK_LV_FONT_DEFAULT_SMALL, since it is set the ifdef block in main.c line 97. I think there might be other places where the monochrome display is assumed inherently.

In that case it can be added as select LV_USE_THEME_MONO under config ZMK_DISPLAY. If that's a good idea, I can open a separate PR for that as a fix.

Copy link

@sfcgeorge sfcgeorge left a comment

Choose a reason for hiding this comment

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

Reviewing just to say I've been using this for a week and it's working as expected. Is there anything left to do to take this off Draft and get wider review?

@caksoylar
Copy link
Contributor Author

On my end, the question on the previous comment is the main thing I want to clear up to figure out the best approach.

Copy link
Collaborator

@joelspadin joelspadin left a comment

Choose a reason for hiding this comment

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

Working for me.

The config is currently named CONFIG_ZMK_DISPLAY_INVERT but maybe it should be CONFIG_ZMK_MONO_THEME_INVERT or something else?

I think what you have is fine unless mono is the only theme that will ever support being inverted.

Should we default to LV_USE_THEME_MONO unconditionally?

This seems like it would be an improvement to me, so long as it remains possible to override it to another theme. All of the most common displays to use with keyboards are monochrome.

app/src/display/main.c Outdated Show resolved Hide resolved
app/src/display/Kconfig Outdated Show resolved Hide resolved
@caksoylar caksoylar marked this pull request as ready for review May 14, 2023 19:23
@caksoylar
Copy link
Contributor Author

Thanks for testing and the review @joelspadin, I changed to use the mono theme by default (using imply, so can be overridden) and simplified the init like you suggested. I also split it to two commits accordingly; I can split the PR into two if desired as well.

@caksoylar caksoylar force-pushed the theme-invert branch 2 times, most recently from eb0bea6 to 37bd06b Compare May 14, 2023 21:11
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.

Just one minor code style comment. Otherwise LGTM. Thanks!

app/src/display/main.c Outdated Show resolved Hide resolved
app/src/display/Kconfig Show resolved Hide resolved
All displays currently used with ZMK are monochrome so it makes sense
to enable the mono theme by default, which can be disabled by the user
since we use the "imply" statement.

Without this theme setting, the small font size selection for widgets
at the bottom of the stock status screen does not work.
@caksoylar
Copy link
Contributor Author

caksoylar commented May 20, 2023

Looks like some conditional layers tests are failing but I couldn't reproduce locally 😕

Edit: Reruns after a force push seem to be succeeding OK.

Add CONFIG_ZMK_DISPLAY_INVERT Kconfig to invert colors
(black-on-white to white-on-black) on monochrome screens.
Currently applies only if CONFIG_LV_USE_THEME_MONO is selected,
which is the default unless user overrides it.
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.

Thanks!

@petejohanson petejohanson merged commit 864394b into zmkfirmware:main May 22, 2023
43 checks passed
@caksoylar caksoylar deleted the theme-invert branch May 22, 2023 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants