Add alert when set stopwatch reaches target - #108
Conversation
IamDg
left a comment
There was a problem hiding this comment.
Hi,
Thank you for your contribution!
The implementation might work but it will suffer from the following bugs:
-
Bug A: Editing While Paused
- The Scenario: A user sets a 60s target, runs the stopwatch, and pauses it at 20s. They realize they want a 90s target instead, so they manually edit the text field to 01:30 (90s) while paused.
- The Problem:
set.elapsedTimeis updated to 90.- When they click Play to resume,
setStopwatchTargetsalready has an entry for this set ID (60). targetElapsedTimeevaluates to 60 (the old target), whileinitialElapsedTimeevaluates to 90 (the newly edited value).- In the very first iteration of the loop,
newElapsedTimestarts at 90. Since90 >= 60is true, the stopwatch immediately terminates and sounds the completion alert, ignoring the user's manual change.
-
Bug B: Resume Bug (Major UX Breakdown)
-
The Scenario: A user completes 20s of a 60s duration set. The app is put into the background, gets killed by the OS (process death), or is closed and reopened. The workout is restored from the local database.
-
The Problem:
- In the ViewModel's init,
_idsOfSetsWithStopwatchNotStartedAtLeastOnceis repopulated with all restored set IDs:
_idsOfSetsWithStopwatchNotStartedAtLeastOnce.update { exercises.value.flatMap { it.sets }.map { it.id }.toSet() }- The
setStopwatchTargetsmap is a pure in-memory MutableMap and is now completely empty. - When the user taps Play to resume their progress:
- targetElapsedTime is computed via
getOrPut(id) { set.elapsedTime }. Since the map is empty, it puts the currentset.elapsedTime(20) as the target duration! - initialElapsedTime is set to 0 because the set ID is found in
idsOfSetsWithStopwatchNotStartedAtLeastOnce. - Result: The set's elapsed time resets to 0 and will stop/alert when it hits 20 seconds instead of the original 60 seconds. The user's progress is completely ruined.
- targetElapsedTime is computed via
- In the ViewModel's init,
-
-
Bug C: Memory Leaks on Exercise Deletion
- When a set is deleted individually via
deleteSet(id), its target is cleaned up:setStopwatchTargets.remove(id). - However, when an entire exercise is deleted via
deleteExercise(exerciseId), its associated set IDs are never cleared fromsetStopwatchTargets, leading to minor memory accumulation during long workout sessions.
- When a set is deleted individually via
| if (targetElapsedTime > 0 && newElapsedTime >= targetElapsedTime) { | ||
| if (userPreferences.restTimerSoundOn.value) { | ||
| soundPlayer.playAlert() | ||
| } | ||
| updateIdSetWithRunningStopwatch(null) | ||
| return | ||
| } | ||
|
|
||
| delay(1000) |
There was a problem hiding this comment.
The condition can be simplified as shown. Also delay now uses the modern overload with duration instead of integer.
| if (targetElapsedTime > 0 && newElapsedTime >= targetElapsedTime) { | |
| if (userPreferences.restTimerSoundOn.value) { | |
| soundPlayer.playAlert() | |
| } | |
| updateIdSetWithRunningStopwatch(null) | |
| return | |
| } | |
| delay(1000) | |
| if (targetElapsedTime in 1..newElapsedTime) { | |
| if (userPreferences.restTimerSoundOn.value) { | |
| soundPlayer.playAlert() | |
| } | |
| updateIdSetWithRunningStopwatch(null) | |
| return | |
| } | |
| delay(1000.milliseconds) |
|
Sry about that yeah the implementation was way to simple and I didn't thought about the edge cases. I will do a proper commit later. |
No worries. Take your time |
Fixes missing alert sound when a duration-based set timer reaches its configured time, using the existing rest timer sound preference.
Issue #101