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

Timer: Remember last timer setting #2013

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

Conversation

tausen
Copy link

@tausen tausen commented Feb 11, 2024

Remember last duration used in Timer app. Reset to zero with long press on start button.

Fixes #1991

Tested in InfiniSim and on hardware, seems to work as desired:

InfiniSim_2024-02-11_202413

A few notes to consider:

  • There is no difference between restarting a paused timer and starting a new one, so briefly pausing will also update the suggested new duration
  • Storing minutes and seconds in two simple ints rather than a std::chrono::seconds is probably lighter (less memory, fewer divisions/modulo 60), but this is probably more readable? I have no strong opinions here.
  • Probably requires some changes once timer: Add ringing and counter #1971 is merged

Copy link

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

Thanks for this 1st contribution @tausen !

There is no difference between restarting a paused timer and starting a new one, so briefly pausing will also update the suggested new duration

I'm wondering if this would be the expected behavior by the users. Shouldn't the timer be reset to the last settings instead of the last paused value? This is an open question, I'm not really sure about this.

@@ -6,6 +6,8 @@

using namespace Pinetime::Applications::Screens;

static std::chrono::seconds lastTimer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the global static variable needed here? Couldn't we add this variable as a member variable of the class?

Copy link
Author

Choose a reason for hiding this comment

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

Had resolved, but latest commit re-introduces this -- if lastTimer is a member of the Timer app, it'll be destroyed when closing the app... Open for suggestions :)

src/displayapp/screens/Timer.cpp Outdated Show resolved Hide resolved
@tausen tausen force-pushed the timer-memory branch 7 times, most recently from 25a479d to ec17031 Compare April 28, 2024 13:51
@tausen
Copy link
Author

tausen commented Apr 28, 2024

Thanks for the feedback! PR updated accordingly.

Shouldn't the timer be reset to the last settings instead of the last paused value?

After some contemplation I've updated the PR to handle pause and stop differently. This ended up a bit more intrusive, but ultimately for the better IMO :)

I couldn't really find a good way to fit two buttons in the bottom like in the stopwatch app without things getting tight, so I've re-used the hold action on the start/stop/pause button:

  • Short press to start/pause
  • Long press while paused to stop, reverting to previously used timer setting
  • Long press while stopped to reset to 00:00

InfiniSim_2024-04-28_150611

I also had to nudge configTOTAL_HEAP_SIZE from 40 to 39 with these changes. Unsure whether I did something silly or things were just on the edge before?

@tausen
Copy link
Author

tausen commented Apr 28, 2024

Pushed one more commit (to be squashed or discarded) -- this saves the last timer setting as soon as it's changed instead of when the timer expires or is stopped. The user can now enter the timer app, configure it for some duration, exit the app again (for example to read a notification), then go back to the timer app and not have to re-configure it.

@tausen tausen requested a review from JF002 May 7, 2024 18:08
@thun11o
Copy link

thun11o commented Jul 7, 2024

Thanks for this 1st contribution @tausen !

There is no difference between restarting a paused timer and starting a new one, so briefly pausing will also update the suggested new duration

I'm wondering if this would be the expected behavior by the users. Shouldn't the timer be reset to the last settings instead of the last paused value? This is an open question, I'm not really sure about this.

New to this project and just got my first PineTime, I'm super happy with it!

I had a Samsung Watch Active before and it resets to the initial value once the timer expires which to me seems correct.

tausen added 2 commits July 12, 2024 22:14
Remember last duration used in Timer app. Stop with long press while
paused. Reset to zero with long press while stopped.
ld reports "region RAM overflowed with stack" otherwise
@tausen
Copy link
Author

tausen commented Jul 12, 2024

Thanks for the input, @thun11o, I believe the behavior with my changes should match what you're used to then :)
I'm using the timer a few times every day and usually at the same setting +/- a minute and it works quite well 👍

I've rebased this branch onto the latest main.

@tausen
Copy link
Author

tausen commented Jul 12, 2024

I've started work on a version based on #1971 over here: https://github.com/tausen/InfiniTime/tree/timer-ringing-memory
A few things I'd change before promoting it to a PR, but seems to work well so far. With longer vibration time, count up when done and memory of the last setting, it behaves much like the timers I'm used to from other devices :)

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.

Restore timer duration when finished
3 participants