Conversation
|
A lot of paragraphs, long explanations in both the pr and code. Almost 50% of the lines added is explanations. Must be gemini. |
|
Opus and GPT actually, you should've seen the initial draft ;) Honestly, I find they're great for making unit tests, which do sometimes end up being a bit too substantive. They're also pretty good at summarizing what I'm trying to get at, although most LLMs do tend to be on the lengthy side, as you've noted. Anyway, did you get a chance to look at the changes themselves? If so, any feedback on that part? |
Seems good so far. What was the short-term goal of this? To basically stop marking watched on Trakt when the user pauses with less than 20% remaining? @halibiram Thoughts? |
TL;DR: what you said, except the other way round :) The PR doesn't change what Trakt/Simkl record; they still mark something watched at 80% server-side. The issue was that locally it uses 90%, so if you exit around 85%, the provider has already dropped its progress while locally it's still considered in progress. That could leave the season overview showing neither progress nor a watched badge until the tracker history synced back up. This change fixes that mismatch by retaining the local position when the provider no longer has one, and making local completion runtime-aware. So a 22-minute episode at 85% can be considered complete, while a 60-minute one at 85% isn't. |
33fafdf to
f096d76
Compare
|
Folded in an additional fix for the external-player handoff that surfaced during recent on device testing: Also tightened the terminal-ending check against an open-ended sentinel in the animeskip data, and addressed the lengthyness a bit :) |
ieno
changed the title
Trakt and Simkl delete the resume point once a stop scrobble reports 80% or more. getAllEpisodeProgress reads only provider progress while a provider is connected, so the position saved on exit was never read back and the episode card showed nothing until history synced. Retain local in-progress entries the provider has no entry for. Provider entries still win. Completed local entries are not retained, so they cannot resurrect watched state removed elsewhere. Seed the local flow with onStart so the DataStore read does not gate the combine's first emission. The completed badge now takes precedence over the progress bar on the episode card.
Trakt and Simkl treat a stop scrobble at 80% or more as completion. Local completion used WatchProgress.COMPLETED_THRESHOLD at 90%. Exiting between the two told the tracker the item was finished while recording it as in progress locally. A fixed percentage ignores runtime: 85% of a 22 minute episode leaves three minutes, 85% of a 60 minute episode leaves nine. PlaybackCompletionRules qualifies an exit on remaining time or on credits having started, above the 80% floor the provider has already acted on. The 90% threshold stays as the fallback. The four minute window only opens a band under the fallback between 20 and 40 minutes of runtime. Outside that the floor or the fallback is reached first. Tests pin both crossovers from either side. A segment counts as credits when it ends within four minutes of the file end and runs no longer than that. The length bound is a deliberate cutoff, not something the metadata asserts: without it an ending annotated from 42:00 to the end of a 60 minute file completes an exit at 48:00, with twelve minutes left. Bounding the start at the 80% floor instead was rejected, because it leaves the clause doing almost nothing on the runtimes where skip metadata exists. Open-ended animeskip intervals and non-finite times fail the bounds. leavesCurrentItem separates an item exit from a teardown that keeps the same item playing, such as an engine switch or codec recovery. It defaults to false. Episode switches resolve it from the target video id, since a stream selection can target the episode already playing. The external-player handoff passes false: stopAndRelease ran just before the intent, so handing off a short episode at 85% completed it locally for playback the user was continuing elsewhere. This covers local state only; the flush still emits a stop scrobble, so the provider drops its resume point either way.
…ends saveWatchProgressInternal read hasMarkedCurrentEpisodeCompleted, then set it inside its write coroutine, after awaiting normalizeParentContentId. That put a suspend call between the read and the write. An episode switch continues on the calling thread meanwhile, reaches resetLoadingOverlayForNewStream and clears the latch, and the write then lands against the incoming episode. Its own completion is skipped, leaving it at its last periodic save. Whether the call actually suspends on a given run does not matter; the code has to be correct either way, and it was not. Replace the boolean with EpisodeCompletionLatch, whose claim() is a single read-modify-write. The read cannot be separated from the write, so the shape that caused this cannot be spelled. The decision is resolved before the launch: only contentId is normalized in the coroutine, and isCompleted() does not read it. A claim always reaches its write. normalizeParentContentId resolves through a StateFlow and a string parse, with no throw surface, so there is no path that claims and then skips markAsCompleted. The latch is confined to the player's main-thread callbacks and its viewModelScope jobs, so a plain field is correct. An atomic would imply cross-thread use that does not exist, and would not address the ordering, which is the actual property. flushPlaybackSnapshotForSwitchOrExit had a second version of the same problem. It read the position to decide completion, then saveWatchProgress read it again to persist, with a stop scrobble dispatched in between and the player not yet stopped. currentPlaybackWriteSnapshot resolves position and duration once, carrying the guards saveWatchProgress already applied. EpisodeCompletionLatchTest pins the invariant the switch broke: a claim, a reset, then a claim that still succeeds. It covers the latch, not the call sites, so a device run is still needed to confirm the controller claims and resets in the right places. Switch episodes during the outro, let the next episode play to its end, confirm it is marked watched.
f096d76 to
074dc90
Compare
|
Rebased onto current dev for the new profileId parameter in Also folded the handoff fix into commit 2, so we're back to 3 commits. The completion latch and ending-segment bound are correctness fixes. |
2 participants
Summary
Fix the provider/local progress mismatch, and align local exit completion with the provider's 80% behavior when the remaining runtime allows it.
Tracker behavior is unchanged. Local completion behavior changes.
Exit completion rule:
The 80% floor applies before any of the qualifications. A credits segment means
outro,edormixed-edfrom the skip-intro providers.Maintainer decision requested. This changes local completion policy, not tracker behavior. The rule runs in the player, so it applies to every exit including movies and users without Trakt or Simkl. Please explicitly approve that scope.
This is not a fixed "less than 20% remaining" threshold. A percentage ignores runtime: 85% of a 22 minute episode leaves three minutes, 85% of a 60 minute episode leaves nine. Four minutes remaining means the same thing on both, which is what a flat threshold cannot express.
What changed
mergeEpisodeProgressWithRetainedLocalretains in-progress local entries the provider has no key for. Provider entries win; completed local entries are excluded.getAllEpisodeProgressseeds its local source so the DataStore read does not gate the combined projection.PlaybackCompletionRulesdecides exit completion.isTerminalEndingSegmentaccepts a credits segment only when it both ends near the file end and is short enough to be credits; open-ended and malformed intervals are rejected.leavesCurrentItemdistinguishes a real exit from same-item teardown, source and engine switches, and the external-player handoff.EpisodeCompletionLatchandcurrentPlaybackWriteSnapshotmake the completion claim and the position it is based on resolve before the write coroutine can suspend.PR type
Why
Exiting an episode between 80% and 90% left it neither in progress nor watched until history synced.
Tracker completion starts at 80%, while local completion used
WatchProgress.COMPLETED_THRESHOLDat 90%.flushPlaybackSnapshotForSwitchOrExitused the tracker threshold for the stop scrobble and the local 90% threshold for local completion, so an exit between the two marked watched remotely while keeping the item in progress locally.The episode card had a second cause.
getAllEpisodeProgressreads only provider progress when one is connected, so the locally saved position was not read back once the provider removed its resume point. Continue Watching already retained local progress; the episode path did not.Issue or approval
Fixes #3348
b580d7e7achangedWatchProgress.COMPLETED_THRESHOLDfrom 85% to 90%. This PR leaves it at 90% and adds an earlier completion path above an 80% floor for short remaining runtimes.Reproduction steps
Requires Trakt or Simkl connected and selected as the watch progress source.
Before this change the episode has no badge and no progress bar, and Continue Watching still offers it until history syncs. After it, the episode either shows a watched badge or a progress bar at the position reached, depending on how much runtime was left.
UI / behavior impact
No layout or design change. The existing watched/progress UI now reflects the corrected state.
A watched episode no longer renders a progress bar alongside the badge. Resume still uses the retained position, so a watched episode can offer Resume at 85% where it previously offered Play.
Policy check
CONTRIBUTING.md.Scope boundaries
The completion rule applies to every configuration; retained-local progress applies only to the first.
Intentionally not changed
WatchProgress.COMPLETED_THRESHOLD, which stays at 90%.No extra polish. No component, layout or design changes, and no unrelated cleanups.
Known limitations
SearchViewModel,HomeViewModelPresentationPipelineandLibraryViewModelcall.first()on the seeded projection and can therefore miss retained entries. Those callers received provider-only data before this PR and receive provider-only data now.Testing
Unit tests: 55 new, in
PlaybackCompletionRulesTest(40),TrackingProgressProjectionTest(7),EpisodeSwitchTargetTest(4) andEpisodeCompletionLatchTest(4), covering the decision table edges, projection merge semantics, episode-switch classification and the completion latch invariant.Full suite: 1225 tests on this branch against 1170 on
devatcf83131c7, 15 failures on each with identical names. The 55-test difference is exactly the new tests.Device: NVIDIA Shield TV Pro (2019), Android 11, Simkl connected and selected as progress source, both engines exercised. Percentages are
providedProgressfrom the scrobble diagnostics.flagisleavesCurrentItem.In these tests, a stop scrobble at or above 80% moved the episode into Simkl's history, so a watched badge in the table is often provider-sourced rather than local.
Not covered: cloud library playback, for lack of a cloud library on the test device. The credits-segment clause is only the deciding rule when credits start with more than four minutes remaining. Across 95 episodes from 16 shows the median outro is 66 s; on four episodes checked against real runtimes, credits began at 95-97% of runtime.
Supporting evidence, from an earlier run before the latch and credits-length changes. The 85.01% row is the reported bug; that card was blank before this change.
Screenshots / Video
Not a UI change.
Breaking changes
None.
Linked issues
Fixes #3348