fix: use filepaths as cache keys, not File objects (#1128)

* Fix of asset caches:

1. AssetCache - key is set to File.path because File incorrectly checks for presence in cache
2. OrderedAssetCache - in add function is sometimes set index of asset to -1 even if it is the same as already stored. So additional check is implemented to return asset.

* Added bool EnabledRemove  to AssetCache. Setting it to false during Saving of note in Editor  blocks possible call of PdfImageEditor.loadOut

Call of PdfImageEditor.loadOut during saving the note (for example during automatic save) removes background pdf from AssetCache and causes problems with missing assets in saved file.

This solves issue #1066

* fix: add collection to pubspec

* chore: formatting and misc changes

* fix: only remove cache item if not used in another image

---------

Co-authored-by: Adil Hanney <adilh+github@orchid.anonaddy.com>

lib/components/canvas/_asset_cache.dart

@@ -1,7 +1,9 @@
 import 'dart:convert';
 import 'dart:io';
 
+import 'package:collection/collection.dart';
 import 'package:flutter/painting.dart';
+import 'package:logging/logging.dart';
 import 'package:saber/components/canvas/image/editor_image.dart';
 
 /// A cache for assets that are loaded from disk.
@@ -14,11 +16,17 @@ import 'package:saber/components/canvas/image/editor_image.dart';
 class AssetCache {
   AssetCache();
 
-  /// Maps a file to its value.
-  final Map<File, Object> _cache = {};
+  final log = Logger('AssetCache');
 
-  /// Maps a file to the visible images that use it.
-  final Map<File, Set<EditorImage>> _images = {};
+  /// Maps a file path to its value.
+  final Map<String, Object> _cache = {};
+
+  /// Maps a file path to the visible images that use it.
+  final Map<String, Set<EditorImage>> _images = {};
+
+  /// Whether items from the cache can be removed:
+  /// set to false during file save.
+  bool allowRemovingAssets = true;
 
   /// Marks [image] as currently visible.
   ///
@@ -28,13 +36,13 @@ class AssetCache {
   /// in which case this function does nothing.
   void addImage<T extends Object>(EditorImage image, File? file, T value) {
     if (file == null) return;
-    _images.putIfAbsent(file, () => {}).add(image);
-    _cache[file] = value;
+    _images.putIfAbsent(file.path, () => {}).add(image);
+    _cache[file.path] = value;
   }
 
   /// Returns null if [file] is not found.
   T? get<T extends Object>(File file) {
-    return _cache[file] as T?;
+    return _cache[file.path] as T?;
   }
 
   /// Marks [image] as no longer visible.
@@ -46,13 +54,19 @@ class AssetCache {
   ///
   /// Returns whether the image was present in the cache.
   bool removeImage(EditorImage image) {
-    for (final file in _images.keys) {
-      if (_images[file]!.remove(image)) {
-        _images.remove(file);
-        _cache.remove(file);
+    if (!allowRemovingAssets) return false;
+
+    for (final filePath in _images.keys) {
+      final imagesUsingFile = _images[filePath]!;
+      imagesUsingFile.remove(image);
+
+      if (imagesUsingFile.isEmpty) {
+        _images.remove(filePath);
+        _cache.remove(filePath);
         return true;
       }
     }
+
     return false;
   }
 
@@ -65,12 +79,28 @@ class AssetCache {
 class OrderedAssetCache {
   OrderedAssetCache();
 
+  final log = Logger('OrderedAssetCache');
+
   final List<Object> _cache = [];
 
   /// Adds [value] to the cache if it is not already present and
   /// returns the index of the added item.
   int add<T extends Object>(T value) {
-    final index = _cache.indexOf(value);
+    int index = _cache.indexOf(value);
+    if (index == -1 && value is List<int>) {
+      // Lists need to be compared per item
+      final listEq = const ListEquality().equals;
+      for (int i = 0; i < _cache.length; i++) {
+        final cacheItem = _cache[i];
+        if (cacheItem is! List<int>) continue;
+        if (!listEq(value, cacheItem)) continue;
+
+        index = i;
+        break;
+      }
+    }
+    log.fine('OrderedAssetCache.add: index = $index, value = $value');
+
     if (index == -1) {
       _cache.add(value);
       return _cache.length - 1;

lib/pages/editor/editor.dart

@@ -12,6 +12,7 @@ import 'package:flutter_quill/flutter_quill.dart' as flutter_quill;
 import 'package:keybinder/keybinder.dart';
 import 'package:logging/logging.dart';
 import 'package:printing/printing.dart';
+import 'package:saber/components/canvas/_asset_cache.dart';
 import 'package:saber/components/canvas/_stroke.dart';
 import 'package:saber/components/canvas/canvas.dart';
 import 'package:saber/components/canvas/canvas_gesture_detector.dart';
@@ -827,9 +828,16 @@ class EditorState extends State<Editor> {
     }
 
     final filePath = coreInfo.filePath + Editor.extension;
-    final (bson, assets) = coreInfo.saveToBinary(
-      currentPageIndex: currentPageIndex,
-    );
+    final Uint8List bson;
+    final OrderedAssetCache assets;
+    coreInfo.assetCache.allowRemovingAssets = false;
+    try {
+      (bson, assets) = coreInfo.saveToBinary(
+        currentPageIndex: currentPageIndex,
+      );
+    } finally {
+      coreInfo.assetCache.allowRemovingAssets = true;
+    }
     try {
       await Future.wait([
         FileManager.writeFile(filePath, bson, awaitWrite: true),

pubspec.lock

@@ -163,7 +163,7 @@ packages:
     source: hosted
     version: "1.0.0"
   collection:
-    dependency: transitive
+    dependency: "direct main"
     description:
       name: collection
       sha256: ee67cb0715911d28db6bf4af1026078bd6f0128b07a5f66fb2ed94ec6783c09a

pubspec.yaml

@@ -143,6 +143,8 @@ dependencies:
 
   mutex: ^3.1.0
 
+  collection: ^1.0.0
+
 dev_dependencies:
   flutter_test:
     sdk: flutter