Reset subtitle delay when the new track has none saved #73

Merged

justinzeus merged 1 commit from fm/sub-offset-reset into fm/tv-round2

2026-11-08 01:37:40 +01:00

justinzeus commented

2026-11-08 01:34:08 +01:00

Owner

Change

Reset the enabled subtitle track's delay to zero when it has no saved track modification. Direct-play track switches previously copied the old track's delay and only replaced it when the new track had a saved row.

One standalone, upstream-path-only commit: Reset subtitle delay when the new track has none saved. Add a regression using the existing MockK/Robolectric harness: start on track A with 2 seconds saved, then switch to track B with no saved delay, track B with a saved -750 ms delay, or subtitles disabled. The test also guards against falling back to media-item reload. No database, proto, renderer, player-gate, or Wolfskin-specific changes.

Verification

Before the fix: 19 tests, one failure; the missing-row switch expected 0s but retained 2s.
After the fix: all 19 PlaybackViewModelTests pass, including all three subtitle-switch cases.
Command: bash docker/gradle.sh debug :app:testDefaultDebugUnitTest --tests 'com.github.damontecres.wholphin.ui.playback.PlaybackViewModelTests' -x :wolfskin-design:testDebugUnitTest. The exclusion avoids the unrelated design suite inherited by app Test tasks.
ktlint 1.8.0 passes the touched test file. Production-file checking reports only the pre-existing chain-continuation violation at PlaybackViewModel.kt:838:102, reproduced against the committed baseline and left unchanged.

Scope and risk

This is only subtitle-delay persistence/reset commit 3, independent of the renderer/control work. Disabled subtitles retain the inactive delay without loading a disabled-track row. Validation exercises view-model switching with mocked player/network/persistence boundaries; no APK, emulator, ADB, or TV-box check. Burned-in subtitle support, positive-delay seek behavior and paused libass repaint are outside this change.

AI-assisted implementation; this PR is on the private fork only, not the upstream repository.

## Change Reset the enabled subtitle track's delay to zero when it has no saved track modification. Direct-play track switches previously copied the old track's delay and only replaced it when the new track had a saved row. One standalone, upstream-path-only commit: `Reset subtitle delay when the new track has none saved`. Add a regression using the existing MockK/Robolectric harness: start on track A with 2 seconds saved, then switch to track B with no saved delay, track B with a saved -750 ms delay, or subtitles disabled. The test also guards against falling back to media-item reload. No database, proto, renderer, player-gate, or Wolfskin-specific changes. ## Verification - Before the fix: 19 tests, one failure; the missing-row switch expected `0s` but retained `2s`. - After the fix: all 19 `PlaybackViewModelTests` pass, including all three subtitle-switch cases. - Command: `bash docker/gradle.sh debug :app:testDefaultDebugUnitTest --tests 'com.github.damontecres.wholphin.ui.playback.PlaybackViewModelTests' -x :wolfskin-design:testDebugUnitTest`. The exclusion avoids the unrelated design suite inherited by app Test tasks. - ktlint 1.8.0 passes the touched test file. Production-file checking reports only the pre-existing chain-continuation violation at `PlaybackViewModel.kt:838:102`, reproduced against the committed baseline and left unchanged. ## Scope and risk This is only subtitle-delay persistence/reset commit 3, independent of the renderer/control work. Disabled subtitles retain the inactive delay without loading a disabled-track row. Validation exercises view-model switching with mocked player/network/persistence boundaries; no APK, emulator, ADB, or TV-box check. Burned-in subtitle support, positive-delay seek behavior and paused libass repaint are outside this change. AI-assisted implementation; this PR is on the private fork only, not the upstream repository.

justinzeus added 1 commit 2026-11-08 01:34:08 +01:00

Reset subtitle delay when the new track has none saved

review/code-claude Code APPROVE: reset exactly per plan, persistence unchanged

Details

review/design-claude Design PASS: no UI change

Details

8da699ca8b

justinzeus commented

2026-11-08 01:36:46 +01:00

Author

Owner

Review of the subtitle delay reset slice at head 8da699ca8baa41d14741e6824ba183214a792bec (base fm/tv-round2).

Code verdict: APPROVE (review/code-claude = success).
Design verdict: PASS (review/design-claude = success). No UI file, string or token is touched.

Resets where the plan says.

Plan commit 3 says that when subtitles are enabled the delay becomes result?.delayMs?.milliseconds ?: Duration.ZERO. The diff does exactly that, still inside subtitleIndexEnabled.
Disabled subtitles are untouched: the inactive track's delay is kept and no disabled-track row is looked up.
loadSubtitleDelay runs after a direct-play in-place switch, which is the buggy path, and after a media-item prepare. After a prepare the playback state is rebuilt with delay zero, so the reset changes nothing there.
A track with a saved row still gets its saved value.

Persistence unchanged.

Nothing is written on reset. Saving still happens only from the user's delay change.
No database, proto or renderer change.

MPV.

There is no MPV-specific code. MPV reads currentPlayback.subtitleDelay.
Saved rows load as before. The only difference is that a track without a saved row no longer inherits the previous track's delay, which was the same stale-delay bug on MPV.

Test.

The PlaybackViewModelTests case starts with track A at 2 s saved and switches by changeSubtitleStream. B with no row gives zero, B with -750 ms gives -750 ms, and DISABLED keeps 2 s.
A setMediaItem count guards against a reload fallback, and a coVerify(exactly = 0) guards the disabled lookup.
Per the PR it fails before (expected:<0s> but was:<2s>) and passes after (19 tests). I did not re-run it.

Merge and scope. A merge-tree check against current fm/tv-round2 is clean. It is a standalone commit with no Wolfskin-specific change.

Non-blocking notes:

The load is asynchronous. A user nudge in the few milliseconds between the index update and the IO write could be overwritten, now also when no row exists. This is a pre-existing pattern and very narrow.
A reset is not persisted, so a track saved at 0 and a track that never had a row are indistinguishable. This has no behavioural effect.
The test uses mocked player, network and persistence. There is no renderer or device validation, as the PR states.

Full report (review 11): /root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md. Not run: Gradle/make, nothing on the box.

Review of the subtitle delay reset slice at head `8da699ca8baa41d14741e6824ba183214a792bec` (base `fm/tv-round2`). **Code verdict: APPROVE (`review/code-claude` = success).** **Design verdict: PASS (`review/design-claude` = success).** No UI file, string or token is touched. **Resets where the plan says.** - Plan commit 3 says that when subtitles are enabled the delay becomes `result?.delayMs?.milliseconds ?: Duration.ZERO`. The diff does exactly that, still inside `subtitleIndexEnabled`. - Disabled subtitles are untouched: the inactive track's delay is kept and no disabled-track row is looked up. - `loadSubtitleDelay` runs after a direct-play in-place switch, which is the buggy path, and after a media-item prepare. After a prepare the playback state is rebuilt with delay zero, so the reset changes nothing there. - A track with a saved row still gets its saved value. **Persistence unchanged.** - Nothing is written on reset. Saving still happens only from the user's delay change. - No database, proto or renderer change. **MPV.** - There is no MPV-specific code. MPV reads `currentPlayback.subtitleDelay`. - Saved rows load as before. The only difference is that a track without a saved row no longer inherits the previous track's delay, which was the same stale-delay bug on MPV. **Test.** - The `PlaybackViewModelTests` case starts with track A at 2 s saved and switches by `changeSubtitleStream`. B with no row gives zero, B with -750 ms gives -750 ms, and DISABLED keeps 2 s. - A `setMediaItem` count guards against a reload fallback, and a `coVerify(exactly = 0)` guards the disabled lookup. - Per the PR it fails before (`expected:<0s> but was:<2s>`) and passes after (19 tests). I did not re-run it. **Merge and scope.** A merge-tree check against current `fm/tv-round2` is clean. It is a standalone commit with no Wolfskin-specific change. **Non-blocking notes:** 1. The load is asynchronous. A user nudge in the few milliseconds between the index update and the IO write could be overwritten, now also when no row exists. This is a pre-existing pattern and very narrow. 2. A reset is not persisted, so a track saved at 0 and a track that never had a row are indistinguishable. This has no behavioural effect. 3. The test uses mocked player, network and persistence. There is no renderer or device validation, as the PR states. Full report (review 11): `/root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md`. Not run: Gradle/make, nothing on the box.