From 11075ef54a2a2e474da037dc5ee4657761c9a621 Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Thu, 13 Aug 2026 16:39:19 +0600 Subject: [PATCH] fold the ordinal suffix into the share display name derivation --- .../repositories/asset_media.repository.dart | 57 +++++++++---------- .../asset_media_repository_test.dart | 52 ++++++++--------- 2 files changed, 52 insertions(+), 57 deletions(-) diff --git a/mobile/lib/repositories/asset_media.repository.dart b/mobile/lib/repositories/asset_media.repository.dart index ce7c14cd92..14484c0f1b 100644 --- a/mobile/lib/repositories/asset_media.repository.dart +++ b/mobile/lib/repositories/asset_media.repository.dart @@ -140,34 +140,29 @@ class AssetMediaRepository { return '${baseName.isEmpty ? _shareFallbackName(asset) : baseName}-preview.jpg'; } - static String _shareDisplayName(BaseAsset asset, ShareAssetType fileType) => - switch (asset.isVideo ? ShareAssetType.original : fileType) { - ShareAssetType.original => getOriginalShareFilename(asset), - ShareAssetType.preview => _getPreviewFilename(asset), - }; - @visibleForTesting - static String getOrdinalShareDisplayName(String displayName, int occurrence) { - if (occurrence <= 0) { - return displayName; + static String shareDisplayName(BaseAsset asset, ShareAssetType fileType, Map occurrences) { + final name = switch (asset.isVideo ? ShareAssetType.original : fileType) { + ShareAssetType.original => getOriginalShareFilename(asset), + ShareAssetType.preview => _getPreviewFilename(asset), + }; + final occurrence = occurrences.update(name, (count) => count + 1, ifAbsent: () => 0); + if (occurrence == 0) { + return name; } - return '${p.basenameWithoutExtension(displayName)} ($occurrence)${p.extension(displayName)}'; + return '${p.basenameWithoutExtension(name)} ($occurrence)${p.extension(name)}'; } bool _isCancelled(Completer? cancelCompleter) => cancelCompleter?.isCompleted ?? false; - Future<_ShareFile?> _getLocalOriginalShareFile(BaseAsset asset, String localId, int occurrence) async { + Future<_ShareFile?> _getLocalOriginalShareFile(BaseAsset asset, String localId, String displayName) async { final file = await _storageRepository.getFileForAsset(localId); if (file == null) { _log.warning("Local original file not found for sharing: $asset"); return null; } - return ( - file: file, - tempEntity: CurrentPlatform.isIOS ? file : null, - displayName: getOrdinalShareDisplayName(getOriginalShareFilename(asset), occurrence), - ); + return (file: file, tempEntity: CurrentPlatform.isIOS ? file : null, displayName: displayName); } @visibleForTesting @@ -231,14 +226,14 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemoteOriginalShareFile( BaseAsset asset, String remoteId, { - required int occurrence, + required String displayName, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { return _downloadRemoteShareFile( taskId: 'share-original-$remoteId-${DateTime.now().microsecondsSinceEpoch}', url: getOriginalUrlForRemoteId(remoteId, edited: asset.isEdited), - displayName: getOrdinalShareDisplayName(getOriginalShareFilename(asset), occurrence), + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -247,14 +242,14 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemotePreviewShareFile( BaseAsset asset, String remoteId, { - required int occurrence, + required String displayName, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { return _downloadRemoteShareFile( taskId: 'share-preview-$remoteId-${DateTime.now().microsecondsSinceEpoch}', url: getThumbnailUrlForRemoteId(remoteId, type: AssetMediaSize.preview, edited: asset.isEdited), - displayName: getOrdinalShareDisplayName(_getPreviewFilename(asset), occurrence), + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -262,13 +257,13 @@ class AssetMediaRepository { Future<_ShareFile?> _getOriginalShareFile( BaseAsset asset, { - required int occurrence, + required String displayName, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { final localId = asset.localId; if (localId != null && !asset.isEdited) { - return _getLocalOriginalShareFile(asset, localId, occurrence); + return _getLocalOriginalShareFile(asset, localId, displayName); } final remoteId = asset.remoteId; @@ -280,7 +275,7 @@ class AssetMediaRepository { return _getRemoteOriginalShareFile( asset, remoteId, - occurrence: occurrence, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -288,7 +283,8 @@ class AssetMediaRepository { Future<_ShareFile?> _getPreviewShareFile( BaseAsset asset, { - required int occurrence, + required String displayName, + required Map occurrences, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { @@ -297,7 +293,7 @@ class AssetMediaRepository { final remotePreview = await _getRemotePreviewShareFile( asset, remoteId, - occurrence: occurrence, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -308,7 +304,8 @@ class AssetMediaRepository { final localId = asset.localId; if (localId != null) { - return _getLocalOriginalShareFile(asset, localId, occurrence); + // the fallback shares the original file, so it gets an original-style name, not the preview one + return _getLocalOriginalShareFile(asset, localId, shareDisplayName(asset, ShareAssetType.original, occurrences)); } _log.warning("Asset has no local or remote ID for preview sharing: $asset"); @@ -349,19 +346,19 @@ class AssetMediaRepository { } final effectiveFileType = asset.isVideo ? ShareAssetType.original : fileType; - final displayName = _shareDisplayName(asset, fileType); - final occurrence = occurrences.update(displayName, (count) => count + 1, ifAbsent: () => 0); + final displayName = shareDisplayName(asset, fileType, occurrences); final shareFile = switch (effectiveFileType) { ShareAssetType.original => await _getOriginalShareFile( asset, - occurrence: occurrence, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), ShareAssetType.preview => await _getPreviewShareFile( asset, - occurrence: occurrence, + displayName: displayName, + occurrences: occurrences, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), diff --git a/mobile/test/repositories/asset_media_repository_test.dart b/mobile/test/repositories/asset_media_repository_test.dart index c1f4a3d090..5693d96200 100644 --- a/mobile/test/repositories/asset_media_repository_test.dart +++ b/mobile/test/repositories/asset_media_repository_test.dart @@ -3,6 +3,7 @@ import 'dart:io'; import 'package:background_downloader/background_downloader.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/constants/enums.dart'; import 'package:immich_mobile/repositories/asset_media.repository.dart'; import 'package:path/path.dart' as p; @@ -45,39 +46,36 @@ void main() { expect(task.filename, name); }); + }); - test('same-named assets get ordinal names after the first', () async { - final first = buildTask( - 'share-original-remote-1-111', - AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 0), - ); - final second = buildTask( - 'share-original-remote-2-222', - AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 1), - ); - - expect(p.basename(await first.filePath()), 'IMG-0001.jpg'); - expect(p.basename(await second.filePath()), 'IMG-0001 (1).jpg'); - }); - + group('shareDisplayName', () { // receivers flatten attachments into one list, so identical names clobber each other - test('sharing the same asset twice gives the second copy an ordinal name', () async { - final second = buildTask( - 'share-original-remote-1-222222', - AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 1), - ); + test('same-named assets get ordinal names after the first', () { + final first = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'); + final second = TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-0001.jpg'); + final occurrences = {}; - expect(p.basename(await second.filePath()), 'IMG-0001 (1).jpg'); + expect(AssetMediaRepository.shareDisplayName(first, ShareAssetType.original, occurrences), 'IMG-0001.jpg'); + expect(AssetMediaRepository.shareDisplayName(second, ShareAssetType.original, occurrences), 'IMG-0001 (1).jpg'); }); - // the ordinal follows batch order, not download success, so a failed first copy must not rename the survivor - test('a failed first duplicate leaves the survivor with its batch ordinal name', () { - final survivor = buildTask( - 'share-original-remote-2-222', - AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 1), - ); + test('sharing the same asset twice gives the second copy an ordinal name', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'); + final occurrences = {}; - expect(survivor.filename, 'IMG-0001 (1).jpg'); + AssetMediaRepository.shareDisplayName(asset, ShareAssetType.original, occurrences); + + expect(AssetMediaRepository.shareDisplayName(asset, ShareAssetType.original, occurrences), 'IMG-0001 (1).jpg'); + }); + + // the preview fallback shares the original file under an original-style name, which must not + // inherit an ordinal from the preview name counted for the same asset + test('preview and original names are counted separately', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'); + final occurrences = {}; + + expect(AssetMediaRepository.shareDisplayName(asset, ShareAssetType.preview, occurrences), 'IMG-0001-preview.jpg'); + expect(AssetMediaRepository.shareDisplayName(asset, ShareAssetType.original, occurrences), 'IMG-0001.jpg'); }); });