Skip to content

Fix timer ignoring workout repetitions #46

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.

Sign up for GitHub

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

Open
Maria1-eng wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix timer ignoring workout repetitions #46

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 31 additions & 8 deletions ...ain/kotlin/com/majotyler/hiittimer/presentation/playWorkoutScreen/PlayWorkoutVIewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ class PlayWorkoutVIewModel(
private val currentWorkoutInterval: Interval?
get() = currentWorkout?.intervals?.getOrNull(index = currentWorkoutIntervalIndex)

/** Tracks how many times the current workout's intervals have been repeated (0-based). */

@Tyler-Lopez Tyler-Lopez Apr 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
/** Tracks how many times the current workout's intervals have been repeated (0-based). */
/** The index of the current repetition of the [currentWorkoutInterval] */

This is just a nit, but I would change the comment to be like this to be more in line with the others.

private var currentWorkoutRepetitionIndex: Int = 0

private var pollProgressJob: Job? = null
private var runningMark: TimeMark? = null

Expand Down Expand Up @@ -164,14 +167,34 @@ class PlayWorkoutVIewModel(
currentWorkoutIntervalState == PlayWorkoutStateOfInterval.RESTING
val currentIntervalIsLastInWorkout =
currentWorkoutIntervalIndex == lastIndexOfIntervalsInCurrentWorkout
val hasFinishedAllWorkouts = isResting && currentIntervalIsLastInWorkout

if (hasFinishedAllWorkouts) {
_play.value = false
_text.value = "Start"
_enabled.value = false
pollProgressJob?.cancel()

val hasFinishedCurrentWorkoutCycle = isResting && currentIntervalIsLastInWorkout

if (hasFinishedCurrentWorkoutCycle) {
val totalRepetitions = currentWorkout?.repetitions ?: 1
val hasMoreRepetitions = currentWorkoutRepetitionIndex < totalRepetitions - 1
val isLastWorkout = currentWorkoutIndex == workouts.lastIndex
Comment on lines +170 to +175

@Tyler-Lopez Tyler-Lopez Apr 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Do all these names make sense to you? It got rid of hasFinishedAllWorkouts in favor of isLastWorkout, for example.


when {
hasMoreRepetitions -> {
currentWorkoutRepetitionIndex++
currentWorkoutIntervalIndex = 0
currentWorkoutIntervalState = PlayWorkoutStateOfInterval.EXERCISING
currentStepProgressMs = 0L
}
!isLastWorkout -> {
currentWorkoutRepetitionIndex = 0
currentWorkoutIndex++

@Tyler-Lopez Tyler-Lopez Apr 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This PR actually does something besides fixing repetitions 😮

So you see that right? It noticed that we also were ignoring the fact that we only were iterating over just one Workout, when we could have a whole list of Workout.

That is why this PR has so many lines change, it also includes that.

Could we rename this PR and commit something like Fix timer ignoring multiple workouts and repetitions?

@Maria1-eng Maria1-eng Apr 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We were initially focusing on a single workout because each workout already contains multiple intervals and exercises. My intention was that users could select which workout they want to run, which is why I was considering adding a database to manage multiple workouts. What do you think?

@Tyler-Lopez Tyler-Lopez Apr 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

So we will make it so instead of incrementing workouts, you always just "play" one workout at a time, right? Never more than one workout at a time.

Then, because in the future, you will save workouts in a database, it'll be really easy to finish one workout and start another?

I think that makes a lot of sense, and I bet if you fed these instructions into Claude it could kind of put some things together.

One thing is that this would kind of be a big refactor because it probably eliminates the need to have the whole "Build Workout" screen.

That said, I think if you want to merge this as-is it is fine, or you could remove (or tell Claude to remove) the lines about incrementing the workout for now.

currentWorkoutIntervalIndex = 0
currentWorkoutIntervalState = PlayWorkoutStateOfInterval.EXERCISING
currentStepProgressMs = 0L
}
else -> {
_play.value = false
_text.value = "Start"
_enabled.value = false
pollProgressJob?.cancel()
}
}
} else {
nextStep()
}
Expand Down