diff --git a/mobile/lib/repositories/asset_media.repository.dart b/mobile/lib/repositories/asset_media.repository.dart index ce372f8b2c..43e0d55a48 100644 --- a/mobile/lib/repositories/asset_media.repository.dart +++ b/mobile/lib/repositories/asset_media.repository.dart @@ -128,16 +128,16 @@ class AssetMediaRepository { @visibleForTesting static String getOriginalShareFilename(BaseAsset asset) { - final fallbackName = asset.remoteId ?? asset.localId ?? 'asset'; final hasUsableName = asset.name.replaceAll(_pathSeparators, '').isNotEmpty; - return hasUsableName ? _sanitizeFilename(asset.name) : fallbackName; + return hasUsableName ? _sanitizeFilename(asset.name) : _shareFallbackName(asset); } + static String _shareFallbackName(BaseAsset asset) => asset.remoteId ?? asset.localId ?? 'asset'; + static String _getPreviewFilename(BaseAsset asset) { final sanitizedFilename = _sanitizeFilename(asset.name); final baseName = p.basenameWithoutExtension(sanitizedFilename); - final fallbackName = asset.remoteId ?? asset.localId ?? 'asset'; - return '${baseName.isEmpty ? fallbackName : baseName}-preview.jpg'; + return '${baseName.isEmpty ? _shareFallbackName(asset) : baseName}-preview.jpg'; } static String _shareDisplayName(BaseAsset asset, ShareAssetType fileType) => @@ -146,6 +146,14 @@ class AssetMediaRepository { ShareAssetType.preview => _getPreviewFilename(asset), }; + @visibleForTesting + static String getOrdinalShareDisplayName(String displayName, int occurrence) { + if (occurrence <= 0) { + return displayName; + } + return '${p.basenameWithoutExtension(displayName)} ($occurrence)${p.extension(displayName)}'; + } + @visibleForTesting static Map countShareDisplayNames(List assets, ShareAssetType fileType) { final counts = {}; @@ -158,14 +166,18 @@ class AssetMediaRepository { bool _isCancelled(Completer? cancelCompleter) => cancelCompleter?.isCompleted ?? false; - Future<_ShareFile?> _getLocalOriginalShareFile(BaseAsset asset, String localId) async { + Future<_ShareFile?> _getLocalOriginalShareFile(BaseAsset asset, String localId, int occurrence) 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: getOriginalShareFilename(asset)); + return ( + file: file, + tempEntity: CurrentPlatform.isIOS ? file : null, + displayName: getOrdinalShareDisplayName(getOriginalShareFilename(asset), occurrence), + ); } @visibleForTesting @@ -174,14 +186,13 @@ class AssetMediaRepository { required String url, required Map headers, required String displayName, - required bool isDuplicate, }) { return DownloadTask( taskId: taskId, url: url, headers: headers, - // receiving apps show the shared file's own name, so duplicates keep the task id prefix to stay unique - filename: isDuplicate ? '$taskId-$displayName' : displayName, + // receiving apps show the shared file's own name, so duplicates get an ordinal suffix in the display name + filename: displayName, directory: taskId, baseDirectory: BaseDirectory.temporary, group: kShareDownloadGroup, @@ -193,7 +204,6 @@ class AssetMediaRepository { required String taskId, required String url, required String displayName, - required bool isDuplicate, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { @@ -202,7 +212,6 @@ class AssetMediaRepository { url: url, headers: ApiService.getRequestHeaders(), displayName: displayName, - isDuplicate: isDuplicate, ); final downloader = FileDownloader(); final statusUpdate = await downloader.download( @@ -232,15 +241,14 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemoteOriginalShareFile( BaseAsset asset, String remoteId, { - required bool isDuplicate, + required int occurrence, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { return _downloadRemoteShareFile( taskId: 'share-original-$remoteId-${DateTime.now().microsecondsSinceEpoch}', url: getOriginalUrlForRemoteId(remoteId, edited: asset.isEdited), - displayName: getOriginalShareFilename(asset), - isDuplicate: isDuplicate, + displayName: getOrdinalShareDisplayName(getOriginalShareFilename(asset), occurrence), cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -249,15 +257,14 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemotePreviewShareFile( BaseAsset asset, String remoteId, { - required bool isDuplicate, + required int occurrence, 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: _getPreviewFilename(asset), - isDuplicate: isDuplicate, + displayName: getOrdinalShareDisplayName(_getPreviewFilename(asset), occurrence), cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -265,13 +272,13 @@ class AssetMediaRepository { Future<_ShareFile?> _getOriginalShareFile( BaseAsset asset, { - required bool isDuplicate, + required int occurrence, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { final localId = asset.localId; if (localId != null && !asset.isEdited) { - return _getLocalOriginalShareFile(asset, localId); + return _getLocalOriginalShareFile(asset, localId, occurrence); } final remoteId = asset.remoteId; @@ -283,7 +290,7 @@ class AssetMediaRepository { return _getRemoteOriginalShareFile( asset, remoteId, - isDuplicate: isDuplicate, + occurrence: occurrence, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -291,7 +298,7 @@ class AssetMediaRepository { Future<_ShareFile?> _getPreviewShareFile( BaseAsset asset, { - required bool isDuplicate, + required int occurrence, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { @@ -300,7 +307,7 @@ class AssetMediaRepository { final remotePreview = await _getRemotePreviewShareFile( asset, remoteId, - isDuplicate: isDuplicate, + occurrence: occurrence, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -311,7 +318,7 @@ class AssetMediaRepository { final localId = asset.localId; if (localId != null) { - return _getLocalOriginalShareFile(asset, localId); + return _getLocalOriginalShareFile(asset, localId, occurrence); } _log.warning("Asset has no local or remote ID for preview sharing: $asset"); @@ -344,6 +351,7 @@ class AssetMediaRepository { updateProgress(); final displayNameCounts = countShareDisplayNames(assets, fileType); + final displayNameOccurrences = {}; for (final asset in assets) { if (_isCancelled(cancelCompleter)) { @@ -352,18 +360,21 @@ class AssetMediaRepository { } final effectiveFileType = asset.isVideo ? ShareAssetType.original : fileType; - final isDuplicate = displayNameCounts[_shareDisplayName(asset, fileType)]! > 1; + final displayName = _shareDisplayName(asset, fileType); + final occurrence = displayNameCounts[displayName]! > 1 + ? displayNameOccurrences.update(displayName, (count) => count + 1, ifAbsent: () => 1) + : 0; final shareFile = switch (effectiveFileType) { ShareAssetType.original => await _getOriginalShareFile( asset, - isDuplicate: isDuplicate, + occurrence: occurrence, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), ShareAssetType.preview => await _getPreviewShareFile( asset, - isDuplicate: isDuplicate, + occurrence: occurrence, 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 5ef4ad9bd6..ef944807f2 100644 --- a/mobile/test/repositories/asset_media_repository_test.dart +++ b/mobile/test/repositories/asset_media_repository_test.dart @@ -19,14 +19,12 @@ void main() { ); }); - DownloadTask buildTask(String taskId, String displayName, {bool isDuplicate = false}) => - AssetMediaRepository.buildShareDownloadTask( - taskId: taskId, - url: 'https://example.com/api/assets/some-id/original', - headers: const {}, - displayName: displayName, - isDuplicate: isDuplicate, - ); + DownloadTask buildTask(String taskId, String displayName) => AssetMediaRepository.buildShareDownloadTask( + taskId: taskId, + url: 'https://example.com/api/assets/some-id/original', + headers: const {}, + displayName: displayName, + ); group('buildShareDownloadTask', () { test('saves a unique original under the asset name, without the task id prefix (#29468)', () async { @@ -49,46 +47,61 @@ void main() { expect(task.filename, name); }); - test('two same-named assets download to two distinct prefixed names', () async { + test('two same-named assets download to ordinal names in batch order', () async { final assets = [ TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'), TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-0001.jpg'), ]; final counts = AssetMediaRepository.countShareDisplayNames(assets, ShareAssetType.original); - final isDuplicate = counts['IMG-0001.jpg']! > 1; + expect(counts['IMG-0001.jpg'], 2); - final first = buildTask('share-original-remote-1-111', 'IMG-0001.jpg', isDuplicate: isDuplicate); - final second = buildTask('share-original-remote-2-222', 'IMG-0001.jpg', isDuplicate: isDuplicate); - - expect(p.basename(await first.filePath()), 'share-original-remote-1-111-IMG-0001.jpg'); - expect(p.basename(await second.filePath()), 'share-original-remote-2-222-IMG-0001.jpg'); - }); - - test('sharing the same asset twice downloads to distinct paths', () async { - final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'); - final counts = AssetMediaRepository.countShareDisplayNames([asset, asset], ShareAssetType.original); - final isDuplicate = counts['IMG-0001.jpg']! > 1; - - final first = buildTask('share-original-remote-1-111111', 'IMG-0001.jpg', isDuplicate: isDuplicate); - final second = buildTask('share-original-remote-1-222222', 'IMG-0001.jpg', isDuplicate: isDuplicate); - - expect(await first.filePath(), isNot(await second.filePath())); - }); - - test('a failed duplicate leaves the survivor with the prefixed name', () { - final assets = [ - TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'), - TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-0001.jpg'), - ]; - final counts = AssetMediaRepository.countShareDisplayNames(assets, ShareAssetType.original); - - final survivor = buildTask( + final first = buildTask( 'share-original-remote-1-111', - 'IMG-0001.jpg', - isDuplicate: counts['IMG-0001.jpg']! > 1, + AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 1), + ); + final second = buildTask( + 'share-original-remote-2-222', + AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 2), ); - expect(survivor.filename, 'share-original-remote-1-111-IMG-0001.jpg'); + expect(p.basename(await first.filePath()), 'IMG-0001 (1).jpg'); + expect(p.basename(await second.filePath()), 'IMG-0001 (2).jpg'); + }); + + // 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 asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'); + final counts = AssetMediaRepository.countShareDisplayNames([asset, asset], ShareAssetType.original); + expect(counts['IMG-0001.jpg'], 2); + + final first = buildTask( + 'share-original-remote-1-111111', + AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 1), + ); + final second = buildTask( + 'share-original-remote-1-222222', + AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 2), + ); + + expect(p.basename(await first.filePath()), 'IMG-0001 (1).jpg'); + expect(p.basename(await second.filePath()), 'IMG-0001 (2).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 assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-0001.jpg'), + ]; + final counts = AssetMediaRepository.countShareDisplayNames(assets, ShareAssetType.original); + expect(counts['IMG-0001.jpg'], 2); + + final survivor = buildTask( + 'share-original-remote-2-222', + AssetMediaRepository.getOrdinalShareDisplayName('IMG-0001.jpg', 2), + ); + + expect(survivor.filename, 'IMG-0001 (2).jpg'); }); });