Creation of PhotoManagerSafe + lock in LocalSyncService + improved download UI + missing notify call

2024-12-10 13:25:00 +01:00
parent 8692421b9a
commit 0cb79102fd
6 changed files with 239 additions and 81 deletions
--- a/mobile/lib/services/local_sync_service.dart
+++ b/mobile/lib/services/local_sync_service.dart
@@ -23,6 +23,7 @@ import "package:photos/utils/debouncer.dart";
 import "package:photos/utils/photo_manager_util.dart";
 import "package:photos/utils/sqlite_util.dart";
 import 'package:shared_preferences/shared_preferences.dart';
+import 'package:synchronized/synchronized.dart';
 import 'package:tuple/tuple.dart';

 class LocalSyncService {
@@ -31,6 +32,7 @@ class LocalSyncService {
  late SharedPreferences _prefs;
  Completer<void>? _existingSync;
  late Debouncer _changeCallbackDebouncer;
+  final Lock _lock = Lock();

  static const kDbUpdationTimeKey = "db_updation_time";
  static const kHasCompletedFirstImportKey = "has_completed_firstImport";
@@ -77,50 +79,57 @@ class LocalSyncService {
    }
    _existingSync = Completer<void>();
    final int ownerID = Configuration.instance.getUserID()!;
-    final existingLocalFileIDs = await _db.getExistingLocalFileIDs(ownerID);
-    _logger.info("${existingLocalFileIDs.length} localIDs were discovered");
+    
+    // We use a lock to prevent synchronisation to occur while it is downloading
+    // as this introduces wrong entry in FilesDB due to race condition
+    // This is a fix for https://github.com/ente-io/ente/issues/4296
+    await _lock.synchronized(() async {
+      final existingLocalFileIDs = await _db.getExistingLocalFileIDs(ownerID);
+      _logger.info("${existingLocalFileIDs.length} localIDs were discovered");

-    final syncStartTime = DateTime.now().microsecondsSinceEpoch;
-    final lastDBUpdationTime = _prefs.getInt(kDbUpdationTimeKey) ?? 0;
-    final startTime = DateTime.now().microsecondsSinceEpoch;
-    if (lastDBUpdationTime != 0) {
-      await _loadAndStoreDiff(
-        existingLocalFileIDs,
-        fromTime: lastDBUpdationTime,
-        toTime: syncStartTime,
-      );
-    } else {
-      // Load from 0 - 01.01.2010
-      Bus.instance.fire(SyncStatusUpdate(SyncStatus.startedFirstGalleryImport));
-      var startTime = 0;
-      var toYear = 2010;
-      var toTime = DateTime(toYear).microsecondsSinceEpoch;
-      while (toTime < syncStartTime) {
+      final syncStartTime = DateTime.now().microsecondsSinceEpoch;
+      final lastDBUpdationTime = _prefs.getInt(kDbUpdationTimeKey) ?? 0;
+      final startTime = DateTime.now().microsecondsSinceEpoch;
+      if (lastDBUpdationTime != 0) {
+        await _loadAndStoreDiff(
+          existingLocalFileIDs,
+          fromTime: lastDBUpdationTime,
+          toTime: syncStartTime,
+        );
+      } else {
+        // Load from 0 - 01.01.2010
+        Bus.instance.fire(SyncStatusUpdate(SyncStatus.startedFirstGalleryImport));
+        var startTime = 0;
+        var toYear = 2010;
+        var toTime = DateTime(toYear).microsecondsSinceEpoch;
+        while (toTime < syncStartTime) {
+          await _loadAndStoreDiff(
+            existingLocalFileIDs,
+            fromTime: startTime,
+            toTime: toTime,
+          );
+          startTime = toTime;
+          toYear++;
+          toTime = DateTime(toYear).microsecondsSinceEpoch;
+        }
        await _loadAndStoreDiff(
          existingLocalFileIDs,
          fromTime: startTime,
-          toTime: toTime,
+          toTime: syncStartTime,
        );
-        startTime = toTime;
-        toYear++;
-        toTime = DateTime(toYear).microsecondsSinceEpoch;
      }
-      await _loadAndStoreDiff(
-        existingLocalFileIDs,
-        fromTime: startTime,
-        toTime: syncStartTime,
-      );
-    }
-    if (!hasCompletedFirstImport()) {
-      await _prefs.setBool(kHasCompletedFirstImportKey, true);
-      await _refreshDeviceFolderCountAndCover(isFirstSync: true);
-      _logger.fine("first gallery import finished");
-      Bus.instance
-          .fire(SyncStatusUpdate(SyncStatus.completedFirstGalleryImport));
-    }
-    final endTime = DateTime.now().microsecondsSinceEpoch;
-    final duration = Duration(microseconds: endTime - startTime);
-    _logger.info("Load took " + duration.inMilliseconds.toString() + "ms");
+      if (!hasCompletedFirstImport()) {
+        await _prefs.setBool(kHasCompletedFirstImportKey, true);
+        await _refreshDeviceFolderCountAndCover(isFirstSync: true);
+        _logger.fine("first gallery import finished");
+        Bus.instance
+            .fire(SyncStatusUpdate(SyncStatus.completedFirstGalleryImport));
+      }
+      final endTime = DateTime.now().microsecondsSinceEpoch;
+      final duration = Duration(microseconds: endTime - startTime);
+      _logger.info("Load took " + duration.inMilliseconds.toString() + "ms");
+    });
+    
    _existingSync?.complete();
    _existingSync = null;
  }
@@ -240,6 +249,10 @@ class LocalSyncService {
    }
  }

+  Lock getLock() {
+    return _lock;
+  }
+
  bool hasGrantedPermissions() {
    return _prefs.getBool(kHasGrantedPermissionsKey) ?? false;
  }
@@ -361,7 +374,7 @@ class LocalSyncService {
        unawaited(checkAndSync());
      });
    });
-    PhotoManager.startChangeNotify();
+    PhotoManagerSafe.startChangeNotify(null);
  }

  Future<void> checkAndSync() async {
--- a/mobile/lib/ui/tools/editor/image_editor_page.dart
+++ b/mobile/lib/ui/tools/editor/image_editor_page.dart
@@ -27,6 +27,7 @@ import 'package:photos/ui/tools/editor/filtered_image.dart';
 import 'package:photos/ui/viewer/file/detail_page.dart';
 import 'package:photos/utils/dialog_util.dart';
 import 'package:photos/utils/navigation_util.dart';
+import "package:photos/utils/photo_manager_util.dart";
 import 'package:photos/utils/toast_util.dart';
 import 'package:syncfusion_flutter_core/theme.dart';
 import 'package:syncfusion_flutter_sliders/sliders.dart';
@@ -358,7 +359,7 @@ class _ImageEditorPageState extends State<ImageEditorPage> {
              ".JPEG";
      //Disabling notifications for assets changing to insert the file into
      //files db before triggering a sync.
-      await PhotoManager.stopChangeNotify();
+      await PhotoManagerSafe.stopChangeNotify(widget.originalFile.title!);
      final AssetEntity? newAsset =
          await (PhotoManager.editor.saveImage(result, filename: fileName));
      final newFile = await ente.EnteFile.fromAsset(
@@ -410,7 +411,7 @@ class _ImageEditorPageState extends State<ImageEditorPage> {
      showToast(context, S.of(context).oopsCouldNotSaveEdits);
      _logger.severe(e, s);
    } finally {
-      await PhotoManager.startChangeNotify();
+      await PhotoManagerSafe.startChangeNotify(widget.originalFile.title!);
    }
  }

--- a/mobile/lib/ui/tools/editor/video_editor_page.dart
+++ b/mobile/lib/ui/tools/editor/video_editor_page.dart
@@ -24,6 +24,7 @@ import "package:photos/ui/tools/editor/video_trim_page.dart";
 import "package:photos/ui/viewer/file/detail_page.dart";
 import "package:photos/utils/dialog_util.dart";
 import "package:photos/utils/navigation_util.dart";
+import "package:photos/utils/photo_manager_util.dart";
 import "package:photos/utils/toast_util.dart";
 import "package:video_editor/video_editor.dart";

@@ -238,7 +239,7 @@ class _VideoEditorPageState extends State<VideoEditorPage> {
              ".mp4";
          //Disabling notifications for assets changing to insert the file into
          //files db before triggering a sync.
-          await PhotoManager.stopChangeNotify();
+          await PhotoManagerSafe.stopChangeNotify(widget.file.title!);

          try {
            final AssetEntity? newAsset =
@@ -299,6 +300,8 @@ class _VideoEditorPageState extends State<VideoEditorPage> {
      );
    } catch (_) {
      await dialog.hide();
+    } finally {
+      await PhotoManagerSafe.startChangeNotify(widget.file.title!);
    }
  }
 }
--- a/mobile/lib/ui/viewer/actions/file_selection_actions_widget.dart
+++ b/mobile/lib/ui/viewer/actions/file_selection_actions_widget.dart
@@ -822,9 +822,12 @@ class _FileSelectionActionsWidgetState
  }

  Future<void> _download(List<EnteFile> files) async {
+    final totalFiles = files.length;
+    int downloadedFiles = 0;
+
    final dialog = createProgressDialog(
      context,
-      S.of(context).downloading,
+      S.of(context).downloading + "$downloadedFiles/$totalFiles",
      isDismissible: true,
    );
    await dialog.show();
@@ -832,7 +835,12 @@ class _FileSelectionActionsWidgetState
      final futures = <Future>[];
      for (final file in files) {
        if (file.localID == null) {
-          futures.add(downloadToGallery(file));
+          futures.add(
+            downloadToGallery(file).then((_) {
+              downloadedFiles++;
+              dialog.update(message: S.of(context).downloading + " ($downloadedFiles/$totalFiles)");
+            }),
+          );
        }
      }
      await Future.wait(futures);
--- a/mobile/lib/utils/file_download_util.dart
+++ b/mobile/lib/utils/file_download_util.dart
@@ -21,6 +21,7 @@ import "package:photos/utils/data_util.dart";
 import "package:photos/utils/fake_progress.dart";
 import "package:photos/utils/file_key.dart";
 import "package:photos/utils/file_util.dart";
+import "package:photos/utils/photo_manager_util.dart";

 final _logger = Logger("file_download_util");

@@ -189,50 +190,54 @@ Future<void> downloadToGallery(EnteFile file) async {
    final bool downloadLivePhotoOnDroid =
        type == FileType.livePhoto && Platform.isAndroid;
    AssetEntity? savedAsset;
-    final File? fileToSave = await getFile(file);
-    //Disabling notifications for assets changing to insert the file into
-    //files db before triggering a sync.
-    await PhotoManager.stopChangeNotify();
-    if (type == FileType.image) {
-      savedAsset = await PhotoManager.editor
-          .saveImageWithPath(fileToSave!.path, title: file.title!);
-    } else if (type == FileType.video) {
-      savedAsset =
-          await PhotoManager.editor.saveVideo(fileToSave!, title: file.title!);
-    } else if (type == FileType.livePhoto) {
-      final File? liveVideoFile =
-          await getFileFromServer(file, liveVideo: true);
-      if (liveVideoFile == null) {
-        throw AssertionError("Live video can not be null");
+    // We use a lock to prevent synchronisation to occur while it is downloading
+    // as this introduces wrong entry in FilesDB due to race condition
+    // This is a fix for https://github.com/ente-io/ente/issues/4296
+    await LocalSyncService.instance.getLock().synchronized(() async {
+      final File? fileToSave = await getFile(file);
+      //Disabling notifications for assets changing to insert the file into
+      //files db before triggering a sync.
+      await PhotoManagerSafe.stopChangeNotify(file.generatedID.toString());
+      if (type == FileType.image) {
+        savedAsset = await PhotoManager.editor
+            .saveImageWithPath(fileToSave!.path, title: file.title!);
+      } else if (type == FileType.video) {
+        savedAsset =
+            await PhotoManager.editor.saveVideo(fileToSave!, title: file.title!);
+      } else if (type == FileType.livePhoto) {
+        final File? liveVideoFile =
+            await getFileFromServer(file, liveVideo: true);
+        if (liveVideoFile == null) {
+          throw AssertionError("Live video can not be null");
+        }
+        if (downloadLivePhotoOnDroid) {
+          await _saveLivePhotoOnDroid(fileToSave!, liveVideoFile, file);
+        } else {
+          savedAsset = await PhotoManager.editor.darwin.saveLivePhoto(
+            imageFile: fileToSave!,
+            videoFile: liveVideoFile,
+            title: file.title!,
+          );
+        }
      }
-      if (downloadLivePhotoOnDroid) {
-        await _saveLivePhotoOnDroid(fileToSave!, liveVideoFile, file);
-      } else {
-        savedAsset = await PhotoManager.editor.darwin.saveLivePhoto(
-          imageFile: fileToSave!,
-          videoFile: liveVideoFile,
-          title: file.title!,
+      if (savedAsset != null) {
+        file.localID = savedAsset!.id;
+        await FilesDB.instance.insert(file);
+        Bus.instance.fire(
+          LocalPhotosUpdatedEvent(
+            [file],
+            source: "download",
+          ),
        );
+      } else if (!downloadLivePhotoOnDroid && savedAsset == null) {
+        _logger.severe('Failed to save assert of type $type');
      }
-    }
-
-    if (savedAsset != null) {
-      file.localID = savedAsset.id;
-      await FilesDB.instance.insert(file);
-      Bus.instance.fire(
-        LocalPhotosUpdatedEvent(
-          [file],
-          source: "download",
-        ),
-      );
-    } else if (!downloadLivePhotoOnDroid && savedAsset == null) {
-      _logger.severe('Failed to save assert of type $type');
-    }
+    });
  } catch (e) {
    _logger.severe("Failed to save file", e);
    rethrow;
  } finally {
-    await PhotoManager.startChangeNotify();
+    await PhotoManagerSafe.startChangeNotify(file.generatedID.toString());
    LocalSyncService.instance.checkAndSync().ignore();
  }
 }
--- a/mobile/lib/utils/photo_manager_util.dart
+++ b/mobile/lib/utils/photo_manager_util.dart
@@ -1,4 +1,7 @@
+import 'dart:async';
+import "package:logging/logging.dart";
 import "package:photo_manager/photo_manager.dart";
+import 'package:synchronized/synchronized.dart';

 Future<PermissionState> requestPhotoMangerPermissions() {
  return PhotoManager.requestPermissionExtend(
@@ -10,3 +13,128 @@ Future<PermissionState> requestPhotoMangerPermissions() {
    ),
  );
 }
+
+final _logger = Logger("PhotoManagerUtil");
+// This is a wrapper for safe handling of PhotoManager.startChangeNotify() and
+// PhotoManager.stopChangeNotify(). Since PhotoManager is globally shared, we want
+// to make sure no notification is sent while it should not. The logic is that it will
+// only start if no other asset (or task) requested to stop changes, or if the expiration
+// time for the asset (task) expired. '_processingAssets' should be seen as a queue of
+// open requests.
+class PhotoManagerSafe {
+  // Tracks processing assets with their expiration times
+  static final Map<String?, DateTime> _processingAssets = {};
+
+  // Timer for monitoring asset processing
+  static Timer? _expirationTimer;
+
+  // Synchronization lock
+  static final _lock = Lock();
+
+  // Estimate processing duration based on file size
+  static Duration _estimateProcessingDuration(int fileSize) {
+    final estimatedSeconds = (fileSize / (1024 * 1024)).ceil() * 2;
+    return Duration(
+      seconds: estimatedSeconds.clamp(5, 120),
+    );
+  }
+
+  // Manage asset processing state. Lock ensures no start/stop is performed
+  // at the same time.
+  static Future<void> manageAssetProcessing({
+    required String? assetId,
+    required bool isStarting,
+    int? fileSize,
+  }) async {
+    await _lock.synchronized(() async {
+      try {
+        if (isStarting) {
+          // Remove the asset from processing only if assetId is not null
+          if (assetId != null) {
+            _processingAssets.remove(assetId);
+          }
+
+          // Restart change notify only if no assets are processing and no stop was requested
+          if (_processingAssets.isEmpty) {
+            await PhotoManager.startChangeNotify();
+          }
+
+          _stopExpirationMonitoringIfEmpty();
+        } else {
+          // Stopping the asset
+          final duration = _estimateProcessingDuration(
+            fileSize ?? 10 * 1024 * 1024, // 10MB default
+          );
+
+          // First asset to request stop
+          if (_processingAssets.isEmpty) {
+            await PhotoManager.stopChangeNotify();
+          }
+
+          // Track the processing asset with expiration
+          _processingAssets[assetId] = DateTime.now().add(duration);
+
+          _startOrContinueExpirationMonitoring();
+        }
+      } catch (e, stackTrace) {
+        _logger.severe(
+          "${isStarting ? 'Start' : 'Stop'}ChangeNotify error for ID $assetId", 
+          e, 
+          stackTrace,
+        );
+        rethrow;
+      }
+    });
+  }
+
+  // Start or continue the expiration monitoring timer
+  static void _startOrContinueExpirationMonitoring() {
+    if (_expirationTimer != null && _expirationTimer!.isActive) return;
+
+      _expirationTimer = Timer.periodic(const Duration(seconds: 1), (timer) {
+        final now = DateTime.now();
+        
+        // Remove expired assets
+        _processingAssets.removeWhere((assetId, expiresAt) {
+          final bool isExpired = expiresAt.isBefore(now);
+          if (isExpired) {
+          }
+          return isExpired;
+        });
+
+        // Handle asset processing completion
+        if (_processingAssets.isEmpty) {
+          
+          // Start ChangeNotify
+          try {
+            PhotoManager.startChangeNotify();
+          } catch (e, stackTrace) {
+            _logger.severe("Error restarting change notify", e, stackTrace);
+          }
+
+          _stopExpirationMonitoringIfEmpty();
+        }
+    });
+  }
+
+  // Stop the expiration monitoring timer if no assets are being processed
+  static void _stopExpirationMonitoringIfEmpty() {
+    if (_processingAssets.isEmpty) {
+      _expirationTimer?.cancel();
+      _expirationTimer = null;
+    }
+  }
+
+  static Future<void> stopChangeNotify(String? assetId, {int? fileSize}) =>
+    manageAssetProcessing(
+      assetId: assetId, 
+      isStarting: false, 
+      fileSize: fileSize,
+    );
+
+  static Future<void> startChangeNotify(String? assetId) =>
+    manageAssetProcessing(
+      assetId: assetId, 
+      isStarting: true,
+    );
+}