Skip to content

Make watched/resume state display deterministic#1611

Open
ctismer wants to merge 1 commit into
damontecres:mainfrom
ctismer:pr/watched-state-on-resume
Open

Make watched/resume state display deterministic#1611
ctismer wants to merge 1 commit into
damontecres:mainfrom
ctismer:pr/watched-state-on-resume

Conversation

@ctismer

@ctismer ctismer commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Description

The played/watched state is not displayed reliably. The watched status is sometimes not updated in time, so the tile is only correct after leaving it and coming back.

This can be avoided entirely, because the player's final state is fully known in the client. The state stored on the server is still used when leaving the current view.

This affects the resume progress on grids and the "continue watching" row, as well as manually marking an item watched/unwatched.

Related issues

Bug fix — described above.

Testing

Manually verified on the Android TV emulator and a Fire TV Stick (Fire OS / Android 7.1.2):

  • Grid: play an item, press back → the resume bar appears immediately.
  • "Continue watching" row: resume an item, fast-forward near the end, go back → the tile updates at once (and drops when finished).
  • Detail page: mark an item watched/unwatched → the grid reflects it immediately on return.

Unit tests added: WithLocalPlaybackTests (resume-percentage derivation) and PatchWatchingRowTests (row patch/drop logic). assembleDefaultDebug and testDefaultDebugUnitTest pass; pre-commit (ktlint) is clean.
These unit tests cover the decision logic only. The end-to-end wiring — the lifecycle resume trigger, the actual UI rendering and the timing — can only be verified by an instrumented/integration test (emulator + a fake data layer).
@damontecres Would you welcome a separate PR/discussion to set that up?

AI or LLM usage

The analysis and the fix were done by me with AI assistance (Claude). I wrote the description's substance; the English wording was polished/translated with AI. I understand the code and can maintain it.

@damontecres damontecres left a comment

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.

I have not fully reviewed these changes, but please consider replacing the pseudo-subscription class PlaybackResultCache for a flow based one that ViewModels can collect from.

There is an example of this in MediaManagementService. It publishes deleted items on a flow. Then ViewModels in the entire nav backstack can collect it, if they want, to update their state in the background even before the user goes back to the page.

@ctismer
ctismer force-pushed the pr/watched-state-on-resume branch from f4252fb to 2e47a7c Compare June 22, 2026 17:13
@ctismer

ctismer commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Great idea! This reminds me of Qt/PySide's Signal/Slot mechanism. Nice to see the same pattern here. Will implement this! 😄

@ctismer
ctismer force-pushed the pr/watched-state-on-resume branch from 2e47a7c to f2f9649 Compare June 23, 2026 09:22
@ctismer

ctismer commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Done — replaced PlaybackResultCache with a flow-based PlaybackResultService that publishes outcomes on a SharedFlow, modelled on MediaManagementService. ViewModels across the back stack now collect it and update their state in the background, so the home rows and library grid no longer pull on resume.

Manually verified on the emulator (resume bar after partial playback, finished items dropping out of "continue watching", manual mark-unwatched). Tests adjusted: PatchWatchingRowTests now covers the non-consuming lookup, and a new PlaybackResultServiceTest covers the flow contract. assembleDefaultDebug, testDefaultDebugUnitTest and pre-commit (ktlint) all pass.

Nicer too: the state is updated live in the background and is already correct on return, rather than catching up after the fact.

@ctismer
ctismer force-pushed the pr/watched-state-on-resume branch 2 times, most recently from 07ba20b to 146a937 Compare June 23, 2026 21:33
@ctismer
ctismer requested a review from damontecres June 24, 2026 20:10
@ctismer
ctismer force-pushed the pr/watched-state-on-resume branch from 146a937 to 929c098 Compare June 24, 2026 21:16
…the server

Derive the resume percentage locally for freshly-played items, refresh the
home rows on resume, and remember manual watched toggles, so grids and the
continue-watching row reflect the new state immediately rather than racing
the asynchronous server playback-stopped report.
@ctismer
ctismer force-pushed the pr/watched-state-on-resume branch from 929c098 to f5fc34e Compare June 27, 2026 22:12
@JustinZeus

Copy link
Copy Markdown
Contributor

Work's a bit slow on my end, so I figured I'd shoot for a review here. Reading through I think I spotted a bug, and it happened to land in a flow I actually use (marking an item unwatched to reset its progress), so rather than guess I spun up an emulator to check, and it does behave the way I expected.

The main fix works great: play partway, back out, and the resume bar shows on the grid right away.

The unwatch path is where it goes sideways though. Repro with a movie that has a resume bar: mark it watched, then mark it unwatched, then back to the grid. The bar is still sitting on the tile at the old position, even though the detail page (fresh from the server) correctly shows no resume. It only clears once you leave the grid and come back.

Walking the chain: markUnplayed comes back with the resume already cleared, but FavoriteWatchManager.setWatched only records playbackPositionTicks + played into the cache, so content.playedPercentage (the correct cleared value) is dropped. On return withLocalPlayback(0, false) has nothing to go on, falls into its else and keeps the existing playedPercentage. The card then draws the bar straight off playedPercentage regardless of played (the watchedPercent?.let { } block), so the stale percentage shows up as a bar.

Threading the cleared percentage through from content would probably do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants