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

Improved the Terminal Watchfaces UI #2135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JustScott
Copy link
Contributor

These changes were originally proposed in PR #2001 along with the addition of weather data on it's own line, but it was suggested they be seperated. This PR is just the UI improvements without the weather, and #2001 will add the weather.

  • Reorder code to match the widgets order in the UI.

  • Use InfintimeTheme Colors instead of hardcoded hex values

  • Added a new InfinitimeTheme color: gray, using it to turn certain values gray when they contain no data

  • Implement @vkareh's variable battery icon color to the battery percentage text.

  • Replaced the 'You have mail.' notification message with the message '[1]+ Notify' to better fit the terminal lore.

InfiniSim_2024-10-03_155938
InfiniSim_2024-10-03_155950

Copy link

github-actions bot commented Oct 3, 2024

Build size and comparison to main:

Section Size Difference
text 374944B 416B
data 948B 0B
bss 63488B 0B

+ Reorder code to match the widgets order in the UI.

+ Use InfintimeTheme Colors instead of hardcoded hex values

+ Added a new InfinitimeTheme color: gray, using it to turn certain
  values gray when they contain no data

+ Implement @vkareh's [variable battery icon](InfiniTimeOrg#1964)
  color to the battery percentage text.

+ Replaced the 'You have mail.' notification message with the message
  '[1]+ Notify' to better fit the terminal lore.
@JustScott JustScott force-pushed the improve_terminal_watchface_ui branch from c312a6c to d2382e1 Compare October 4, 2024 15:03
Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

I don't use terminal so no strong opinion on visual style but looks nice to me

Maybe we should use this opportunity to fix the badly named fields (they should be camelCase)? But that could also be another PR, definitely not a blocker

// between red and green, thus avoiding the darker RGB on medium battery
// charges and giving us a much nicer color range.
uint8_t hue = batteryPercentRemaining.Get() * 120 / 100;
lv_obj_set_style_local_text_color(batteryValue, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hsv_to_rgb(hue, 100, 100));
Copy link
Member

Choose a reason for hiding this comment

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

I think colours here are nice, but this shows yellow at 50% right? I think it would make more sense to follow a curve closer to the current battery icon (yellow at 15, red at 5). Not sure what function would represent this well but coming up with something in Desmos/similar would be easy

Also if this is to be used in other places as well, it should probably be moved out so other screens can use it. I don't know if that's the goal here though

Copy link
Member

Choose a reason for hiding this comment

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

I see this is from #1964 , so I think it definitely makes sense to move the colour calculation algorithm to somewhere where other apps can use it

heartbeat = heartRateController.HeartRate();
heartbeatRunning = heartRateController.State() != Controllers::HeartRateController::States::Stopped;
if (heartbeat.IsUpdated() || heartbeatRunning.IsUpdated()) {
if (heartbeatRunning.Get()) {
lv_label_set_text_fmt(heartbeatValue, "[L_HR]#ee3311 %d bpm#", heartbeat.Get());

Copy link
Member

Choose a reason for hiding this comment

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

Newline

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.

2 participants