From 2e1a7130fcaa7bd5f068a3554b9a93c678b2a7a3 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Mon, 17 Aug 2026 01:49:15 +0200 Subject: [PATCH] refactor(downloads): one auto-remove-watched core, trimmed offline-watch provider, shared focus-restore mixin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auto-remove-watched-downloads rule existed twice (DownloadProvider's sweep and OfflineWatchProvider's single-item copy) — the kind/completed/delete/title core now lives once on DownloadProvider, with the watched judgment staying per caller because the offline path fires before metadata can reflect a local mark. OfflineWatchProvider drops five members with no production callers (isSyncing, getPendingSyncCount, isWatched, getViewOffset, getEpisodesWithWatchStatus) plus the sync-service listener that only served them. The downloads screen's three verbatim 'suppressAutoFocus flipped — focus the first item' didUpdateWidget blocks are now one mixin. --- lib/mixins/unsuppress_focus_mixin.dart | 34 ++++++ lib/providers/download_provider.dart | 25 +++- lib/providers/offline_watch_provider.dart | 109 ++---------------- lib/screens/downloads/downloads_screen.dart | 52 +++------ lib/widgets/download_tree_view.dart | 22 +--- .../offline_watch_provider_test.dart | 87 +------------- 6 files changed, 86 insertions(+), 243 deletions(-) create mode 100644 lib/mixins/unsuppress_focus_mixin.dart diff --git a/lib/mixins/unsuppress_focus_mixin.dart b/lib/mixins/unsuppress_focus_mixin.dart new file mode 100644 index 000000000..5ca7e3090 --- /dev/null +++ b/lib/mixins/unsuppress_focus_mixin.dart @@ -0,0 +1,34 @@ +import 'package:flutter/widgets.dart'; + +/// Focuses [firstItemFocusNode] when the owning widget's `suppressAutoFocus` +/// flag flips from true to false — the downloads screen's tab-switch handoff +/// to whichever content widget just became active. +/// +/// Consumers implement [firstItemFocusDebugLabel] and [suppressAutoFocusOf], +/// then hand [firstItemFocusNode] to their first focusable item. +mixin UnsuppressFocusFirstMixin on State { + late final FocusNode firstItemFocusNode = FocusNode(debugLabel: firstItemFocusDebugLabel); + + String get firstItemFocusDebugLabel; + + /// Reads the owning widget's `suppressAutoFocus` flag. + bool suppressAutoFocusOf(T widget); + + @override + void didUpdateWidget(T oldWidget) { + super.didUpdateWidget(oldWidget); + if (suppressAutoFocusOf(oldWidget) && !suppressAutoFocusOf(widget)) { + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted && firstItemFocusNode.canRequestFocus) { + firstItemFocusNode.requestFocus(); + } + }); + } + } + + @override + void dispose() { + firstItemFocusNode.dispose(); + super.dispose(); + } +} diff --git a/lib/providers/download_provider.dart b/lib/providers/download_provider.dart index 29db49b51..40ccb4616 100644 --- a/lib/providers/download_provider.dart +++ b/lib/providers/download_provider.dart @@ -1696,6 +1696,25 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin } } + /// Deletes [globalKey] if it is a completed episode/movie download; returns + /// the deleted item's display title, or null when it is not an auto-remove + /// candidate. + /// + /// Shared core of the auto-remove-watched rule. The watched judgment stays + /// with the caller: the sweep in [autoDeleteWatchedDownloads] trusts server + /// metadata, while [OfflineWatchProvider] fires right after a local + /// mark-watched that metadata cannot reflect yet. + Future deleteWatchedDownloadCandidate(String globalKey, {required String logContext}) async { + final meta = _resolvedMetadata(globalKey); + if (meta == null) return null; + if (!meta.isEpisode && !meta.isMovie) return null; + if (_downloads[globalKey]?.status != DownloadStatus.completed) return null; + + appLogger.i('Auto-deleting $logContext download: ${meta.title} ($globalKey)'); + await deleteDownload(globalKey); + return meta.title ?? t.common.unknown; + } + /// Auto-delete downloaded episodes/movies that are now marked as watched. /// /// Only deletes individual episodes and movies, never show/season containers. @@ -1711,16 +1730,14 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin for (final globalKey in completedKeys) { final meta = _resolvedMetadata(globalKey); if (meta == null) continue; - if (!meta.isEpisode && !meta.isMovie) continue; if (!meta.isWatched) continue; // Don't delete the episode that's currently playing if (activeGlobalKey != null && meta.globalKey == activeGlobalKey) continue; try { - appLogger.i('Auto-deleting watched download: ${meta.title} ($globalKey)'); - await deleteDownload(globalKey); - deletedTitles.add(meta.title ?? t.common.unknown); + final title = await deleteWatchedDownloadCandidate(globalKey, logContext: 'watched'); + if (title != null) deletedTitles.add(title); } catch (e) { appLogger.w('Failed to auto-delete watched download $globalKey: $e'); } diff --git a/lib/providers/offline_watch_provider.dart b/lib/providers/offline_watch_provider.dart index e37ed882e..f27063fc4 100644 --- a/lib/providers/offline_watch_provider.dart +++ b/lib/providers/offline_watch_provider.dart @@ -1,12 +1,12 @@ +import 'dart:async'; + import 'package:flutter/foundation.dart'; import '../media/ids.dart'; import '../i18n/strings.g.dart'; import '../media/episode_collection.dart'; import '../media/media_item.dart'; -import '../media/media_item_types.dart'; import '../mixins/disposable_change_notifier_mixin.dart'; -import '../models/download_models.dart'; import '../services/offline_watch_sync_service.dart'; import '../services/settings_service.dart'; import '../utils/app_logger.dart'; @@ -18,69 +18,13 @@ import '../utils/global_key_utils.dart'; /// Provider for offline watch status UI state. /// /// Provides: -/// - Effective watch status (local changes + cached server data) /// - Offline "OnDeck" calculation for shows /// - Manual mark watched/unwatched while offline class OfflineWatchProvider extends ChangeNotifier with DisposableChangeNotifierMixin { final OfflineWatchSyncService _syncService; final DownloadProvider _downloadProvider; - OfflineWatchProvider({required this._syncService, required this._downloadProvider}) { - _syncService.addListener(_onSyncServiceChanged); - } - - void _onSyncServiceChanged() { - safeNotifyListeners(); - } - - /// Whether a sync is in progress - bool get isSyncing => _syncService.isSyncing; - - /// Get count of pending sync items - Future getPendingSyncCount() => _syncService.getPendingSyncCount(); - - /// Get the effective watch status for a media item. - /// - /// Priority: - /// 1. Local offline action (if exists) - /// 2. Cached server data from API cache - /// 3. Metadata from download provider - /// - /// Returns true if watched, false otherwise. - Future isWatched(String globalKey) async { - final localStatus = await _syncService.getLocalWatchStatus(globalKey); - if (localStatus != null) { - return localStatus; - } - - final metadata = _downloadProvider.getMetadata(globalKey); - if (metadata != null) { - return metadata.isWatched; - } - - return false; - } - - /// Get the effective view offset (resume position) for a media item. - /// - /// Priority: - /// 1. Local offline progress (if exists) - /// 2. Metadata from download provider - /// - /// Returns null if no position is available. - @visibleForTesting - Future getViewOffset(String globalKey) async { - final localOffset = await _syncService.getLocalViewOffset(globalKey); - if (localOffset != null) { - return localOffset; - } - - final localStatus = await _syncService.getLocalWatchStatus(globalKey); - if (localStatus == true) return null; - - final metadata = _downloadProvider.getMetadata(globalKey); - return metadata?.viewOffsetMs; - } + OfflineWatchProvider({required this._syncService, required this._downloadProvider}); /// Get episodes for a show in the shared [sortEpisodesByWatchOrder] order, /// so the offline watch order matches what "download next N" selects @@ -181,33 +125,21 @@ class OfflineWatchProvider extends ChangeNotifier with DisposableChangeNotifierM patchId: WatchPatchId.offlineAction(profileId: queued.profileId, rowId: queued.rowId, revision: queued.revision), ); safeNotifyListeners(); - _autoDeleteIfWatched(serverId, itemId); + unawaited(_autoDeleteIfWatched(serverId, itemId)); } /// Auto-delete a download if the auto-remove setting is enabled. - void _autoDeleteIfWatched(ServerId serverId, String itemId) { + Future _autoDeleteIfWatched(ServerId serverId, String itemId) async { final settings = SettingsService.instanceOrNull; if (settings == null || !settings.read(SettingsService.autoRemoveWatchedDownloads)) return; final globalKey = buildGlobalKey(ServerId(serverId), itemId); - final meta = _downloadProvider.getMetadata(globalKey); - if (meta == null) return; - if (!meta.isEpisode && !meta.isMovie) return; - - final progress = _downloadProvider.downloads[globalKey]; - if (progress?.status != DownloadStatus.completed) return; - - appLogger.i('Auto-deleting locally-watched download: ${meta.title} ($globalKey)'); - _downloadProvider - .deleteDownload(globalKey) - .then( - (_) { - showMainSnackBar(t.messages.autoRemovedWatchedDownload(title: meta.title ?? t.common.unknown)); - }, - onError: (e) { - appLogger.w('Failed to auto-delete locally-watched download $globalKey: $e'); - }, - ); + try { + final title = await _downloadProvider.deleteWatchedDownloadCandidate(globalKey, logContext: 'locally-watched'); + if (title != null) showMainSnackBar(t.messages.autoRemovedWatchedDownload(title: title)); + } catch (e) { + appLogger.w('Failed to auto-delete locally-watched download $globalKey: $e'); + } } /// Mark an item as unwatched while offline. @@ -225,23 +157,4 @@ class OfflineWatchProvider extends ChangeNotifier with DisposableChangeNotifierM ); safeNotifyListeners(); } - - /// Get downloaded episodes for a show with their watch status. - /// - /// Returns a list of (episode, isWatched) pairs. - /// Uses batched database query for efficiency. - Future> getEpisodesWithWatchStatus(String showId) async { - final episodes = _downloadProvider.getDownloadedEpisodesForShow(showId); - if (episodes.isEmpty) return []; - - final watchStatuses = await _resolveEpisodeWatchStatuses(episodes); - - return [for (final episode in episodes) (episode, watchStatuses[episode.globalKey]!)]; - } - - @override - void dispose() { - _syncService.removeListener(_onSyncServiceChanged); - super.dispose(); - } } diff --git a/lib/screens/downloads/downloads_screen.dart b/lib/screens/downloads/downloads_screen.dart index 76003e304..5bb485f23 100644 --- a/lib/screens/downloads/downloads_screen.dart +++ b/lib/screens/downloads/downloads_screen.dart @@ -21,6 +21,7 @@ import '../../services/settings_service.dart'; import '../../widgets/settings_builder.dart'; import '../../utils/global_key_utils.dart'; import '../../mixins/tab_navigation_mixin.dart'; +import '../../mixins/unsuppress_focus_mixin.dart'; import '../../mixins/refreshable.dart'; import '../../utils/media_image_helper.dart'; import '../../utils/platform_detector.dart'; @@ -268,27 +269,13 @@ class _DownloadsGridContent extends StatefulWidget { State<_DownloadsGridContent> createState() => _DownloadsGridContentState(); } -class _DownloadsGridContentState extends State<_DownloadsGridContent> { - final FocusNode _firstItemFocusNode = FocusNode(debugLabel: 'DownloadsGrid_firstItem'); +class _DownloadsGridContentState extends State<_DownloadsGridContent> + with UnsuppressFocusFirstMixin<_DownloadsGridContent> { + @override + String get firstItemFocusDebugLabel => 'DownloadsGrid_firstItem'; @override - void dispose() { - _firstItemFocusNode.dispose(); - super.dispose(); - } - - @override - void didUpdateWidget(_DownloadsGridContent oldWidget) { - super.didUpdateWidget(oldWidget); - // When suppressAutoFocus changes from true to false, focus the first item - if (oldWidget.suppressAutoFocus && !widget.suppressAutoFocus) { - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted && _firstItemFocusNode.canRequestFocus) { - _firstItemFocusNode.requestFocus(); - } - }); - } - } + bool suppressAutoFocusOf(_DownloadsGridContent widget) => widget.suppressAutoFocus; /// Navigate focus to the sidebar void _navigateToSidebar() { @@ -332,7 +319,7 @@ class _DownloadsGridContentState extends State<_DownloadsGridContent> { final item = items[position.index]; return FocusableMediaCard( item: item, - focusNode: position.index == 0 ? _firstItemFocusNode : null, + focusNode: position.index == 0 ? firstItemFocusNode : null, disableScale: position.disableScale, onBack: widget.onBack, isOffline: true, // Downloaded content works without server @@ -392,26 +379,13 @@ class _DownloadedMusicContent extends StatefulWidget { State<_DownloadedMusicContent> createState() => _DownloadedMusicContentState(); } -class _DownloadedMusicContentState extends State<_DownloadedMusicContent> { - final FocusNode _firstItemFocusNode = FocusNode(debugLabel: 'DownloadsMusic_firstItem'); +class _DownloadedMusicContentState extends State<_DownloadedMusicContent> + with UnsuppressFocusFirstMixin<_DownloadedMusicContent> { + @override + String get firstItemFocusDebugLabel => 'DownloadsMusic_firstItem'; @override - void dispose() { - _firstItemFocusNode.dispose(); - super.dispose(); - } - - @override - void didUpdateWidget(_DownloadedMusicContent oldWidget) { - super.didUpdateWidget(oldWidget); - if (oldWidget.suppressAutoFocus && !widget.suppressAutoFocus) { - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted && _firstItemFocusNode.canRequestFocus) { - _firstItemFocusNode.requestFocus(); - } - }); - } - } + bool suppressAutoFocusOf(_DownloadedMusicContent widget) => widget.suppressAutoFocus; Future _playAlbumFrom(List albumTracks, MediaItem track) async { final album = track.parentId; @@ -541,7 +515,7 @@ class _DownloadedMusicContentState extends State<_DownloadedMusicContent> { isFirst: row.isFirst, isLast: row.isLast, showArtist: true, - focusNode: isFirstTrackRow ? _firstItemFocusNode : null, + focusNode: isFirstTrackRow ? firstItemFocusNode : null, onBack: widget.onBack, onTap: () => _playAlbumFrom(row.albumTracks, item), ), diff --git a/lib/widgets/download_tree_view.dart b/lib/widgets/download_tree_view.dart index 871d9807f..bd6c0174d 100644 --- a/lib/widgets/download_tree_view.dart +++ b/lib/widgets/download_tree_view.dart @@ -10,6 +10,7 @@ import '../media/media_kind.dart'; import '../models/download_models.dart'; import '../utils/dialogs.dart'; import '../utils/global_key_utils.dart'; +import '../mixins/unsuppress_focus_mixin.dart'; import 'download_status_icon.dart'; /// Represents a node in the download tree @@ -79,27 +80,14 @@ class DownloadTreeView extends StatefulWidget { State createState() => _DownloadTreeViewState(); } -class _DownloadTreeViewState extends State { +class _DownloadTreeViewState extends State with UnsuppressFocusFirstMixin { final Set _expandedNodes = {}; - final FocusNode _firstItemFocusNode = FocusNode(debugLabel: 'DownloadTreeView_firstItem'); @override - void dispose() { - _firstItemFocusNode.dispose(); - super.dispose(); - } + String get firstItemFocusDebugLabel => 'DownloadTreeView_firstItem'; @override - void didUpdateWidget(DownloadTreeView oldWidget) { - super.didUpdateWidget(oldWidget); - if (oldWidget.suppressAutoFocus && !widget.suppressAutoFocus) { - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted && _firstItemFocusNode.canRequestFocus) { - _firstItemFocusNode.requestFocus(); - } - }); - } - } + bool suppressAutoFocusOf(DownloadTreeView widget) => widget.suppressAutoFocus; @override Widget build(BuildContext context) { @@ -402,7 +390,7 @@ class _DownloadTreeViewState extends State { onDelete: widget.onDelete, onNavigateLeft: widget.onNavigateLeft, onBack: widget.onBack, - rowFocusNode: isFirst ? _firstItemFocusNode : null, + rowFocusNode: isFirst ? firstItemFocusNode : null, autofocus: isFirst && !widget.suppressAutoFocus, pauseAllChildren: _pauseAllChildren, resumeAllChildren: _resumeAllChildren, diff --git a/test/providers/offline_watch_provider_test.dart b/test/providers/offline_watch_provider_test.dart index b80ee2489..8f0a32c76 100644 --- a/test/providers/offline_watch_provider_test.dart +++ b/test/providers/offline_watch_provider_test.dart @@ -44,73 +44,12 @@ void main() { }); group('OfflineWatchProvider', () { - test('initial isSyncing reflects sync service state', () { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - expect(p.isSyncing, isFalse); - expect(syncService.isSyncing, isFalse); - p.dispose(); - }); - - test('getPendingSyncCount delegates to sync service (initially 0)', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - expect(await p.getPendingSyncCount(), 0); - p.dispose(); - }); - - test('isWatched returns false when no local action and no metadata', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - expect(await p.isWatched('srv:absent'), isFalse); - p.dispose(); - }); - - test('getViewOffset returns null when no local progress and no metadata', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - expect(await p.getViewOffset('srv:absent'), isNull); - p.dispose(); - }); - - test('getViewOffset returns null for local progress that crossed watched threshold', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - - await syncService.queueProgressUpdate( - serverId: ServerId('srv'), - itemId: '42', - viewOffset: 95000, - duration: 100000, - ); - - expect(await p.isWatched('srv:42'), isTrue); - expect(await p.getViewOffset('srv:42'), isNull); - - p.dispose(); - }); - test('getNextUnwatchedEpisode returns null for show with no downloads', () async { final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); expect(await p.getNextUnwatchedEpisode('show-123'), isNull); p.dispose(); }); - test('getEpisodesWithWatchStatus returns empty list for show with no downloads', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - expect(await p.getEpisodesWithWatchStatus('show-123'), isEmpty); - p.dispose(); - }); - - test('forwards listener notifications from sync service', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - - var notified = 0; - p.addListener(() => notified++); - - // queueMarkWatched on the sync service notifies its listeners; the - // provider's internal listener forwards via safeNotifyListeners. - await syncService.queueMarkWatched(serverId: ServerId('srv'), itemId: '42'); - expect(notified, greaterThanOrEqualTo(1)); - - p.dispose(); - }); - test('markAsWatched queues an offline action and notifies', () async { final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); @@ -120,9 +59,7 @@ void main() { await p.markAsWatched(serverId: ServerId('srv'), itemId: '50'); // The local watch status now reads as true via the sync service. - expect(await p.isWatched('srv:50'), isTrue); - // At least one notification: from sync service forwarding + provider's - // explicit safeNotifyListeners after queueing. + expect(await syncService.getLocalWatchStatus('srv:50'), isTrue); expect(notified, greaterThanOrEqualTo(1)); p.dispose(); @@ -132,29 +69,9 @@ void main() { final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); await p.markAsUnwatched(serverId: ServerId('srv'), itemId: '60'); - expect(await p.isWatched('srv:60'), isFalse); + expect(await syncService.getLocalWatchStatus('srv:60'), isFalse); p.dispose(); }); - - test('dispose removes the sync service listener', () async { - final p = OfflineWatchProvider(syncService: syncService, downloadProvider: downloadProvider); - - var notified = 0; - p.addListener(() => notified++); - - // Sanity: listener is registered - await syncService.queueMarkWatched(serverId: ServerId('srv'), itemId: '70'); - final preDisposeNotifies = notified; - expect(preDisposeNotifies, greaterThanOrEqualTo(1)); - - p.dispose(); - - // After dispose, sync service notifications should not call our - // listener (provider unsubscribed). Mutating the sync service post- - // dispose must not throw on the provider side. - await syncService.queueMarkUnwatched(serverId: ServerId('srv'), itemId: '70'); - expect(notified, preDisposeNotifies); - }); }); }