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 1 commit into
base: main
Choose a base branch
from

Conversation

JustScott
Copy link
Contributor

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 374784B 256B
data 948B 0B
bss 63488B 0B

@FintasticMan FintasticMan added enhancement Enhancement to an existing app/feature UI/UX User interface/User experience labels Jan 17, 2024
@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
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

@@ -13,7 +13,7 @@ namespace Pinetime {
namespace Widgets {
class StatusIcons {
public:
StatusIcons(const Controllers::Battery& batteryController, const Controllers::Ble& bleController);
StatusIcons(const Controllers::Battery& batteryController, const Controllers::Ble& bleController, Controllers::Timer& timer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be const Controllers::Timer& timer to keep it consistent.

@@ -29,7 +29,8 @@ namespace Pinetime {
const Controllers::Battery& batteryController,
const Controllers::Ble& bleController,
Controllers::DateTime& dateTimeController,
std::array<Applications, 6>& applications);
std::array<Applications, 6>& applications,
Controllers::Timer& timer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest putting this below the bleController to keep it consistent with all other controllers used by the StatusIcon.

@@ -19,6 +19,7 @@ namespace Pinetime {
const Pinetime::Controllers::Battery& batteryController,
const Pinetime::Controllers::Ble& bleController,
Controllers::DateTime& dateTimeController,
Pinetime::Controllers::Timer& timer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this under bleController (all statusicon controllers seem to be always grouped together, they're also all const).

@JustScott JustScott changed the title Add Timer's Time Remaining to Digitial Watchface Add Timer's Time Remaining to StatusIcons Feb 21, 2024
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.
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.

3 participants