-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[Awake] Fix for countdown timer drift #41684
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
base: main
Are you sure you want to change the base?
Conversation
…o separate method.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes countdown timer drift issues in the Awake module by replacing the combined Observable.Timer and Observable.Interval approach with a single interval timer that checks against a fixed expiry time.
- Consolidates duplicate timer completion logic into a shared
HandleTimerCompletion
method - Removes the ~50 day timer limit, allowing up to ~136 years (uint.MaxValue seconds)
- Fixes timer drift by using absolute time comparisons instead of cumulative intervals
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if (TimeRemaining >= 0) | ||
Observable.Interval(TimeSpan.FromSeconds(1)) | ||
.Select(_ => targetExpiryTime - DateTimeOffset.Now) | ||
.TakeWhile(remaining => remaining.TotalSeconds > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition remaining.TotalSeconds > 0
will exclude the case where exactly 0 seconds remain, potentially causing the timer to miss the final update before completion. Consider using >= 0
to ensure the timer displays '0 seconds remaining' before completing.
.TakeWhile(remaining => remaining.TotalSeconds > 0) | |
.TakeWhile(remaining => remaining.TotalSeconds >= 0) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At zero seconds, the full duration will have expired and the application will immediately end the timed mode.
The next thing that would happen is that the application would exit completely (if in "standalone" mode), or would enter the Passive mode, which updates the system tray icon straight away, removing the hover text.
The "00d 00h 00m 00s" countdown timer would likely never be seen.
{ | ||
TimeRemaining = (uint)remainingTimeSpan.TotalSeconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting TotalSeconds
to uint
will truncate negative values to unexpected large numbers if the timer has already expired. Since TakeWhile
should prevent negative values, consider adding Math.Max(0, remainingTimeSpan.TotalSeconds)
for safety.
TimeRemaining = (uint)remainingTimeSpan.TotalSeconds; | |
TimeRemaining = (uint)Math.Max(0, remainingTimeSpan.TotalSeconds); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TakeWhile
clause for the Observable.Interval
is remaining.TotalSeconds > 0
. It is therefore not possible for the "tick" code to be executed with a negative value for TimeRemaining
.
Observable.Interval(TimeSpan.FromSeconds(1))
.Select(_ => targetExpiryTime - DateTimeOffset.Now)
.TakeWhile(remaining => remaining.TotalSeconds > 0) // condition - there must be time remaining
.Subscribe(
remainingTimeSpan =>
{
// At this point, the seconds remaining cannot be negative.
TimeRemaining = (uint)remainingTimeSpan.TotalSeconds;
Summary of the Pull Request
SetExpirableKeepAwake
andSetTimedKeepAwake
.uint.MaxValue
seconds, or ~136 years.PR Checklist
Detailed Description of the Pull Request / Additional comments
This replaces the combined
Observable.Timer
andObservable.Interval
timers with a single 1-second Interval timer which checks against a fixed expiry time.Validation Steps Performed
Checked that:
--time-limit
parameter, and expiry occurs on time.