diff --git a/lib/database/app_database.dart b/lib/database/app_database.dart index bcf8ccfd..b4d48f4f 100644 --- a/lib/database/app_database.dart +++ b/lib/database/app_database.dart @@ -984,6 +984,37 @@ class AppDatabase extends _$AppDatabase { }); } + /// Drop queued `progress` rows for one item, leaving `watched`/`unwatched` + /// rows alone. Returns how many were removed. + /// + /// The mirror of the purge [insertWatchAction] performs: a terminal watch + /// state written straight to the server (the online path, which queues + /// nothing) also supersedes any progress still waiting to replay. Without + /// it, [getPendingWatchActions] hands back the older progress row — it + /// orders by `createdAt` — and replaying it rewrites the resume position the + /// mark just cleared, pinning the item to Continue Watching (#1812). + /// + /// Progress queued *after* a mark is a genuine rewatch and is not affected: + /// this only runs at the moment the mark lands. + Future deleteQueuedProgressForItem({ + String? profileId, + required ServerId serverId, + String? clientScopeId, + required String ratingKey, + }) { + return _runPendingMutation(() async { + final globalKey = buildGlobalKey(ServerId(serverId), ratingKey); + return (delete(offlineWatchProgress)..where( + (t) => + t.globalKey.equals(globalKey) & + _nullableTextPredicate(t.profileId, profileId) & + _nullableTextPredicate(t.clientScopeId, clientScopeId) & + t.actionType.equals(OfflineActionType.progress.id), + )) + .go(); + }); + } + /// Delete a specific watch action after successful sync Future deleteWatchAction(int id) { return _runPendingMutation(() async { diff --git a/lib/media/media_server_client.dart b/lib/media/media_server_client.dart index 6cf641d0..8e808abe 100644 --- a/lib/media/media_server_client.dart +++ b/lib/media/media_server_client.dart @@ -348,6 +348,14 @@ abstract class MediaServerClient { /// emission (and tracker fan-out); the offline sync replay calls this /// directly precisely because the event already fired when the action was /// queued. + /// + /// Postcondition: once this completes the backend must no longer treat + /// [item] as resumable, so it cannot come back from [fetchContinueWatching]. + /// Plex gets this for free (PMS filters watched items out of its on-deck + /// hub). MediaBrowser derives Continue Watching membership purely from + /// `UserData.PlaybackPositionTicks > 0`, so its implementation must ensure + /// the resume position is cleared rather than assume the played flag did it + /// (#1812). Future markWatched(MediaItem item); /// Mark [item] as unwatched. Transport only — see [markWatched]. diff --git a/lib/providers/discover_provider.dart b/lib/providers/discover_provider.dart index 12447144..f82181dc 100644 --- a/lib/providers/discover_provider.dart +++ b/lib/providers/discover_provider.dart @@ -489,15 +489,29 @@ class DiscoverProvider extends ChangeNotifier with DisposableChangeNotifierMixin } if (event.changeType == WatchStateChangeType.removedFromContinueWatching) { - final remaining = _onDeck.where((item) => item.id != event.itemId).toList(); - if (remaining.length != _onDeck.length) { - _onDeck = remaining; - safeNotifyListeners(); - } + _evictFromOnDeck((item) => item.id == event.itemId); + } else if (event.changeType == WatchStateChangeType.watched || + (event.changeType == WatchStateChangeType.progressUpdate && event.isNowWatched == true)) { + // Finished items have no business in Continue Watching, so drop the row + // now instead of waiting a round trip for the refetch below to confirm + // it. Marking a season or show watched takes its on-deck episode with + // it, matching the parent-aware filter this subscription uses — the + // series' successor comes back from the refetch (#1812). + _evictFromOnDeck( + (item) => item.id == event.itemId || item.parentId == event.itemId || item.grandparentId == event.itemId, + ); } unawaited(refreshContinueWatching()); } + void _evictFromOnDeck(bool Function(MediaItem item) matches) { + final remaining = _onDeck.where((item) => !matches(item)).toList(); + if (remaining.length == _onDeck.length) return; + _onDeck = remaining; + safeNotifyListeners(); + unawaited(_syncSystemShelf(_onDeck)); + } + /// Everything on screen: the Continue Watching row plus every hub row. Iterable get _visibleItems => _onDeck.followedBy(_hubs.expand((hub) => hub.items)); diff --git a/lib/services/jellyfin_client/parts/watch_state.dart b/lib/services/jellyfin_client/parts/watch_state.dart index 17a85477..2173861f 100644 --- a/lib/services/jellyfin_client/parts/watch_state.dart +++ b/lib/services/jellyfin_client/parts/watch_state.dart @@ -1,10 +1,41 @@ part of '../../jellyfin_client.dart'; mixin _JellyfinWatchStateMethods on _JellyfinClientInternals { + /// Marking played normally zeroes `UserData.PlaybackPositionTicks` server-side, + /// which is what drops the row from Continue Watching — membership on this API + /// is derived from the position alone, never from `Played` (verified on + /// Jellyfin 10.11.10). Relying on that side effect is not enough: a stale + /// playback report replayed from the offline queue, another client, or a + /// server-side plugin can leave `Played` set *and* a position behind, and the + /// row is then pinned to Continue Watching forever (#1812). + /// + /// So assert the postcondition instead of assuming it, using the + /// `UserItemDataDto` the mark already returns. The follow-up write costs a + /// request only when the invariant is actually broken. + /// + /// Folders (series/season) report their own position as 0 while the server + /// resets their children recursively, so nothing extra is owed here. @override Future markWatched(MediaItem item) async { final response = await _http.post(paths.playedItem(item.id), queryParameters: {'userId': connection.userId}); throwIfHttpError(response); + + final data = response.data; + final positionMs = data is Map ? jellyfinTicksToMs(data['PlaybackPositionTicks']) : null; + if (positionMs == null || positionMs <= 0) return; + + appLogger.d('JellyfinClient: ${item.id} stayed resumable after mark-played; clearing its resume position'); + await _clearResumePosition(item.id); + } + + /// Drop [itemId]'s resume bookmark without touching its played flag. + Future _clearResumePosition(String itemId) async { + final response = await _http.post( + paths.userItemData(itemId), + queryParameters: {'userId': connection.userId}, + body: {'PlaybackPositionTicks': 0}, + ); + throwIfHttpError(response); } @override diff --git a/lib/services/media_browser_paths.dart b/lib/services/media_browser_paths.dart index c3088d6a..6091b046 100644 --- a/lib/services/media_browser_paths.dart +++ b/lib/services/media_browser_paths.dart @@ -34,6 +34,18 @@ class MediaBrowserPaths { String playedItem(String itemId) => dialect.requiresUserScopedItemRoutes ? '$_user/PlayedItems/${_id(itemId)}' : '/UserPlayedItems/${_id(itemId)}'; + /// Per-user playback-state write. `POST` with `{"PlaybackPositionTicks": 0}` + /// clears the resume bookmark while leaving `Played` untouched (verified on + /// Jellyfin 10.11.10 for both spellings). + /// + /// Continue Watching membership on this API is derived purely from + /// `UserData.PlaybackPositionTicks > 0` — `Played` is not consulted — so this + /// is the only route that can guarantee a finished item stops being + /// resumable. See [MediaServerClient.markWatched]. + String userItemData(String itemId) => dialect.requiresUserScopedItemRoutes + ? '$_user/Items/${_id(itemId)}/UserData' + : '/UserItems/${_id(itemId)}/UserData'; + /// Favourite flag write route (`POST` to add, `DELETE` to remove). String favoriteItem(String itemId) => dialect.requiresUserScopedItemRoutes ? '$_user/FavoriteItems/${_id(itemId)}' diff --git a/lib/services/offline_watch_sync_service.dart b/lib/services/offline_watch_sync_service.dart index cc972c1d..4fd02cfe 100644 --- a/lib/services/offline_watch_sync_service.dart +++ b/lib/services/offline_watch_sync_service.dart @@ -86,7 +86,48 @@ class OfflineWatchSyncService extends ChangeNotifier { /// silently drops local watch progress. static const int maxSyncAttempts = 5; - OfflineWatchSyncService({required this._database, required this._serverManager}); + StreamSubscription? _watchStateSubscription; + + OfflineWatchSyncService({required this._database, required this._serverManager}) { + _watchStateSubscription = WatchStateNotifier().stream.listen(_onWatchStateChanged); + } + + /// A terminal watch state just landed, so any progress still queued for that + /// item is stale and must not replay. + /// + /// [AppDatabase.insertWatchAction] already purges the queue when the mark is + /// itself queued (offline). The online path writes straight to the server and + /// queues nothing, so without this the older progress row survives and + /// [syncPendingItems] later rewrites the resume position the mark cleared — + /// on MediaBrowser that pins the item to Continue Watching for good (#1812). + /// Plex is unaffected in practice (PMS discards a replay whose `updated` + /// timestamp is stale) but the queue entry is meaningless there too. + /// + /// Progress recorded *after* the mark is a genuine rewatch: it is queued + /// later, so it is never touched here. + void _onWatchStateChanged(WatchStateEvent event) { + if (_isShutDown) return; + if (event.changeType != WatchStateChangeType.watched && event.changeType != WatchStateChangeType.unwatched) { + return; + } + unawaited(_discardQueuedProgress(ServerId(event.serverId), event.itemId)); + } + + Future _discardQueuedProgress(ServerId serverId, String itemId) async { + try { + final removed = await _database.deleteQueuedProgressForItem( + profileId: _activeProfileId, + serverId: serverId, + clientScopeId: await _clientScopeIdForItem(serverId, itemId), + ratingKey: itemId, + ); + if (removed == 0) return; + appLogger.d('Dropped $removed superseded queued progress action(s) for $serverId:$itemId'); + notifyListeners(); + } catch (e) { + appLogger.w('Failed to drop superseded queued progress for $serverId:$itemId', error: e); + } + } /// Whether a sync is currently in progress bool get isSyncing => _isSyncing; @@ -834,6 +875,8 @@ class OfflineWatchSyncService extends ChangeNotifier { @override void dispose() { _isShutDown = true; + _watchStateSubscription?.cancel(); + _watchStateSubscription = null; if (_offlineModeSource != null && _offlineModeListener != null) { _offlineModeSource!.removeListener(_offlineModeListener!); } diff --git a/test/providers/discover_provider_test.dart b/test/providers/discover_provider_test.dart index 2964aff4..f96ffb61 100644 --- a/test/providers/discover_provider_test.dart +++ b/test/providers/discover_provider_test.dart @@ -415,6 +415,66 @@ void main() { expect(aggregation.hubCalls, hubCallsBefore); }); + test('watched event drops the row immediately, before the refetch answers', () async { + aggregation.onDeckResult = () => [_item('ep-1'), _item('ep-2')]; + await provider.load(); + + // A finished item has no business on the shelf, so the row must go now + // rather than a round trip later — and it must not come back if the + // backend is still returning it (#1812). + var sawImmediateRemoval = false; + provider.addListener(() { + if (provider.onDeck.length == 1 && provider.onDeck.single.id == 'ep-2') { + sawImmediateRemoval = true; + } + }); + aggregation.onDeckResult = () => [_item('ep-2')]; + + WatchStateNotifier().notifyWatched(item: _item('ep-1')); + await pumpEventQueue(); + + expect(sawImmediateRemoval, isTrue); + expect(provider.onDeck.map((i) => i.id), ['ep-2']); + }); + + test('marking a show watched drops its on-deck episode', () async { + aggregation.onDeckResult = () => [_item('ep-1', parentId: 'season-1', grandparentId: 'show-1'), _item('ep-2')]; + await provider.load(); + aggregation.onDeckResult = () => [_item('ep-2')]; + + WatchStateNotifier().notifyWatched(item: _item('show-1', kind: MediaKind.show)); + await pumpEventQueue(); + + expect(provider.onDeck.map((i) => i.id), ['ep-2']); + }); + + test('threshold-crossing progress drops the row too', () async { + aggregation.onDeckResult = () => [_item('ep-1'), _item('ep-2')]; + await provider.load(); + aggregation.onDeckResult = () => [_item('ep-2')]; + + WatchStateNotifier().notifyProgress(item: _item('ep-1'), viewOffset: 95000, duration: 100000); + await pumpEventQueue(); + + expect(provider.onDeck.map((i) => i.id), ['ep-2']); + }); + + test('unwatched event evicts nothing', () async { + aggregation.onDeckResult = () => [_item('ep-1'), _item('ep-2')]; + await provider.load(); + + var sawShorterList = false; + provider.addListener(() { + if (provider.onDeck.length < 2) sawShorterList = true; + }); + + WatchStateNotifier().notifyWatched(item: _item('ep-1'), isNowWatched: false); + await pumpEventQueue(); + + expect(sawShorterList, isFalse); + expect(provider.onDeck.map((i) => i.id), ['ep-1', 'ep-2']); + }); + test('deletion drops the item from on-deck and hubs, then refreshes continue watching only', () async { aggregation.onDeckResult = () => [_item('ep-1'), _item('ep-2')]; aggregation.hubsResult = () => [ diff --git a/test/services/jellyfin_client_emby_dialect_test.dart b/test/services/jellyfin_client_emby_dialect_test.dart index c67b3d91..277bb696 100644 --- a/test/services/jellyfin_client_emby_dialect_test.dart +++ b/test/services/jellyfin_client_emby_dialect_test.dart @@ -1356,6 +1356,82 @@ void main() { }); }); + group('MediaBrowser mark-watched clears a surviving resume position', () { + // Continue Watching membership on this API is `PlaybackPositionTicks > 0` + // alone, so a played item that keeps a position is pinned to the shelf + // forever. markWatched must assert that postcondition, not assume it + // (#1812). + http.Response Function(http.Request) respondWithPosition(int ticks) { + return (request) { + if (request.url.path.endsWith('/UserData')) return http.Response('', 204); + return jsonResponse({'ItemId': 'item-1', 'Played': true, 'PlaybackPositionTicks': ticks}); + }; + } + + test('Jellyfin clears it through the unprefixed user-data route', () async { + final requests = _RequestCapture(respondWithPosition(12000000000)); + final client = testJellyfinClient(handler: requests.handle); + addTearDown(client.close); + + await client.markWatched(_item(MediaBackend.jellyfin)); + + expect(requests.log, [ + 'POST /UserPlayedItems/item-1?userId=user-1', + 'POST /UserItems/item-1/UserData?userId=user-1', + ]); + expect(jsonDecode(requests.requests.last.body), {'PlaybackPositionTicks': 0}); + }); + + test('Emby clears it through the user-scoped user-data route', () async { + final requests = _RequestCapture(respondWithPosition(12000000000)); + final client = testEmbyClient(handler: requests.handle); + addTearDown(client.close); + + await client.markWatched(_item(MediaBackend.emby)); + + expect(requests.log, [ + 'POST /Users/user-1/PlayedItems/item-1?userId=user-1', + 'POST /Users/user-1/Items/item-1/UserData?userId=user-1', + ]); + expect(jsonDecode(requests.requests.last.body), {'PlaybackPositionTicks': 0}); + }); + + test('a mark that already zeroed the position issues no follow-up write', () async { + // The normal case. A second request on every mark would double the cost + // of the app's most common watch-state write. + final requests = _RequestCapture(respondWithPosition(0)); + final client = testJellyfinClient(handler: requests.handle); + addTearDown(client.close); + + await client.markWatched(_item(MediaBackend.jellyfin)); + + expect(requests.log, ['POST /UserPlayedItems/item-1?userId=user-1']); + }); + + test('a body without UserData is left alone', () async { + // Older servers answer the played toggle with 204 and no DTO; there is + // nothing to assert against, so do not guess. + final requests = _RequestCapture((_) => http.Response('', 204)); + final client = testJellyfinClient(handler: requests.handle); + addTearDown(client.close); + + await client.markWatched(_item(MediaBackend.jellyfin)); + + expect(requests.log, ['POST /UserPlayedItems/item-1?userId=user-1']); + }); + + test('a failed clear surfaces as MediaServerHttpException', () async { + final requests = _RequestCapture((request) { + if (request.url.path.endsWith('/UserData')) return http.Response('nope', 500); + return jsonResponse({'ItemId': 'item-1', 'Played': true, 'PlaybackPositionTicks': 12000000000}); + }); + final client = testJellyfinClient(handler: requests.handle); + addTearDown(client.close); + + await expectLater(client.markWatched(_item(MediaBackend.jellyfin)), throwsA(isA())); + }); + }); + group('MediaBrowser name-pair item fields', () { test('Emby item tags and genres survive the browse path via the name-pair arrays', () async { // Real Emby DTO shape: the plain arrays are absent, the name-pair diff --git a/test/services/offline_watch_sync_service_test.dart b/test/services/offline_watch_sync_service_test.dart index ee649b1b..1e557d1e 100644 --- a/test/services/offline_watch_sync_service_test.dart +++ b/test/services/offline_watch_sync_service_test.dart @@ -604,6 +604,111 @@ void main() { }); }); + // ============================================================ + // Superseded queued progress (#1812) + // ============================================================ + + group('queued progress superseded by a watch-state write', () { + MediaItem itemFor(String id) => + testMediaItem(id: id, backend: MediaBackend.jellyfin, kind: MediaKind.movie, serverId: 'srv'); + + // The online mark writes straight to the server and queues nothing, so + // nothing purges the queue the way insertWatchAction does for the offline + // mark. Replaying the stale row afterwards rewrites the resume position the + // mark cleared, which pins the item to Continue Watching on MediaBrowser. + test('a watched event drops the queued progress row for that item', () async { + final (svc: svc, db: db, mgr: mgr) = _makeService(); + addTearDown(() async { + svc.dispose(); + mgr.dispose(); + await db.close(); + }); + + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '42', viewOffset: 50000, duration: 100000); + expect(await svc.getPendingSyncCount(), 1); + + WatchStateNotifier().notifyWatched(item: itemFor('42')); + await pumpEventQueue(); + + expect(await svc.getPendingSyncCount(), 0); + expect(await db.getLatestWatchAction('srv:42'), isNull); + }); + + test('an unwatched event drops it too', () async { + final (svc: svc, db: db, mgr: mgr) = _makeService(); + addTearDown(() async { + svc.dispose(); + mgr.dispose(); + await db.close(); + }); + + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '42', viewOffset: 50000, duration: 100000); + + WatchStateNotifier().notifyWatched(item: itemFor('42'), isNowWatched: false); + await pumpEventQueue(); + + expect(await svc.getPendingSyncCount(), 0); + }); + + test('other items and queued manual marks are left alone', () async { + final (svc: svc, db: db, mgr: mgr) = _makeService(); + addTearDown(() async { + svc.dispose(); + mgr.dispose(); + await db.close(); + }); + + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '42', viewOffset: 50000, duration: 100000); + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '43', viewOffset: 50000, duration: 100000); + await svc.queueMarkWatched(serverId: ServerId('srv'), itemId: '44'); + + WatchStateNotifier().notifyWatched(item: itemFor('42')); + await pumpEventQueue(); + + expect(await db.getLatestWatchAction('srv:42'), isNull); + expect((await db.getLatestWatchAction('srv:43'))?.actionType, 'progress'); + expect((await db.getLatestWatchAction('srv:44'))?.actionType, 'watched'); + }); + + test('progress recorded after the mark survives — that is a rewatch', () async { + final (svc: svc, db: db, mgr: mgr) = _makeService(); + addTearDown(() async { + svc.dispose(); + mgr.dispose(); + await db.close(); + }); + + WatchStateNotifier().notifyWatched(item: itemFor('42')); + await pumpEventQueue(); + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '42', viewOffset: 5000, duration: 100000); + + expect((await db.getLatestWatchAction('srv:42'))?.viewOffset, 5000); + }); + + test('a superseded row never reaches the server on the next sync', () async { + final (svc: svc, db: db, mgr: mgr) = _makeService(); + addTearDown(() async { + svc.dispose(); + mgr.dispose(); + await db.close(); + }); + svc.setActiveProfileId('p1'); + + final client = _RecordingMediaClient(serverId: ServerId('srv'), backend: MediaBackend.jellyfin); + mgr.debugRegisterClientForTesting(client); + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '42', viewOffset: 50000, duration: 100000); + + WatchStateNotifier().notifyWatched(item: itemFor('42')); + await pumpEventQueue(); + await svc.syncPendingItems(); + + // The whole point: no position write, so the resume bookmark the mark + // cleared stays cleared. + expect(client.stopped, isEmpty); + expect(client.started, isEmpty); + }); + }); + // ============================================================ // getLocalWatchStatus // ============================================================