From dca51a752abbe1e95bfedd7b1b7e01aa32a32238 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sun, 12 Jul 2026 01:22:26 +0200 Subject: [PATCH] refactor(downloads): centralize metadata merging --- lib/media/media_item_merge.dart | 14 +++++++ lib/providers/download_provider.dart | 22 ++++------ lib/services/download_manager_service.dart | 16 +------ test/media/media_item_merge_test.dart | 49 ++++++++++++++++++++++ test/providers/download_provider_test.dart | 22 ++++++++++ 5 files changed, 95 insertions(+), 28 deletions(-) create mode 100644 lib/media/media_item_merge.dart create mode 100644 test/media/media_item_merge_test.dart diff --git a/lib/media/media_item_merge.dart b/lib/media/media_item_merge.dart new file mode 100644 index 00000000..f99499de --- /dev/null +++ b/lib/media/media_item_merge.dart @@ -0,0 +1,14 @@ +import 'ids.dart'; +import 'media_item.dart'; + +/// Merge freshly fetched metadata with identity and library context already +/// known by the caller. The fetched item owns descriptive fields, while +/// existing context wins when the backend omits it. +MediaItem mergeFetchedMediaItem({required MediaItem fetched, required ServerId fallbackServerId, MediaItem? existing}) { + return fetched.copyWith( + serverId: existing?.serverId ?? fetched.serverId ?? fallbackServerId, + serverName: existing?.serverName ?? fetched.serverName, + libraryId: fetched.libraryId ?? existing?.libraryId, + libraryTitle: fetched.libraryTitle ?? existing?.libraryTitle, + ); +} diff --git a/lib/providers/download_provider.dart b/lib/providers/download_provider.dart index 8dd6f848..fda54253 100644 --- a/lib/providers/download_provider.dart +++ b/lib/providers/download_provider.dart @@ -5,6 +5,7 @@ import 'package:flutter/foundation.dart'; import '../i18n/strings.g.dart'; import '../media/media_backend.dart'; import '../media/media_item.dart'; +import '../media/media_item_merge.dart'; import '../media/media_item_types.dart'; import '../media/media_kind.dart'; import '../media/media_version.dart'; @@ -1128,7 +1129,8 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin if (!_downloadManager.downloadsSupported) return false; final ownerProfileId = claimForProfileId ?? _requireActiveProfileId(); - final globalKey = metadata.globalKey; + var metadataToStore = metadata.serverId == null ? metadata.copyWith(serverId: client.serverId) : metadata; + final globalKey = metadataToStore.globalKey; // Don't duplicate the physical download. If another profile already owns // the shared row, claiming it makes it visible for the owning profile. @@ -1152,18 +1154,16 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin // Skip the fetch when offline — it would just fail. The partial metadata // from whatever hub/grid invoked the queue is good enough to enqueue; the // actual video URL resolves later when we're back online. - MediaItem metadataToStore = metadata; if (_offlineSource?.isOffline ?? false) { appLogger.d('Offline — using partial metadata for ${metadata.id}'); } else { try { final fullMetadata = await client.fetchItem(metadata.id); if (fullMetadata != null) { - metadataToStore = fullMetadata.copyWith( - serverId: metadata.serverId ?? fullMetadata.serverId, - serverName: metadata.serverName ?? fullMetadata.serverName, - libraryId: fullMetadata.libraryId ?? metadata.libraryId, - libraryTitle: fullMetadata.libraryTitle ?? metadata.libraryTitle, + metadataToStore = mergeFetchedMediaItem( + fetched: fullMetadata, + existing: metadataToStore, + fallbackServerId: client.serverId, ); } } catch (e) { @@ -1253,13 +1253,7 @@ class DownloadProvider extends ChangeNotifier with DisposableChangeNotifierMixin try { final fetched = await client.fetchItem(ratingKey); if (fetched != null) { - final existing = metadata; - metadata = fetched.copyWith( - serverId: existing?.serverId ?? fetched.serverId ?? serverId, - serverName: existing?.serverName ?? fetched.serverName, - libraryId: fetched.libraryId ?? existing?.libraryId, - libraryTitle: fetched.libraryTitle ?? existing?.libraryTitle, - ); + metadata = mergeFetchedMediaItem(fetched: fetched, existing: metadata, fallbackServerId: serverId); context.hydratedMetadataKeys.add(globalKey); fetchedFreshMetadata = true; } diff --git a/lib/services/download_manager_service.dart b/lib/services/download_manager_service.dart index 2ba590f6..d754f046 100644 --- a/lib/services/download_manager_service.dart +++ b/lib/services/download_manager_service.dart @@ -14,6 +14,7 @@ import '../database/download_operations.dart'; import '../media/download_resolution.dart'; import '../media/media_backend.dart'; import '../media/media_item.dart'; +import '../media/media_item_merge.dart'; import '../media/media_item_types.dart'; import '../media/media_kind.dart'; import '../media/media_server_client.dart'; @@ -776,7 +777,7 @@ class DownloadManagerService { try { final fetched = await client.fetchItem(ratingKey); if (fetched != null) { - metadata = _mergeFetchedRepairMetadata(serverId: serverId, cached: cached, fetched: fetched); + metadata = mergeFetchedMediaItem(fallbackServerId: serverId, existing: cached, fetched: fetched); await ApiCache.forBackend(client.backend).pinForOffline(ServerId(client.cacheServerId), metadata.id); } } catch (e) { @@ -792,19 +793,6 @@ class DownloadManagerService { return metadata.serverId == null ? metadata.copyWith(serverId: serverId) : metadata; } - MediaItem _mergeFetchedRepairMetadata({ - required ServerId serverId, - required MediaItem? cached, - required MediaItem fetched, - }) { - return fetched.copyWith( - serverId: cached?.serverId ?? fetched.serverId ?? serverId, - serverName: cached?.serverName ?? fetched.serverName, - libraryId: fetched.libraryId ?? cached?.libraryId, - libraryTitle: fetched.libraryTitle ?? cached?.libraryTitle, - ); - } - Future _backfillArtworkPath(DownloadedMediaItem row, MediaItem metadata) async { final thumbPath = metadata.thumbPath; if (thumbPath == null || thumbPath.isEmpty) return; diff --git a/test/media/media_item_merge_test.dart b/test/media/media_item_merge_test.dart new file mode 100644 index 00000000..16af82c5 --- /dev/null +++ b/test/media/media_item_merge_test.dart @@ -0,0 +1,49 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/media/ids.dart'; +import 'package:plezy/media/media_backend.dart'; +import 'package:plezy/media/media_item.dart'; +import 'package:plezy/media/media_item_merge.dart'; +import 'package:plezy/media/media_kind.dart'; + +void main() { + MediaItem item({String? serverId, String? serverName, String? libraryId, String? libraryTitle}) => MediaItem( + id: 'item', + backend: MediaBackend.plex, + kind: MediaKind.movie, + serverId: serverId, + serverName: serverName, + libraryId: libraryId, + libraryTitle: libraryTitle, + ); + + test('uses the authoritative fallback when both items omit server identity', () { + final merged = mergeFetchedMediaItem(fetched: item(), fallbackServerId: ServerId('fallback')); + + expect(merged.serverId, 'fallback'); + expect(merged.globalKey, 'fallback:item'); + }); + + test('preserves existing identity while preferring fetched library context', () { + final merged = mergeFetchedMediaItem( + fetched: item(serverId: 'fetched', serverName: 'Fetched', libraryId: 'new-lib', libraryTitle: 'New'), + existing: item(serverId: 'existing', serverName: 'Existing', libraryId: 'old-lib', libraryTitle: 'Old'), + fallbackServerId: ServerId('fallback'), + ); + + expect(merged.serverId, 'existing'); + expect(merged.serverName, 'Existing'); + expect(merged.libraryId, 'new-lib'); + expect(merged.libraryTitle, 'New'); + }); + + test('fills missing fetched library context from the existing item', () { + final merged = mergeFetchedMediaItem( + fetched: item(), + existing: item(libraryId: 'old-lib', libraryTitle: 'Old'), + fallbackServerId: ServerId('fallback'), + ); + + expect(merged.libraryId, 'old-lib'); + expect(merged.libraryTitle, 'Old'); + }); +} diff --git a/test/providers/download_provider_test.dart b/test/providers/download_provider_test.dart index 790ae46f..c47683d1 100644 --- a/test/providers/download_provider_test.dart +++ b/test/providers/download_provider_test.dart @@ -563,6 +563,28 @@ void main() { p.dispose(); }); + test('queueDownload applies the client server id before checking existing downloads', () async { + final p = DownloadProvider.forTesting(downloadManager: downloadManager, database: db); + await p.ensureInitialized(); + p.debugSeedState( + downloads: {'srv:1': const DownloadProgress(globalKey: 'srv:1', status: DownloadStatus.completed)}, + metadata: {'srv:1': movie}, + ownedDownloadKeys: const {}, + ); + + final count = await p.queueDownload( + movie.copyWith(serverId: null), + _ScopedTestClient(serverId: ServerId('srv'), scopedServerId: 'srv'), + ); + + expect(count, 1); + expect(p.downloads.keys, ['srv:1']); + expect(p.downloads, isNot(contains('1'))); + expect(await db.getDownloadOwnerKeysForProfile('test-profile'), {'srv:1'}); + + p.dispose(); + }); + test('queueDownload expands an album into its tracks via fetchPlayableDescendants', () async { final album = MediaItem( id: 'album-1',