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

settings: Add global widget selection #1959

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

Conversation

vkareh
Copy link
Contributor

@vkareh vkareh commented Jan 10, 2024

Instead of each watch face implementing their own settings for which widgets to display, we can have a global selection of widgets. All watch faces can then determine whether it is enabled and so display it in whichever way makes sense for that face.

Current widgets supported are heart rate, step counter, and weather.

Widgets off:
InfiniSim_2024-01-10_161937
InfiniSim_2024-01-10_161850 InfiniSim_2024-01-10_161904 InfiniSim_2024-02-05_134208

Widgets on:
InfiniSim_2024-01-10_162123
InfiniSim_2024-02-05_134638 InfiniSim_2024-02-05_134518 InfiniSim_2024-02-05_134257

Copy link

github-actions bot commented Jan 10, 2024

Build size and comparison to main:

Section Size Difference
text 375948B 1828B
data 948B 0B
bss 63488B 8B

@FintasticMan FintasticMan added the enhancement Enhancement to an existing app/feature label Jan 11, 2024
Copy link
Member

@FintasticMan FintasticMan left a 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 very useful, thanks!

@vkareh vkareh force-pushed the global-widget-config branch 3 times, most recently from ca33109 to 4eeadff Compare January 16, 2024 21:49
@minacode
Copy link
Contributor

minacode commented Jan 21, 2024

I think you could use the CheckboxList class for the settings menu.

You can find it in src/displayapp/screens/CheckboxList.h.

@vkareh
Copy link
Contributor Author

vkareh commented Jan 22, 2024

@minacode the badly-named CheckboxList class only deals with single-selection settings (radio buttons)

@minacode
Copy link
Contributor

Oh, ok. Maybe we should rename it to something like RadioButtonGroup.

Do you think you could still extract an abstract component from your implementation? I could imagine that an actual checkbox list is common enough.

Maybe it doesn't make sense though. That would be ok. I am just asking.

@vkareh vkareh force-pushed the global-widget-config branch 2 times, most recently from a02aef7 to 039506d Compare January 24, 2024 15:31
@vkareh
Copy link
Contributor Author

vkareh commented Jan 25, 2024

Oh, ok. Maybe we should rename it to something like RadioButtonGroup.

No, the name is based on the lvgl library, which uses checkbox as the naming. There is no radio component in lvgl, so the checkboxlist class emulates the radio behaviour by re-theming the checkboxes and overriding the select mechanism.

Do you think you could still extract an abstract component from your implementation? I could imagine that an actual checkbox list is common enough.

I did an initial PoC, but it means that now CheckboxList needs to return arrays of settings, whether it's a checkbox or a radio. This and the additional code required to make that work makes the entire thing to complex/bloated.

Maybe it doesn't make sense though. That would be ok. I am just asking.

I can poke at it some more at another time, but likely out of scope for this PR.

@minacode
Copy link
Contributor

Ok, nice. Thanks for the detailed answer. Given that, I think your PR is fine the way it is now :)

@vkareh
Copy link
Contributor Author

vkareh commented Feb 5, 2024

I've added new commits for configurable widgets on the Analog watch face. Attached screenshots to the description on top.

@vkareh vkareh force-pushed the global-widget-config branch 3 times, most recently from d54ae36 to 168d9ca Compare February 12, 2024 15:17
@vkareh vkareh force-pushed the global-widget-config branch 2 times, most recently from 49d5ec3 to efdeebf Compare February 19, 2024 01:10
@vkareh vkareh force-pushed the global-widget-config branch 2 times, most recently from 5f365d9 to 42bc7d6 Compare August 19, 2024 17:07
Instead of each watch face implementing their own settings for which
widgets to display, we can have a global selection of widgets. All
watch faces can then determine whether it is enabled and so display it
in whichever way makes sense for that face.

Current widgets supported are heart rate, step counter, and weather.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants