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

Add Timer's Time Remaining to StatusIcons #1967

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

Conversation

JustScott
Copy link
Contributor

@JustScott JustScott commented Jan 14, 2024

(look here for up-to-date images, as the ones below no longer represent the changes in this PR)

Adds a live output of the timer's time remaining, along with an hourGlass symbol to the left. Both of these are placed above the current time and are the same color as the date as to not draw attention away from the current time. The icon and the time remaining are both set to hidden if the timer isn't running.

timer_set_to_show
timer_set_to_show2
timer_set_to_hidden

Looking for opinions on possible changes to the timer's and symbol's font color and/or position.

Copy link

github-actions bot commented Jan 14, 2024

Build size and comparison to main:

Section Size Difference
text 373536B 512B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

@FintasticMan FintasticMan added enhancement Enhancement to an existing app/feature UI/UX User interface/User experience labels Jan 17, 2024
@JustScott JustScott force-pushed the digital_watchface_timer branch from 612500d to 5ef3a85 Compare January 17, 2024 21:51
@vkareh
Copy link
Contributor

vkareh commented Jan 20, 2024

@JustScott

Looking for opinions on possible changes to the timer's and symbol's font color and/or position.

I like this feature a lot, as I use the timer regularly. Since you asked for opinions, how about making it part of the StatusIcons? This way you can see it in several other screens. Although maybe this makes it look too busy. Not sure, but just a suggestion.

@JustScott JustScott force-pushed the digital_watchface_timer branch from 5ef3a85 to 071aebf Compare February 1, 2024 14:44
@JustScott
Copy link
Contributor Author

JustScott commented Feb 1, 2024

@vkareh Here's how the timer would look as a StatusIcon:
timer_in_watchface
timer_in_app_selection

There are currently 2 issues with this:

  1. The status icons are only set to update when switching between screens in the Tile.cpp file, which means the time remaining will only update once each time you swipe between screens. This could be changed to update more often like is done on the clock faces, but I'm unsure of how this would effect power consumption.
  2. The timer is shown on the digital watch face screen both in the status icons and above the current time. We could have just avoided placing it in the watch face itself, and only placed it in the status icons with the change suggested in issue 1 above... but this would now take the same position as the new weather widget. So the obvious solution is just to detect if the current open app is the digital watch face, and set the timer to hidden.

I'll implement these fixes/changes, but first I'd like some feedback on whether this is something people actually want. I personally think it could be nice, but honestly I don't see myself needing to know how much time is remaining on a timer in the second it takes me to switch between apps (but some people may).

@vkareh
Copy link
Contributor

vkareh commented Feb 1, 2024

The timer is shown on the digital watch face screen both in the status icons and above the current time. We could have just avoided placing it in the watch face itself, and only placed it in the status icons with the change suggested in issue 1 above... but this would now take the same position as the new weather widget. So the obvious solution is just to detect if the current open app is the digital watch face, and set the timer to hidden.

I've been using a version of the digital face with the weather within the face itself:
InfiniSim_2024-02-01_131052

I think this change would go nicely with the timer in the status bar.

The status icons are only set to update when switching between screens in the Tile.cpp file, which means the time remaining will only update once each time you swipe between screens. This could be changed to update more often like is done on the clock faces, but I'm unsure of how this would effect power consumption.

This is a problem. It could be solved by adding decreasing the task refresh period like this (not tested):

diff --git a/src/displayapp/screens/Tile.cpp b/src/displayapp/screens/Tile.cpp
index 7c585a4b..ffdb9f44 100644
--- a/src/displayapp/screens/Tile.cpp
+++ b/src/displayapp/screens/Tile.cpp
@@ -87,7 +87,7 @@ Tile::Tile(uint8_t screenID,
   btnm1->user_data = this;
   lv_obj_set_event_cb(btnm1, event_handler);
 
-  taskUpdate = lv_task_create(lv_update_task, 5000, LV_TASK_PRIO_MID, this);
+  taskUpdate = lv_task_create(lv_update_task, 500, LV_TASK_PRIO_MID, this);
 
   UpdateScreen();
 }

500ms lets you see the timer seconds real time and it shouldn't really affect power consumption that much, I think, since the status icons only show up in screens that support auto-sleep of the display.

@JustScott
Copy link
Contributor Author

@vkareh Ah I missed the part where the status icons update every 5 seconds. I think moving this timer to the status icons would be the best solution if the weather icon is moved as shown in your image above. I'll try this out.

@JustScott
Copy link
Contributor Author

Alright I moved the timer from right above the time on the digital watchface, to the StatusIcons 'area'.

Also, I'm VERY new to C++, and was just following what looked to be the right way to pass this timer controller around, but I feel it got added to too many places... so feedback on whether the code is correct or not would be much appreciated. To me it seems like it would be best if I could just define the timer controller within StatusIcons.cpp instead of having to pass it to every StatusIcons class call.

@vkareh
Copy link
Contributor

vkareh commented Feb 1, 2024

I created a PR to move the weather widget: #1998

@JustScott JustScott force-pushed the digital_watchface_timer branch from 8e55197 to a36cde7 Compare February 20, 2024 22:14
@JustScott JustScott changed the title Add Timer's Time Remaining to Digitial Watchface Add Timer's Time Remaining to StatusIcons Feb 21, 2024
@JustScott JustScott force-pushed the digital_watchface_timer branch from a36cde7 to 3fa9dbb Compare March 15, 2024 03:48
@JustScott JustScott force-pushed the digital_watchface_timer branch 2 times, most recently from e289435 to da482ba Compare October 9, 2024 18:50
@JustScott JustScott force-pushed the digital_watchface_timer branch 2 times, most recently from 217e5fc to 8681c2a Compare December 21, 2024 04:49
@JustScott
Copy link
Contributor Author

I've applied the changes from all of vkareh's reviews, and have been using this without issue for quite a while now. Here's some images of how it looks with the digital watch face weather changes. @JF002 would this be something you'd be interested in including in version 1.16.0?

InfiniSim_2024-12-20_225411
InfiniSim_2024-12-20_225412

@JustScott JustScott force-pushed the digital_watchface_timer branch 2 times, most recently from 20c10c2 to 2580264 Compare January 18, 2025 19:47
@JustScott
Copy link
Contributor Author

JustScott commented Jan 18, 2025

Adjusted the code to work with the new alarm icon in the status icons from #1884. To prevent crowding I moved the timer over a hair:

InfiniSim_2025-01-18_134142
InfiniSim_2025-01-18_134501

It doesn't align well with the weather in the digital watch face... but I didn't think it aligned all that well before either:
InfiniSim_2025-01-18_135411

@JustScott JustScott force-pushed the digital_watchface_timer branch from 2580264 to f4b341e Compare January 31, 2025 04:51
@JustScott
Copy link
Contributor Author

JustScott commented Jan 31, 2025

I've moved the timeRemaining and timerIcon to there own timerContainer, and moved that container into the primary container to allow them to dynamically align with the other icons, while being able to place them closer together than the others. When more than two icons (exluding batteryIcon) are made visible, the timerContainer is set to hidden and a soloTimerIcon is made visible. Replacing the live timer with a simple icon allows the user to avoid any overlapping with the time in the top left when many icons are visible, while still letting them know a timer is active... and when a few icons go back to being hidden, the live timer will be made visible again.

12h_with_solo_timer_icon
12h_with_time_remaining

I feel the space between the time and the AM and PM is a bit unecessary and ugly, this is how it would look without the space. I can change it in this PR, or it can be left to another PR... just thought I would show the difference.
12h_no_space_with_time_remaining

digital_watchface_solo_timer_icon
digital_watchface_time_remaining

There could be another icon allowed with the 24 hour time format, but this would require bringing the settingsController in... I'm not opposed to it, but it seems unecessary
24H_icons_with_time_remaining
24H_no_icons_just_timer

@mark9064
Copy link
Member

Maybe to save space the timer could shorten to XXm/XXs?

@JustScott
Copy link
Contributor Author

Maybe to save space the timer could shorten to XXm/XXs?

Incredible timing, I was just working on something like this.

@JustScott JustScott force-pushed the digital_watchface_timer branch from be6bd90 to 18855db Compare February 14, 2025 01:42
@JustScott
Copy link
Contributor Author

Minutes are now only shown if greate than 0. Another icon is also allowed if the user is using 24H time instead of 12H time.

mintues_12h
seconds_12h
minutes_24h
seconds_24h

When a timer is active the time remaining will be displayed in the
StatusIcons bar along with an hour glass symbol, and will be set to
hidden when the timer's off.
This commit moves the timeRemaining and timerIcon into a timerContainer
nested within the primary container. This ensures the timeRemaining and
timerIcon stay closer together than the other items in the primary container,
while still dynamically aligning with the rest of them.  When two or more
icons (excluding batteryIcon) are present, the timeRemaining and timerIcon
are replaced by a soloTimerIcon to prevent overlap with the time in the top
left corner.
Loop through the children of `container` to count visible icons instead
of hardcoding each icon. Adding icons to the container no longer requires
adding another hardcoded icon to the active Icon counter logic.
Removes the minute counter from the timer when under a minute to allow
displaying the remaining second count with more icons.
@JustScott JustScott force-pushed the digital_watchface_timer branch from 18855db to 24cd41f Compare February 14, 2025 01:47
@vkareh
Copy link
Contributor

vkareh commented Feb 14, 2025

If you want to be more aggressive, I'd even remove the colon : when it's just seconds remaining. Seems unnecessary and shaves off a whole character.

This looks great, BTW, and I use this feature every day. Looking forward to rebasing the new look into my daily-driver branch.

@JustScott
Copy link
Contributor Author

JustScott commented Feb 14, 2025

If you want to be more aggressive, I'd even remove the colon : when it's just seconds remaining. Seems unnecessary and shaves off a whole character.

Removed the colon, looks better to me:
no_colon

@JustScott
Copy link
Contributor Author

Reduced timer letter spacing to save space

Allows up to four icons (using an example stopWatch icons for the fourth icon):
normal_letter_spacing
with_reduced_letter_spacing

Another example with hour support from PR #2243:
hour_without_reduced_letter_spacing
hour_with_reduced_letter_spacing

@minacode
Copy link
Contributor

Most aggressive would be to replace the current time with the time remaining for as long as the timer is running 😄

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 UI/UX User interface/User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants