diff --git a/mobile/lib/repositories/asset_media.repository.dart b/mobile/lib/repositories/asset_media.repository.dart index 6058883544..14484c0f1b 100644 --- a/mobile/lib/repositories/asset_media.repository.dart +++ b/mobile/lib/repositories/asset_media.repository.dart @@ -22,7 +22,7 @@ import 'package:path/path.dart' as p; import 'package:photo_manager/photo_manager.dart'; import 'package:share_plus/share_plus.dart'; -typedef _ShareFile = ({File file, bool cleanup, String displayName}); +typedef _ShareFile = ({File file, FileSystemEntity? tempEntity, String displayName}); final assetMediaRepositoryProvider = Provider( (ref) => AssetMediaRepository(ref.watch(nativeSyncApiProvider), ref.watch(storageRepositoryProvider)), @@ -102,40 +102,87 @@ class AssetMediaRepository { } } - /// Deletes temporary files in parallel - Future _cleanupTempFiles(List tempFiles) async { + /// Deletes temporary entities in parallel, removing download task directories recursively + @visibleForTesting + static Future cleanupShareTempFiles(List tempEntities) async { await Future.wait( - tempFiles.map((file) async { + tempEntities.map((entity) async { try { - await file.delete(); + if (entity is Directory) { + await entity.delete(recursive: true); + } else { + await entity.delete(); + } } catch (e) { - _log.warning("Failed to delete temporary file: ${file.path}", e); + _log.warning("Failed to delete temporary file: ${entity.path}", e); } }), ); } - String _sanitizeFilename(String filename) { - return filename.replaceAll(RegExp(r'[\\/]'), '_'); + static final RegExp _pathSeparators = RegExp(r'[\\/]'); + + static String _sanitizeFilename(String filename) { + return filename.replaceAll(_pathSeparators, '_'); } - String _getPreviewFilename(BaseAsset asset) { + @visibleForTesting + static String getOriginalShareFilename(BaseAsset asset) { + final hasUsableName = asset.name.replaceAll(_pathSeparators, '').isNotEmpty; + 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'; + } + + @visibleForTesting + 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(name)} ($occurrence)${p.extension(name)}'; } bool _isCancelled(Completer? cancelCompleter) => cancelCompleter?.isCompleted ?? false; - Future<_ShareFile?> _getLocalOriginalShareFile(BaseAsset asset, String localId) 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, cleanup: CurrentPlatform.isIOS, displayName: _sanitizeFilename(asset.name)); + return (file: file, tempEntity: CurrentPlatform.isIOS ? file : null, displayName: displayName); + } + + @visibleForTesting + static DownloadTask buildShareDownloadTask({ + required String taskId, + required String url, + required Map headers, + required String displayName, + }) { + return DownloadTask( + taskId: taskId, + url: url, + headers: headers, + // 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, + updates: Updates.statusAndProgress, + ); } Future<_ShareFile?> _downloadRemoteShareFile({ @@ -145,14 +192,11 @@ class AssetMediaRepository { Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { - final task = DownloadTask( + final task = buildShareDownloadTask( taskId: taskId, url: url, headers: ApiService.getRequestHeaders(), - filename: '$taskId-$displayName', - baseDirectory: BaseDirectory.temporary, - group: kShareDownloadGroup, - updates: Updates.statusAndProgress, + displayName: displayName, ); final downloader = FileDownloader(); final statusUpdate = await downloader.download( @@ -171,7 +215,8 @@ class AssetMediaRepository { } if (statusUpdate.status == TaskStatus.complete) { - return (file: File(await task.filePath()), cleanup: true, displayName: displayName); + final file = File(await task.filePath()); + return (file: file, tempEntity: file.parent, displayName: displayName); } _log.severe("Download for $displayName failed with status ${statusUpdate.status}", statusUpdate.exception); @@ -181,13 +226,14 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemoteOriginalShareFile( BaseAsset asset, String remoteId, { + 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: _sanitizeFilename(asset.name), + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -196,13 +242,14 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemotePreviewShareFile( BaseAsset asset, String remoteId, { + 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: _getPreviewFilename(asset), + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -210,12 +257,13 @@ class AssetMediaRepository { Future<_ShareFile?> _getOriginalShareFile( BaseAsset asset, { + required String displayName, 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, displayName); } final remoteId = asset.remoteId; @@ -224,11 +272,19 @@ class AssetMediaRepository { return Future.value(null); } - return _getRemoteOriginalShareFile(asset, remoteId, cancelCompleter: cancelCompleter, onProgress: onProgress); + return _getRemoteOriginalShareFile( + asset, + remoteId, + displayName: displayName, + cancelCompleter: cancelCompleter, + onProgress: onProgress, + ); } Future<_ShareFile?> _getPreviewShareFile( BaseAsset asset, { + required String displayName, + required Map occurrences, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { @@ -237,6 +293,7 @@ class AssetMediaRepository { final remotePreview = await _getRemotePreviewShareFile( asset, remoteId, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -247,7 +304,8 @@ class AssetMediaRepository { final localId = asset.localId; if (localId != null) { - return _getLocalOriginalShareFile(asset, localId); + // 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"); @@ -262,7 +320,7 @@ class AssetMediaRepository { void Function(double progress)? onAssetDownloadProgress, }) async { final downloadedXFiles = []; - final tempFiles = []; + final tempFiles = []; final totalAssets = assets.length; var processedAssets = 0; @@ -279,29 +337,35 @@ class AssetMediaRepository { updateProgress(); + final occurrences = {}; + for (final asset in assets) { if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); return 0; } final effectiveFileType = asset.isVideo ? ShareAssetType.original : fileType; + final displayName = shareDisplayName(asset, fileType, occurrences); final shareFile = switch (effectiveFileType) { ShareAssetType.original => await _getOriginalShareFile( asset, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), ShareAssetType.preview => await _getPreviewShareFile( asset, + displayName: displayName, + occurrences: occurrences, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), }; if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); return 0; } @@ -312,8 +376,9 @@ class AssetMediaRepository { } downloadedXFiles.add(XFile(shareFile.file.path, name: shareFile.displayName)); - if (shareFile.cleanup) { - tempFiles.add(shareFile.file); + final tempEntity = shareFile.tempEntity; + if (tempEntity != null) { + tempFiles.add(tempEntity); } processedAssets++; updateProgress(); @@ -325,7 +390,7 @@ class AssetMediaRepository { } if (_isCancelled(cancelCompleter) || !context.mounted) { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); return 0; } @@ -337,7 +402,7 @@ class AssetMediaRepository { downloadedXFiles, sharePositionOrigin: Rect.fromPoints(Offset.zero, Offset(size.width / 3, size.height)), ).then((result) async { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); }), ); diff --git a/mobile/test/repositories/asset_media_repository_test.dart b/mobile/test/repositories/asset_media_repository_test.dart new file mode 100644 index 0000000000..5693d96200 --- /dev/null +++ b/mobile/test/repositories/asset_media_repository_test.dart @@ -0,0 +1,128 @@ +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; + +import '../test_utils.dart'; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUpAll(() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( + const MethodChannel('plugins.flutter.io/path_provider'), + (methodCall) async => '/tmp/immich-share-test', + ); + }); + + 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 { + final task = buildTask('share-original-some-remote-id-123456', 'IMG-0001.jpg'); + + expect(task.filename, 'IMG-0001.jpg'); + expect(p.basename(await task.filePath()), 'IMG-0001.jpg'); + }); + + test('preview shares keep their -preview name', () async { + final task = buildTask('share-preview-some-remote-id-123456', 'IMG-0001-preview.jpg'); + + expect(p.basename(await task.filePath()), 'IMG-0001-preview.jpg'); + }); + + test('names with spaces and unusual characters pass through untouched', () { + const name = 'photo 1 (final) 名字.jpg'; + final task = buildTask('share-original-some-remote-id-123456', name); + + expect(task.filename, name); + }); + }); + + group('shareDisplayName', () { + // receivers flatten attachments into one list, so identical names clobber each other + 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(AssetMediaRepository.shareDisplayName(first, ShareAssetType.original, occurrences), 'IMG-0001.jpg'); + expect(AssetMediaRepository.shareDisplayName(second, ShareAssetType.original, occurrences), 'IMG-0001 (1).jpg'); + }); + + 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 = {}; + + 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'); + }); + }); + + group('getOriginalShareFilename', () { + test('falls back to the remote id when the name is empty', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: ''); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'remote-1'); + }); + + test('falls back when the name is only path separators', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: r'\/'); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'remote-1'); + }); + + test('falls back to the local id when there is no remote id', () { + final asset = TestUtils.createLocalAsset(id: 'local-1').copyWith(name: ''); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'local-1'); + }); + + test('sanitizes separators in a real name instead of falling back', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'holiday/IMG-0001.jpg'); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'holiday_IMG-0001.jpg'); + }); + }); + + group('cleanupShareTempFiles', () { + test('removes only the owned task directories and temp files', () async { + final tempRoot = Directory.systemTemp.createTempSync('immich-share-cleanup'); + addTearDown(() => tempRoot.deleteSync(recursive: true)); + final taskDir = Directory(p.join(tempRoot.path, 'share-original-remote-1-111'))..createSync(); + final downloaded = File(p.join(taskDir.path, 'IMG-0001.jpg'))..createSync(); + final iosLocalTemp = File(p.join(tempRoot.path, 'local-original.jpg'))..createSync(); + final foreignDir = Directory(p.join(tempRoot.path, 'unrelated'))..createSync(); + final foreignFile = File(p.join(foreignDir.path, 'keep.jpg'))..createSync(); + + await AssetMediaRepository.cleanupShareTempFiles([taskDir, iosLocalTemp]); + + expect(taskDir.existsSync(), isFalse); + expect(downloaded.existsSync(), isFalse); + expect(iosLocalTemp.existsSync(), isFalse); + expect(foreignDir.existsSync(), isTrue); + expect(foreignFile.existsSync(), isTrue); + expect(tempRoot.existsSync(), isTrue); + }); + }); +}