Play held-title theme music while the title page is resumed #71

Merged

justinzeus merged 1 commit from fm/theme-music-title into fm/tv-round2

2026-11-07 23:47:29 +01:00

justinzeus commented

2026-11-07 23:44:47 +01:00

Owner

Goal

Activate theme music on held Wolfskin title pages without changing their UI or the Conductarr TV API. Target: fm/tv-round2.

Change

Use the existing upstream ThemeSongPlayer from a LifecycleResumeEffect, restricted to the current held title entry.
Use the series Jellyfin ID for episode pages; catalogue-only titles remain silent.
Reuse AppPreference.PlayThemeMusic: Disabled stays silent and High remains the upstream 0.5 volume.
Release the theme on pause/disposal, outgoing navigation, identity changes and video/trailer hand-off.
Leave theme-file publishing to the separate Conductarr lane. Empty Jellyfin theme lists retain upstream silence.

Verification

33 touched JVM/native Robolectric cases passed, including seven new theme lifecycle cases. The native smoke opens the real episode title destination, observes one series-theme start, activates Play, and verifies theme stop before playback navigation with no restart while the outgoing title remains composed. Other cases cover held movies, metadata refresh/recreation, catalogue-only and disabled titles, pause/resume, disposal, identity ownership and navigation away/back.

bash docker/gradle.sh debug :wolfskin-design:cacheRobolectricSdk :app:testDefaultDebugUnitTest -x :wolfskin-design:testDebugUnitTest --tests '*TitleThemeMusicTest' --tests '*TitlePlaybackNavigationTest' --tests '*TitleEntryBackTest' --tests '*TitleAsyncFocusTest' --tests '*ConductarrTitleSurfaceTest' --tests '*DiscoverRefreshMotionTest' --tests '*DiscoverTitleTimingTest' --tests '*PersonSwitchThreadingTest' --tests '*TitleRequestSafetyTest'

The first run lacked the Android 16 Robolectric runtime for six existing Back cases; the repository cache task supplied it and the final run passed all 33. Shared Gradle lock respected; no full suite, APK build, TV-box access, ADB or Jellyfin mutation.

One deferred manual check

With operator approval, hand-place one theme.m4a symlink in a single held title's layer folder, refresh that item in Jellyfin, then open the title page at High. Confirm audible theme playback, silence when starting video/leaving/backgrounding/switching person/signing out, and one theme when returning. This check was not performed in this lane.

Risk

Audible end-to-end proof depends on Jellyfin indexing a real theme file; native tests use a fake theme player. The existing upstream streaming and volume implementation is unchanged.

Implementation and verification assisted by AI.

## Goal Activate theme music on held Wolfskin title pages without changing their UI or the Conductarr TV API. Target: `fm/tv-round2`. ## Change - Use the existing upstream `ThemeSongPlayer` from a `LifecycleResumeEffect`, restricted to the current held title entry. - Use the series Jellyfin ID for episode pages; catalogue-only titles remain silent. - Reuse `AppPreference.PlayThemeMusic`: Disabled stays silent and High remains the upstream 0.5 volume. - Release the theme on pause/disposal, outgoing navigation, identity changes and video/trailer hand-off. - Leave theme-file publishing to the separate Conductarr lane. Empty Jellyfin theme lists retain upstream silence. ## Verification 33 touched JVM/native Robolectric cases passed, including seven new theme lifecycle cases. The native smoke opens the real episode title destination, observes one series-theme start, activates Play, and verifies theme stop before playback navigation with no restart while the outgoing title remains composed. Other cases cover held movies, metadata refresh/recreation, catalogue-only and disabled titles, pause/resume, disposal, identity ownership and navigation away/back. ```sh bash docker/gradle.sh debug :wolfskin-design:cacheRobolectricSdk :app:testDefaultDebugUnitTest -x :wolfskin-design:testDebugUnitTest --tests '*TitleThemeMusicTest' --tests '*TitlePlaybackNavigationTest' --tests '*TitleEntryBackTest' --tests '*TitleAsyncFocusTest' --tests '*ConductarrTitleSurfaceTest' --tests '*DiscoverRefreshMotionTest' --tests '*DiscoverTitleTimingTest' --tests '*PersonSwitchThreadingTest' --tests '*TitleRequestSafetyTest' ``` The first run lacked the Android 16 Robolectric runtime for six existing Back cases; the repository cache task supplied it and the final run passed all 33. Shared Gradle lock respected; no full suite, APK build, TV-box access, ADB or Jellyfin mutation. ## One deferred manual check With operator approval, hand-place one `theme.m4a` symlink in a single held title's layer folder, refresh that item in Jellyfin, then open the title page at High. Confirm audible theme playback, silence when starting video/leaving/backgrounding/switching person/signing out, and one theme when returning. This check was not performed in this lane. ## Risk Audible end-to-end proof depends on Jellyfin indexing a real theme file; native tests use a fake theme player. The existing upstream streaming and volume implementation is unchanged. Implementation and verification assisted by AI.

justinzeus added 1 commit 2026-11-07 23:44:48 +01:00

Play held-title themes only while their page is resumed

review/code-claude Code APPROVE: held-only, stops on leave/playback/background/person

Details

review/design-claude Design PASS: no visual change

Details

2f3cdf2339

justinzeus commented

2026-11-07 23:47:07 +01:00

Author

Owner

Review of the theme music title-page PR at head 2f3cdf233976e60745c9be10e7c920cad1d83674 (base fm/tv-round2).

Code verdict: APPROVE (review/code-claude = success).
Design verdict: PASS (review/design-claude = success). No visual, focus or layout change: the diff adds one non-visual composable call.

Held titles only.

The theme id comes from a held item only, using the series id for episodes. Catalogue ids (movie:<tmdb>, tv:<tmdb>) don't parse as UUIDs, a missing series id stays silent, and person pages never play. The call also requires the Ready status.
An empty Jellyfin theme list stays silent, as upstream.

Preference.

It reads the existing playThemeSongs setting. Disabled never starts the effect, and switching to Disabled while playing stops it.
The 0.5 High volume is the unchanged upstream player behaviour, and no new setting is added.

Once, not twice.

The effect is a LifecycleResumeEffect keyed on item id, person-and-session owner, and volume.
A metadata refresh does not restart it. Returning to the page restarts it once, after the stop.

Stops on every required event.

Leaving the page or an outgoing entry that is still composed.
Playback: play() stops the theme just before navigateTo(Playback), and the trailer path stops it too.
Backgrounding, a person switch or sign-out, and dispose.
The new native test opens the real episode title page, starts the series theme once, presses Play, and asserts the stop ran before playback navigation, with nothing playing afterwards. The fake player fails if two pages ever play at once.

Regression surface. Only the Hilt-injected constructor parameter and the two stop() calls touch non-theme code. All 11 test constructor sites were updated. A merge-tree check against current fm/tv-round2 is clean. I could not test against the pending polish lanes.

Non-blocking notes:

Upstream ThemeSongPlayer.play and stop each run in their own scope.launch on the IO scope. The mutex serializes them but does not preserve call order. A page swap (title to related title and Back, or a person switch) issues a stop and a play back to back, so an inversion would silence the returning page or leave a theme running on a page that is gone. The window is a few milliseconds and the old upstream screens had the same property. The fake player cannot observe it. If it shows on the box, the fix belongs in the upstream player (an ordered single-threaded dispatch). [INFERENCE]
stop() is asynchronous, so there are a few milliseconds of audio before the video player starts. The stop is issued before navigation, so there is no sustained overlap.
ThemeSongVolume.HIGHEST maps to 75f upstream; ExoPlayer clamps it. Pre-existing.
Offline saved pages that are Ready still try to fetch a theme, and the request fails silently upstream.
The deferred manual check still needs a real theme file: High, then start video, leave, background, switch person, sign out, return.

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

Review of the theme music title-page PR at head `2f3cdf233976e60745c9be10e7c920cad1d83674` (base `fm/tv-round2`). **Code verdict: APPROVE (`review/code-claude` = success).** **Design verdict: PASS (`review/design-claude` = success).** No visual, focus or layout change: the diff adds one non-visual composable call. **Held titles only.** - The theme id comes from a held item only, using the series id for episodes. Catalogue ids (`movie:<tmdb>`, `tv:<tmdb>`) don't parse as UUIDs, a missing series id stays silent, and person pages never play. The call also requires the Ready status. - An empty Jellyfin theme list stays silent, as upstream. **Preference.** - It reads the existing `playThemeSongs` setting. Disabled never starts the effect, and switching to Disabled while playing stops it. - The 0.5 High volume is the unchanged upstream player behaviour, and no new setting is added. **Once, not twice.** - The effect is a `LifecycleResumeEffect` keyed on item id, person-and-session owner, and volume. - A metadata refresh does not restart it. Returning to the page restarts it once, after the stop. **Stops on every required event.** - Leaving the page or an outgoing entry that is still composed. - Playback: `play()` stops the theme just before `navigateTo(Playback)`, and the trailer path stops it too. - Backgrounding, a person switch or sign-out, and dispose. - The new native test opens the real episode title page, starts the series theme once, presses Play, and asserts the stop ran before playback navigation, with nothing playing afterwards. The fake player fails if two pages ever play at once. **Regression surface.** Only the Hilt-injected constructor parameter and the two `stop()` calls touch non-theme code. All 11 test constructor sites were updated. A merge-tree check against current `fm/tv-round2` is clean. I could not test against the pending polish lanes. **Non-blocking notes:** 1. Upstream `ThemeSongPlayer.play` and `stop` each run in their own `scope.launch` on the IO scope. The mutex serializes them but does not preserve call order. A page swap (title to related title and Back, or a person switch) issues a stop and a play back to back, so an inversion would silence the returning page or leave a theme running on a page that is gone. The window is a few milliseconds and the old upstream screens had the same property. The fake player cannot observe it. If it shows on the box, the fix belongs in the upstream player (an ordered single-threaded dispatch). [INFERENCE] 2. `stop()` is asynchronous, so there are a few milliseconds of audio before the video player starts. The stop is issued before navigation, so there is no sustained overlap. 3. `ThemeSongVolume.HIGHEST` maps to `75f` upstream; ExoPlayer clamps it. Pre-existing. 4. Offline saved pages that are Ready still try to fetch a theme, and the request fails silently upstream. 5. The deferred manual check still needs a real theme file: High, then start video, leave, background, switch person, sign out, return. Full report (review 9): `/root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md`. Not run: Gradle/make, nothing on the box.