From 92063bd58fe60303828f4e5027779e2660b8bf35 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Tue, 9 Jun 2026 12:09:26 +0200 Subject: [PATCH] fix(playback): skip redundant mark-watched close #1287 --- lib/media/media_server_client.dart | 28 +++++++++++ lib/services/external_player_service.dart | 6 ++- lib/services/jellyfin_client.dart | 7 +++ lib/services/offline_watch_sync_service.dart | 7 ++- lib/services/playback_progress_tracker.dart | 9 ++-- lib/services/plex_client.dart | 6 +++ .../external_player_service_test.dart | 35 +++++++++++++- .../offline_watch_sync_service_test.dart | 29 +++++++++++ .../playback_progress_tracker_test.dart | 48 +++++++++++++++++++ 9 files changed, 168 insertions(+), 7 deletions(-) diff --git a/lib/media/media_server_client.dart b/lib/media/media_server_client.dart index 51c92884..b143838c 100644 --- a/lib/media/media_server_client.dart +++ b/lib/media/media_server_client.dart @@ -5,6 +5,7 @@ import '../services/playback_initialization_types.dart'; import '../utils/app_logger.dart'; import '../utils/media_server_http_client.dart' show AbortController, MediaServerResponse; import '../utils/external_ids.dart'; +import '../utils/watch_state_notifier.dart'; import 'download_resolution.dart'; import 'ids.dart'; import 'library_filter_result.dart'; @@ -475,6 +476,17 @@ abstract class MediaServerClient { /// one and returns a fixed 0.9. double get watchedThreshold; + /// Whether a playback-stopped report past [watchedThreshold] already marks + /// the item played server-side. When true, in-player auto-scrobble must NOT + /// also call [markWatched]: the server marks it played from the stop report, + /// and the extra `/UserPlayedItems` toggle double-scrobbles through + /// integrations that watch both the played-state change and the + /// playback-stop — e.g. Jellyfin's Trakt plugin fires once on `TogglePlayed` + /// and again on `PlayedToCompletion` (#1287). Plex returns false: its + /// timeline stop doesn't reliably mark watched without an active play + /// session, so the explicit call is still required. + bool get marksWatchedOnPlaybackStopped; + /// First playback signal for [itemId]. Plex sends a `/:/timeline?state=playing` /// heartbeat; Jellyfin opens a `/Sessions/Playing` session row. Subsequent /// ticks must call [reportPlaybackProgress] (Jellyfin distinguishes session @@ -573,6 +585,22 @@ extension MediaServerClientScope on MediaServerClient { ScopedMediaServerClient(:final scopedServerId) => scopedServerId, _ => serverId, }; + + /// Mark [item] watched because it crossed [watchedThreshold] during playback, + /// when a playback-stopped report is/was also sent for the same playback. + /// Backends that mark played from the stop report + /// ([marksWatchedOnPlaybackStopped]) only emit the local watch event — + /// issuing [markWatched] too would double-scrobble via the Jellyfin Trakt + /// plugin (#1287). The local event still keeps the UI and Plezy's own Trakt + /// sync (which key on `watched` events, not progress) in sync; the stop + /// report syncs the server. + Future markWatchedFromPlaybackStop(MediaItem item) async { + if (marksWatchedOnPlaybackStopped) { + WatchStateNotifier().notifyWatched(item: item, isNowWatched: true, cacheServerId: cacheServerId); + } else { + await markWatched(item); + } + } } /// Cache-aware fetch helpers shared by both backends so the offline-first / diff --git a/lib/services/external_player_service.dart b/lib/services/external_player_service.dart index 771551cd..80980925 100644 --- a/lib/services/external_player_service.dart +++ b/lib/services/external_player_service.dart @@ -208,7 +208,11 @@ class ExternalPlayerService { if (position.inMilliseconds / duration.inMilliseconds >= client.watchedThreshold) { try { - await client.markWatched(metadata); + // reportPlaybackStopped above marks the item played on backends that + // support it (Jellyfin); markWatchedFromPlaybackStop then only emits the + // local watch event there to avoid double-scrobbling via the Trakt + // plugin (#1287). Plex still issues the explicit server call. + await client.markWatchedFromPlaybackStop(metadata); unawaited(TrackerCoordinator.instance.markWatched(metadata, client)); } catch (e) { appLogger.w('Failed to mark external playback watched for ${metadata.id}', error: e); diff --git a/lib/services/jellyfin_client.dart b/lib/services/jellyfin_client.dart index 9637ad20..f5c68a45 100644 --- a/lib/services/jellyfin_client.dart +++ b/lib/services/jellyfin_client.dart @@ -242,6 +242,13 @@ class JellyfinClient @override double get watchedThreshold => 0.9; + /// Jellyfin marks an item played from `/Sessions/Playing/Stopped` itself + /// (server `MaxResumePct`, default 90%), so the in-player auto-scrobble must + /// not also `POST /UserPlayedItems` — that double-scrobbles via the Trakt + /// plugin (#1287). Manual mark-watched still hits `/UserPlayedItems`. + @override + bool get marksWatchedOnPlaybackStopped => true; + @override void close() => _http.close(); diff --git a/lib/services/offline_watch_sync_service.dart b/lib/services/offline_watch_sync_service.dart index 5e9f225c..26ab2612 100644 --- a/lib/services/offline_watch_sync_service.dart +++ b/lib/services/offline_watch_sync_service.dart @@ -583,9 +583,12 @@ class OfflineWatchSyncService extends ChangeNotifier { ); } - // If progress exceeded threshold, also mark as watched. + // If progress exceeded threshold, also mark as watched. On backends + // that mark played from the stopped report above (Jellyfin) this only + // emits the local watch event — an explicit markWatched would + // double-scrobble via the Trakt plugin (#1287). if (action.shouldMarkWatched) { - await client.markWatched(item); + await client.markWatchedFromPlaybackStop(item); await TrackerCoordinator.instance.markWatched(item, client); } break; diff --git a/lib/services/playback_progress_tracker.dart b/lib/services/playback_progress_tracker.dart index 0250942c..83c77dfb 100644 --- a/lib/services/playback_progress_tracker.dart +++ b/lib/services/playback_progress_tracker.dart @@ -328,9 +328,12 @@ class PlaybackProgressTracker { if (percent >= threshold) { _scrobbled = true; try { - // The neutral markWatched(MediaItem) call emits the watched event - // through WatchStateNotifier itself, so no extra notify here. - await c.markWatched(metadata); + // Backends that mark the item played from the playback-stopped report + // (Jellyfin) only emit the local watch event here — an explicit + // markWatched would double-scrobble via the Trakt plugin (#1287). + // Plex still issues the server call. Either path emits the watched + // event through WatchStateNotifier, so no extra notify is needed. + await c.markWatchedFromPlaybackStop(metadata); appLogger.d( 'Scrobbled ${metadata.id} (${(percent * 100).toStringAsFixed(0)}% >= ${(threshold * 100).toStringAsFixed(0)}%)', ); diff --git a/lib/services/plex_client.dart b/lib/services/plex_client.dart index 1b186125..1f88683d 100644 --- a/lib/services/plex_client.dart +++ b/lib/services/plex_client.dart @@ -4058,6 +4058,12 @@ class PlexClient @override double get watchedThreshold => watchedThresholdPercent / 100.0; + /// Plex's `/:/timeline?state=stopped` doesn't reliably mark watched without + /// an active play session, so the in-player auto-scrobble still issues the + /// explicit `markWatched` (`/:/scrobble`). See [marksWatchedOnPlaybackStopped]. + @override + bool get marksWatchedOnPlaybackStopped => false; + @override Map get streamHeaders => Map.unmodifiable(config.headers); diff --git a/test/services/external_player_service_test.dart b/test/services/external_player_service_test.dart index 7833c562..f1fad645 100644 --- a/test/services/external_player_service_test.dart +++ b/test/services/external_player_service_test.dart @@ -13,20 +13,29 @@ import 'package:plezy/services/multi_server_manager.dart'; import 'package:plezy/services/offline_watch_sync_service.dart'; class _RecordingClient implements MediaServerClient { + _RecordingClient({this.backend = MediaBackend.plex}); + bool failStart = false; bool failStop = false; final started = <({int positionMs, int? durationMs})>[]; final stopped = <({int positionMs, int? durationMs})>[]; + final watched = []; @override ServerId get serverId => ServerId('srv'); @override - MediaBackend get backend => MediaBackend.plex; + final MediaBackend backend; @override double get watchedThreshold => 0.9; + // Mirror the real clients: Jellyfin marks played from the stopped report, so + // the external-player completion path emits only the local watch event + // (#1287); Plex needs the explicit markWatched. + @override + bool get marksWatchedOnPlaybackStopped => backend == MediaBackend.jellyfin; + @override Future reportPlaybackStarted({ required String itemId, @@ -55,6 +64,11 @@ class _RecordingClient implements MediaServerClient { if (failStop) throw StateError('stop failed'); } + @override + Future markWatched(MediaItem item) async { + watched.add(item.id); + } + @override dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); } @@ -138,5 +152,24 @@ void main() { expect(client.started, [(positionMs: 100000, durationMs: 100000)]); expect(client.stopped, [(positionMs: 100000, durationMs: 100000)]); + expect(client.watched, ['item-1']); + }); + + test('Android external completion on Jellyfin marks watched via the stop report, not markWatched (#1287)', () async { + final client = _RecordingClient(backend: MediaBackend.jellyfin); + + await ExternalPlayerService.reportAndroidExternalProgressForTesting( + positionMs: null, + durationMs: 100000, + playbackCompleted: true, + metadata: _item(durationMs: 100000), + client: client, + ); + + // The stopped report at full duration marks it played server-side… + expect(client.stopped, [(positionMs: 100000, durationMs: 100000)]); + // …so the explicit markWatched is skipped — issuing it would double-scrobble + // through the Jellyfin Trakt plugin. + expect(client.watched, isEmpty); }); } diff --git a/test/services/offline_watch_sync_service_test.dart b/test/services/offline_watch_sync_service_test.dart index 03aed3d2..cdd2e337 100644 --- a/test/services/offline_watch_sync_service_test.dart +++ b/test/services/offline_watch_sync_service_test.dart @@ -77,6 +77,12 @@ class _RecordingMediaClient implements MediaServerClient { @override double get watchedThreshold => 0.9; + // Mirror the real clients: Jellyfin marks played from the stopped report, so + // the auto-scrobble path emits only the local watch event (#1287); Plex needs + // the explicit markWatched. + @override + bool get marksWatchedOnPlaybackStopped => backend == MediaBackend.jellyfin; + @override void close() {} @@ -407,6 +413,29 @@ void main() { expect(client.watched, ['42']); expect(await svc.getPendingSyncCount(), 0); }); + + test('completed Jellyfin offline progress marks watched via the stop report, not markWatched (#1287)', () async { + final (svc: svc, db: db, mgr: mgr) = _makeService(); + addTearDown(() async { + svc.dispose(); + mgr.dispose(); + await db.close(); + }); + + final client = _RecordingMediaClient(serverId: ServerId('srv'), backend: MediaBackend.jellyfin); + mgr.debugRegisterClientForTesting(client); + await svc.queueProgressUpdate(serverId: ServerId('srv'), itemId: '42', viewOffset: 95000, duration: 100000); + + await svc.syncPendingItems(); + + // The stopped report at full duration marks the item played server-side… + expect(client.stopped, hasLength(1)); + expect(client.stopped.single.positionMs, 100000); + // …so the offline replay must NOT also call markWatched — that would + // double-scrobble through the Jellyfin Trakt plugin. + expect(client.watched, isEmpty); + expect(await svc.getPendingSyncCount(), 0); + }); }); // ============================================================ diff --git a/test/services/playback_progress_tracker_test.dart b/test/services/playback_progress_tracker_test.dart index 71360360..2e860bf5 100644 --- a/test/services/playback_progress_tracker_test.dart +++ b/test/services/playback_progress_tracker_test.dart @@ -116,6 +116,11 @@ class _FakePlexClient implements PlexClient { @override double get watchedThreshold => thresholdPercent / 100.0; + /// Plex relies on the explicit markWatched call (no auto-mark from the stop + /// report), so the scrobble path hits [markWatched]. + @override + bool get marksWatchedOnPlaybackStopped => false; + /// (ratingKey, time, state, duration) tuples for every updateProgress call. final List<({String ratingKey, int time, String state, int? duration})> updateProgressCalls = []; @@ -264,6 +269,17 @@ class _DelayedStartClient extends _FakePlexClient { } } +/// Jellyfin-style backend: the playback-stopped report marks the item played +/// server-side, so the in-player scrobble path must emit only the local watch +/// event and skip the explicit server mark (#1287). +class _StopMarksWatchedClient extends _FakePlexClient { + @override + bool get marksWatchedOnPlaybackStopped => true; + + @override + ServerId get serverId => ServerId('srv'); +} + const Object _defaultServerId = Object(); MediaItem _meta({String ratingKey = '42', Object? serverId = _defaultServerId, String? type = 'movie'}) => MediaItem( @@ -659,6 +675,35 @@ void main() { expect(client.markWatchedCalls, ['42']); }); + test('backend that marks watched on stop skips the explicit server mark (#1287)', () async { + // Jellyfin: /Sessions/Playing/Stopped marks the item played server-side, + // so an explicit markWatched here would double-scrobble via the Trakt + // plugin. The local watch event must still fire (UI + Plezy's own Trakt + // sync, which key on `watched` events, not progress). + final client = _StopMarksWatchedClient(); + final player = _FakePlayer(position: const Duration(seconds: 95), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker( + client: client, + metadata: _meta(ratingKey: '42'), + player: player, + isOffline: false, + ); + addTearDown(tracker.dispose); + + final watched = []; + final sub = WatchStateNotifier() + .forItem('42') + .where((e) => e.changeType == WatchStateChangeType.watched) + .listen(watched.add); + addTearDown(sub.cancel); + + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + expect(client.markWatchedCalls, isEmpty); + expect(watched, hasLength(1)); + }); + test('respects a custom server threshold (e.g. 80%)', () async { // 81% >= 80%, but < 90% default. final client = _FakePlexClient(thresholdPercent: 80); @@ -964,6 +1009,9 @@ class _ScrobblePreciseClient implements PlexClient { @override double get watchedThreshold => thresholdPercent / 100.0; + @override + bool get marksWatchedOnPlaybackStopped => false; + bool failScrobbleFirstTime; int markWatchedAttempts = 0; int markWatchedSuccesses = 0;