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
2026-08-31 19:05:00 +06:00
parent 015e5f20d7
commit 006e7fd0d5
2 changed files with 375 additions and 30 deletions
@@ -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<void> _cleanupTempFiles(List<File> tempFiles) async {
+  @protected
+  @visibleForTesting
+  Future<void> cleanupTempFiles(List<FileSystemEntity> 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<void>? 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<void>? 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<void>? 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<void>? 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<void>? 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<void> _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<int> shareAssets(
    List<BaseAsset> assets,
    BuildContext context, {
@@ -257,8 +323,8 @@ class AssetMediaRepository {
    Completer<void>? cancelCompleter,
    void Function(double progress)? onAssetDownloadProgress,
  }) async {
-    final downloadedXFiles = <XFile>[];
-    final tempFiles = <File>[];
+    final shareFiles = <_ShareFile>[];
+    final tempFiles = <FileSystemEntity>[];
    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);
      }),
    );

@@ -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<List<String>> shareCall;
+  final cleanups = <List<FileSystemEntity>>[];
+  final cleanupAfterShare = Completer<List<FileSystemEntity>>();
+
+  @override
+  Future<void> cleanupTempFiles(List<FileSystemEntity> 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<String> taskDirs;
+  late Set<String> failedRemoteIds;
+  late Completer<List<String>> 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<List<String>>();
+    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<Object?, Object?>.from(call.arguments as Map);
+        shareCall.complete((arguments['paths']! as List).cast<String>());
+        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<String> names})> share(
+    WidgetTester tester,
+    List<BaseAsset> assets, {
+    ShareAssetType fileType = ShareAssetType.original,
+    Completer<void>? 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: <String>[]);
+        }
+        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<void>();
+    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']);
+  });
+}