CCExtractor / ultimate_alarm_clock

GNU General Public License v2.0
68 stars 124 forks source link

fix: Timer Functionality Issue When App is Backgrounded #496

Closed oops-shlok closed 6 months ago

oops-shlok commented 6 months ago

Description

This PR fixes the issue in the timerController where the timer countdown would accelerate when the app was backgrounded and resumed.

Proposed Changes

Fixes #494

Screenshots

https://github.com/CCExtractor/ultimate_alarm_clock/assets/83498152/3ee2fb5b-dfde-4836-b484-34ea79bf9a4e

Checklist

SATVIKSH commented 6 months ago

Hey, @oops-shlok I would like to point out some adjustments to your latest commit, 1) The reason why this bug occured was because of the faulty use of app lifecycle. When the application was being brought to focus the app was running loadTimerFromStorage() method which led to the start of another timer over the previously running timer(2 timers running simultaneously hence it was skipping 2 values in one second). So the issue could've been solved by simply removing this method. 2) The methods you've removed from resumeTimer() and pauseTimer() are necessary for the notification of timer completion(which requires some adjustments to be working correctly).

oops-shlok commented 6 months ago

Hey, @oops-shlok I would like to point out some adjustments to your latest commit,

  1. The reason why this bug occured was because of the faulty use of app lifecycle. When the application was being brought to focus the app was running loadTimerFromStorage() method which led to the start of another timer over the previously running timer(2 timers running simultaneously hence it was skipping 2 values in one second). So the issue could've been solved by simply removing this method.
  2. The methods you've removed from resumeTimer() and pauseTimer() are necessary for the notification of timer completion(which requires some adjustments to be working correctly).

Hey @SATVIKSH, i understood the points you mentioned. I’ll update them accordingly. Thanks for pointing out the adjustments.

oops-shlok commented 6 months ago

Hey, @oops-shlok I would like to point out some adjustments to your latest commit,

  1. The reason why this bug occured was because of the faulty use of app lifecycle. When the application was being brought to focus the app was running loadTimerFromStorage() method which led to the start of another timer over the previously running timer(2 timers running simultaneously hence it was skipping 2 values in one second). So the issue could've been solved by simply removing this method.
  2. The methods you've removed from resumeTimer() and pauseTimer() are necessary for the notification of timer completion(which requires some adjustments to be working correctly).

I have updated the code now. I added a condition of '!isTimerRunning.value' to ensure that the timer state is only loaded from storage when the app is resumed and the timer is not already running. This prevents the duplication of timers and ensures that the decrement happens by 1 second

SATVIKSH commented 6 months ago

Hey, @oops-shlok I would like to point out some adjustments to your latest commit,

  1. The reason why this bug occured was because of the faulty use of app lifecycle. When the application was being brought to focus the app was running loadTimerFromStorage() method which led to the start of another timer over the previously running timer(2 timers running simultaneously hence it was skipping 2 values in one second). So the issue could've been solved by simply removing this method.
  2. The methods you've removed from resumeTimer() and pauseTimer() are necessary for the notification of timer completion(which requires some adjustments to be working correctly).

I have updated the code now. I added a condition of '!isTimerRunning.value' to ensure that the timer state is only loaded from storage when the app is resumed and the timer is not already running. This prevents the duplication of timers and ensures that the decrement happens by 1 second

What I was trying to say is, the implementation using the AppLifecycleState.resumed function is redundant. As long as the app is in memory there is no need to override app lifecycle and load the timer from storage since the timer is already loaded in the correct state. Try removing the override of the didChangeAppLifecycleState(AppLifecycleState state) function and it should work just fine :)

oops-shlok commented 6 months ago

Hey, @oops-shlok I would like to point out some adjustments to your latest commit,

  1. The reason why this bug occured was because of the faulty use of app lifecycle. When the application was being brought to focus the app was running loadTimerFromStorage() method which led to the start of another timer over the previously running timer(2 timers running simultaneously hence it was skipping 2 values in one second). So the issue could've been solved by simply removing this method.
  2. The methods you've removed from resumeTimer() and pauseTimer() are necessary for the notification of timer completion(which requires some adjustments to be working correctly).

I have updated the code now. I added a condition of '!isTimerRunning.value' to ensure that the timer state is only loaded from storage when the app is resumed and the timer is not already running. This prevents the duplication of timers and ensures that the decrement happens by 1 second

What I was trying to say is, the implementation using the AppLifecycleState.resumed function is redundant. As long as the app is in memory there is no need to override app lifecycle and load the timer from storage since the timer is already loaded in the correct state. Try removing the override of the didChangeAppLifecycleState(AppLifecycleState state) function and it should work just fine :)

Yes, I understood your point. However, I wanted to keep the override of the didChangeAppLifecycleState because I was planning to use it for another feature that I am planning to work on. But yeah, for now, removing that override would work.

oops-shlok commented 6 months ago

@MarkisDev is this fine?