From 006e7fd0d5a62271fc03bff5be5b4adcbc57dbeb Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Mon, 31 Aug 2026 19:05:00 +0600 Subject: [PATCH] fix(mobile): keep the original filename when sharing downloaded assets (#30267) * fix(mobile): keep the original filename when sharing downloaded assets * use ordinal suffixes for duplicate share filenames * rename display name maps to totals and duplicates seen * drop the display name totals map and keep the first copy name clean * fold the ordinal suffix into the share display name derivation * resolve share names after download and test through the public flow * use the renamed store repository in the share tests * add docstrings to the share file helpers * simplify the share name resolver to a single set * note the share_plus cache folder behavior on the resolver * drop the version pin from the resolver comment * pin the share_plus version in the resolver comment * undo the accidental reformat, keep the version pin * explain the test harness and cut the teardown and wait noise * say why the storage is stubbed, drop the platform override and busy wait for the share cleanup * spy on the cleanup in the share tests instead of waiting for it --- .../repositories/asset_media.repository.dart | 143 ++++++++-- .../asset_media_repository_test.dart | 262 ++++++++++++++++++ 2 files changed, 375 insertions(+), 30 deletions(-) create mode 100644 mobile/test/repositories/asset_media_repository_test.dart diff --git a/mobile/lib/repositories/asset_media.repository.dart b/mobile/lib/repositories/asset_media.repository.dart index 320a123dd..f9a5de3a8 100644 --- a/mobile/lib/repositories/asset_media.repository.dart +++ b/mobile/lib/repositories/asset_media.repository.dart @@ -22,7 +22,9 @@ 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}); +/// A file staged for the share sheet. [tempEntity] is the temp file or +/// directory to delete afterwards, null when [file] is a gallery original. +typedef _ShareFile = ({File file, FileSystemEntity? tempEntity, String displayName}); final assetMediaRepositoryProvider = Provider( (ref) => AssetMediaRepository(ref.watch(nativeSyncApiProvider), ref.watch(storageRepositoryProvider)), @@ -98,12 +100,15 @@ class AssetMediaRepository { } } - /// Deletes temporary files in parallel - Future _cleanupTempFiles(List tempFiles) async { + @protected + @visibleForTesting + Future cleanupTempFiles(List tempFiles) async { await Future.wait( tempFiles.map((file) async { try { - await file.delete(); + if (file.existsSync()) { + await file.delete(recursive: true); + } } catch (e) { _log.warning("Failed to delete temporary file: ${file.path}", e); } @@ -111,27 +116,44 @@ class AssetMediaRepository { ); } - 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) { + 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'; } + static String _shareDisplayName(BaseAsset asset, ShareAssetType fileType) => + switch (asset.isVideo ? ShareAssetType.original : fileType) { + ShareAssetType.original => _getOriginalShareFilename(asset), + ShareAssetType.preview => _getPreviewFilename(asset), + }; + + static String _ordinalShareFilename(String filename, int occurrence) => + '${p.basenameWithoutExtension(filename)} ($occurrence)${p.extension(filename)}'; + 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); } Future<_ShareFile?> _downloadRemoteShareFile({ @@ -145,7 +167,8 @@ class AssetMediaRepository { taskId: taskId, url: url, headers: ApiService.getRequestHeaders(), - filename: '$taskId-$displayName', + filename: displayName, + directory: taskId, baseDirectory: BaseDirectory.temporary, group: kShareDownloadGroup, updates: Updates.statusAndProgress, @@ -162,14 +185,17 @@ class AssetMediaRepository { }, ); + final file = File(await task.filePath()); if (_isCancelled(cancelCompleter)) { + await cleanupTempFiles([file.parent]); return null; } if (statusUpdate.status == TaskStatus.complete) { - return (file: File(await task.filePath()), cleanup: true, displayName: displayName); + return (file: file, tempEntity: file.parent, displayName: displayName); } + await cleanupTempFiles([file.parent]); _log.severe("Download for $displayName failed with status ${statusUpdate.status}", statusUpdate.exception); return null; } @@ -177,13 +203,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, ); @@ -192,13 +219,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, ); @@ -206,12 +234,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; @@ -220,11 +249,18 @@ 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, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { @@ -233,6 +269,7 @@ class AssetMediaRepository { final remotePreview = await _getRemotePreviewShareFile( asset, remoteId, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -243,13 +280,42 @@ class AssetMediaRepository { final localId = asset.localId; if (localId != null) { - return _getLocalOriginalShareFile(asset, localId); + return _getLocalOriginalShareFile(asset, localId, _shareDisplayName(asset, ShareAssetType.original)); } _log.warning("Asset has no local or remote ID for preview sharing: $asset"); return null; } + /// As of share_plus 10.1.4, sharing copies every file into a single cache + /// folder regardless of where it came from, and equal names overwrite each + /// other there. Downloads are renamed to their display name first since + /// receivers only see the on-disk filename, and a name already taken in the + /// batch gets the first free ` (n)` suffix because gallery originals cannot + /// be renamed. + Future _resolveShareFiles(List<_ShareFile> files) async { + final usedNames = { + for (final shareFile in files) + if (shareFile.tempEntity is! Directory) p.basename(shareFile.file.path), + }; + for (var index = 0; index < files.length; index++) { + final shareFile = files[index]; + if (shareFile.tempEntity is! Directory) { + continue; + } + + var occurrence = 0; + var displayName = shareFile.displayName; + while (usedNames.contains(displayName)) { + displayName = _ordinalShareFilename(shareFile.displayName, ++occurrence); + } + + usedNames.add(displayName); + final file = await shareFile.file.rename(p.join(shareFile.file.parent.path, displayName)); + files[index] = (file: file, tempEntity: shareFile.tempEntity, displayName: displayName); + } + } + Future shareAssets( List assets, BuildContext context, { @@ -257,8 +323,8 @@ class AssetMediaRepository { Completer? cancelCompleter, void Function(double progress)? onAssetDownloadProgress, }) async { - final downloadedXFiles = []; - final tempFiles = []; + final shareFiles = <_ShareFile>[]; + final tempFiles = []; final totalAssets = assets.length; var processedAssets = 0; @@ -277,27 +343,34 @@ class AssetMediaRepository { for (final asset in assets) { if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupTempFiles(tempFiles); return 0; } final effectiveFileType = asset.isVideo ? ShareAssetType.original : fileType; + final displayName = _shareDisplayName(asset, fileType); final shareFile = switch (effectiveFileType) { ShareAssetType.original => await _getOriginalShareFile( asset, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), ShareAssetType.preview => await _getPreviewShareFile( asset, + displayName: displayName, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), }; + final tempEntity = shareFile?.tempEntity; + if (tempEntity != null) { + tempFiles.add(tempEntity); + } if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupTempFiles(tempFiles); return 0; } @@ -307,24 +380,34 @@ class AssetMediaRepository { continue; } - downloadedXFiles.add(XFile(shareFile.file.path, name: shareFile.displayName)); - if (shareFile.cleanup) { - tempFiles.add(shareFile.file); - } + shareFiles.add(shareFile); processedAssets++; updateProgress(); } - if (downloadedXFiles.isEmpty) { + if (shareFiles.isEmpty) { _log.warning("No asset can be retrieved for share"); return 0; } if (_isCancelled(cancelCompleter) || !context.mounted) { - await _cleanupTempFiles(tempFiles); + await cleanupTempFiles(tempFiles); return 0; } + try { + await _resolveShareFiles(shareFiles); + } catch (e, s) { + _log.warning("Failed to prepare files for sharing", e, s); + await cleanupTempFiles(tempFiles); + return 0; + } + if (_isCancelled(cancelCompleter) || !context.mounted) { + await cleanupTempFiles(tempFiles); + return 0; + } + final downloadedXFiles = shareFiles.map((shareFile) => XFile(shareFile.file.path)).toList(); + // we dont want to await the share result since the // "preparing" dialog will not disappear until final size = context.sizeData; @@ -332,8 +415,8 @@ class AssetMediaRepository { Share.shareXFiles( downloadedXFiles, sharePositionOrigin: Rect.fromPoints(Offset.zero, Offset(size.width / 3, size.height)), - ).then((result) async { - await _cleanupTempFiles(tempFiles); + ).whenComplete(() async { + await cleanupTempFiles(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 000000000..c94e5aed5 --- /dev/null +++ b/mobile/test/repositories/asset_media_repository_test.dart @@ -0,0 +1,262 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:background_downloader/background_downloader.dart'; +import 'package:drift/drift.dart' hide isNotNull, isNull; +import 'package:drift/native.dart'; +import 'package:flutter/foundation.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/constants/enums.dart'; +import 'package:immich_mobile/data/db/main/database.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; +import 'package:immich_mobile/domain/models/store.model.dart'; +import 'package:immich_mobile/domain/services/store.service.dart'; +import 'package:immich_mobile/entities/store.entity.dart'; +import 'package:immich_mobile/infrastructure/repositories/settings.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/storage.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/store.repository.dart'; +import 'package:immich_mobile/platform/native_sync_api.g.dart'; +import 'package:immich_mobile/repositories/asset_media.repository.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:path/path.dart' as p; + +import '../test_utils.dart'; + +class _MockNativeSyncApi extends Mock implements NativeSyncApi {} + +class _MockPersistentStorage extends Mock implements PersistentStorage {} + +class _MockStorageRepository extends Mock implements StorageRepository {} + +class _TestAssetMediaRepository extends AssetMediaRepository { + _TestAssetMediaRepository(super.nativeSyncApi, super.storageRepository, this.shareCall); + + final Completer> shareCall; + final cleanups = >[]; + final cleanupAfterShare = Completer>(); + + @override + Future cleanupTempFiles(List tempFiles) async { + cleanups.add(tempFiles); + if (shareCall.isCompleted) { + cleanupAfterShare.complete(tempFiles); + } + } +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late Drift db; + late StoreService store; + late Directory tempRoot; + late _MockStorageRepository storage; + late _TestAssetMediaRepository repository; + late List taskDirs; + late Set failedRemoteIds; + late Completer> shareCall; + + setUpAll(() async { + db = Drift(DatabaseConnection(NativeDatabase.memory(), closeStreamsSynchronously: true)); + store = await StoreService.init(storeRepository: StoreRepository(db), listenUpdates: false); + await SettingsRepository.ensureInitialized(db); + await Store.put(StoreKey.serverEndpoint, 'https://example.com/api'); + // the downloader keeps the storage it gets on its first call. the default one spawns an isolate + // that needs the real path_provider plugin, so a stub answers init and the resume data cleanup + final persistentStorage = _MockPersistentStorage(); + when(persistentStorage.initialize).thenAnswer((_) async {}); + when(() => persistentStorage.removeResumeData(any())).thenAnswer((_) async {}); + when(() => persistentStorage.removePausedTask(any())).thenAnswer((_) async {}); + await FileDownloader(persistentStorage: persistentStorage).ready; + }); + + tearDownAll(() async { + await SettingsRepository.reset(); + await store.dispose(); + await db.close(); + }); + + setUp(() { + tempRoot = Directory.systemTemp.createTempSync('immich-share-test'); + storage = _MockStorageRepository(); + shareCall = Completer>(); + repository = _TestAssetMediaRepository(_MockNativeSyncApi(), storage, shareCall); + taskDirs = []; + failedRemoteIds = {}; + + // keeps every temp path the code asks for inside this test's own folder + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( + const MethodChannel('plugins.flutter.io/path_provider'), + (_) async => tempRoot.path, + ); + // captures the file paths the share sheet would receive + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( + const MethodChannel('dev.fluttercommunity.plus/share'), + (call) async { + final arguments = Map.from(call.arguments as Map); + shareCall.complete((arguments['paths']! as List).cast()); + return 'success'; + }, + ); + // stands in for a real download, writes the file and reports the task status + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( + const MethodChannel('com.bbflight.background_downloader'), + (call) async { + if (call.method != 'enqueue') { + return true; + } + final args = call.arguments! as List; + final task = Task.createFromJsonString(args.first as String) as DownloadTask; + final file = File(await task.filePath()); + await file.parent.create(recursive: true); + await file.writeAsString(task.taskId); + final status = failedRemoteIds.any((id) => task.taskId.contains('-$id-')) + ? TaskStatus.failed + : TaskStatus.complete; + if (status == TaskStatus.complete) { + taskDirs.add(file.parent.path); + } + FileDownloader().downloaderForTesting.processStatusUpdate(TaskStatusUpdate(task, status)); + return true; + }, + ); + }); + + tearDown(() async { + if (tempRoot.existsSync()) { + await tempRoot.delete(recursive: true); + } + }); + + Future<({int count, List names})> share( + WidgetTester tester, + List assets, { + ShareAssetType fileType = ShareAssetType.original, + Completer? cancelCompleter, + TargetPlatform platform = TargetPlatform.android, + }) async { + debugDefaultTargetPlatformOverride = platform; + try { + late BuildContext context; + await tester.pumpWidget( + MaterialApp( + home: Builder( + builder: (ctx) { + context = ctx; + return const SizedBox.shrink(); + }, + ), + ), + ); + + final result = await tester.runAsync(() async { + final count = await repository.shareAssets( + assets, + context, + fileType: fileType, + cancelCompleter: cancelCompleter, + ); + if (cancelCompleter?.isCompleted ?? false) { + return (count: count, names: []); + } + final paths = await shareCall.future; + final cleaned = await repository.cleanupAfterShare.future; + expect(cleaned.map((entity) => entity.path), taskDirs); + return (count: count, names: paths.map(p.basename).toList()); + }); + + return result!; + } finally { + debugDefaultTargetPlatformOverride = null; + } + } + + testWidgets('shares sanitized and fallback names then removes task directories', (tester) async { + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'holiday/Photo 1 名字.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: ''), + TestUtils.createRemoteAsset(id: 'remote-3').copyWith(name: r'\/'), + ]; + + final result = await share(tester, assets); + + expect(result.count, 3); + expect(result.names, ['holiday_Photo 1 名字.jpg', 'remote-2', 'remote-3']); + }); + + testWidgets('keeps the first copy and uses the first free ordinal for the next', (tester) async { + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG (1).jpg'), + TestUtils.createRemoteAsset(id: 'remote-3').copyWith(name: 'IMG.jpg'), + ]; + + final result = await share(tester, assets); + + expect(result.count, 3); + expect(result.names, ['IMG.jpg', 'IMG (1).jpg', 'IMG (2).jpg']); + }); + + testWidgets('keeps mixed local and remote paths distinct', (tester) async { + final localFile = File(p.join(tempRoot.path, 'local', 'IMG.jpg')); + localFile.parent.createSync(); + localFile.writeAsStringSync('local'); + when(() => storage.getFileForAsset('local-1')).thenAnswer((_) async => localFile); + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG.jpg'), + TestUtils.createLocalAsset(id: 'local-1').copyWith(name: 'IMG.jpg'), + ]; + + final result = await share(tester, assets); + + expect(result.count, 2); + expect(result.names, ['IMG (1).jpg', 'IMG.jpg']); + expect(localFile.existsSync(), isTrue); + }); + + testWidgets('does not count failed downloads when adding ordinals', (tester) async { + failedRemoteIds.add('remote-1'); + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG.jpg'), + ]; + + final result = await share(tester, assets); + + expect(result.count, 1); + expect(result.names, ['IMG.jpg']); + }); + + testWidgets('cleans the current iOS temp file when cancelled during retrieval', (tester) async { + final cancellation = Completer(); + final localFile = File(p.join(tempRoot.path, 'local', 'IMG.jpg')); + localFile.parent.createSync(); + localFile.writeAsStringSync('local'); + when(() => storage.getFileForAsset('local-1')).thenAnswer((_) async { + cancellation.complete(); + return localFile; + }); + final asset = TestUtils.createLocalAsset(id: 'local-1').copyWith(name: 'IMG.jpg'); + + final result = await share(tester, [asset], cancelCompleter: cancellation, platform: TargetPlatform.iOS); + + expect(result.count, 0); + expect(result.names, isEmpty); + expect(shareCall.isCompleted, isFalse); + expect(repository.cleanups.singleOrNull, [localFile]); + }); + + testWidgets('adds an ordinal when preview and video names match', (tester) async { + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-preview.jpg', type: AssetType.video), + ]; + + final result = await share(tester, assets, fileType: ShareAssetType.preview); + + expect(result.count, 2); + expect(result.names, ['IMG-preview.jpg', 'IMG-preview (1).jpg']); + }); +}