- Mark objects only as clean when saving was successful.

- Only mark objects as clean if they have not been modified since we started saving.

CHANGELOG.md

@@ -1,3 +1,8 @@
+## 2.3.0
+
+- Mark objects only as clean when saving was successful.
+- Only mark objects as clean if they have not been modified since we started saving.
+
 ## 2.2.0
 
 - If argon2 ffi implementation is not available, fallback to pointycastle (dart-only) 

lib/src/kdbx_entry.dart

@@ -9,7 +9,6 @@ import 'package:kdbx/src/kdbx_exceptions.dart';
 import 'package:kdbx/src/kdbx_file.dart';
 import 'package:kdbx/src/kdbx_format.dart';
 import 'package:kdbx/src/kdbx_group.dart';
-import 'package:kdbx/src/kdbx_header.dart';
 import 'package:kdbx/src/kdbx_object.dart';
 import 'package:kdbx/src/kdbx_xml.dart';
 import 'package:logging/logging.dart';
@@ -77,7 +76,7 @@ class KdbxKey {
 extension KdbxEntryInternal on KdbxEntry {
   KdbxEntry cloneInto(KdbxGroup otherGroup, {bool toHistoryEntry = false}) =>
       KdbxEntry.create(
-        otherGroup.file!,
+        otherGroup.file,
         otherGroup,
         isHistoryEntry: toHistoryEntry,
       )
@@ -212,7 +211,7 @@ class KdbxEntry extends KdbxObject {
   StringNode get tags => StringNode(this, 'Tags');
 
   @override
-  set file(KdbxFile? file) {
+  set file(KdbxFile file) {
     super.file = file;
     // TODO this looks like some weird workaround, get rid of the
     // `file` reference.
@@ -346,7 +345,7 @@ class KdbxEntry extends KdbxObject {
       value: bytes,
     );
     modify(() {
-      file!.ctx.addBinary(binary);
+      file.ctx.addBinary(binary);
       _binaries[key] = binary;
     });
     return binary;

lib/src/kdbx_file.dart

@@ -10,6 +10,7 @@ import 'package:kdbx/src/kdbx_format.dart';
 import 'package:kdbx/src/kdbx_group.dart';
 import 'package:kdbx/src/kdbx_header.dart';
 import 'package:kdbx/src/kdbx_object.dart';
+import 'package:kdbx/src/utils/sequence.dart';
 import 'package:logging/logging.dart';
 import 'package:quiver/check.dart';
 import 'package:synchronized/synchronized.dart';
@@ -17,6 +18,8 @@ import 'package:xml/xml.dart' as xml;
 
 final _logger = Logger('kdbx_file');
 
+typedef FileSaveCallback = Future<int> Function(Uint8List bytes);
+
 class KdbxFile {
   KdbxFile(
       this.ctx, this.kdbxFormat, this.credentials, this.header, this.body) {
@@ -54,14 +57,17 @@ class KdbxFile {
   Stream<Set<KdbxObject>> get dirtyObjectsChanged =>
       _dirtyObjectsChanged.stream;
 
-  Future<Uint8List> save() async {
-    return kdbxFormat.save(this);
+  Future<Uint8List> save([FileSaveCallback? saveBytes]) async {
+    return kdbxFormat.save(this, saveBytes);
   }
 
   /// Marks all dirty objects as clean. Called by [KdbxFormat.save].
-  void onSaved() {
-    dirtyObjects.clear();
-    _dirtyObjectsChanged.add(const {});
+  void onSaved(TimeSequence savedAt) {
+    final cleanedObjects = dirtyObjects.where((e) => e.clean(savedAt)).toList();
+
+    dirtyObjects.removeAll(cleanedObjects);
+    _logger.finer('Saved. Remaining dirty objects: ${dirtyObjects.length}');
+    _dirtyObjectsChanged.add(dirtyObjects);
   }
 
   Iterable<KdbxObject> get _allObjects => body.rootGroup

lib/src/kdbx_format.dart

@@ -21,6 +21,7 @@ import 'package:kdbx/src/kdbx_group.dart';
 import 'package:kdbx/src/kdbx_header.dart';
 import 'package:kdbx/src/kdbx_xml.dart';
 import 'package:kdbx/src/utils/byte_utils.dart';
+import 'package:kdbx/src/utils/sequence.dart';
 import 'package:logging/logging.dart';
 import 'package:meta/meta.dart';
 import 'package:pointycastle/export.dart';
@@ -576,10 +577,20 @@ class KdbxFormat {
   }
 
   /// Saves the given file.
-  Future<Uint8List> save(KdbxFile file) async {
+  Future<Uint8List> save(KdbxFile file, FileSaveCallback? saveBytes) async {
     _logger.finer('Saving ${file.body.rootGroup.uuid} '
         '(locked: ${file.saveLock.locked})');
-    return file.saveLock.synchronized(() => _saveSynchronized(file));
+    return file.saveLock.synchronized(() async {
+      final savedAt = TimeSequence.now();
+      final bytes = await _saveSynchronized(file);
+      if (saveBytes != null) {
+        _logger.fine('Saving bytes.');
+        final byteCount = await saveBytes(bytes);
+        _logger.fine('Saved bytes. $byteCount');
+      }
+      file.onSaved(savedAt);
+      return bytes;
+    });
   }
 
   Future<Uint8List> _saveSynchronized(KdbxFile file) async {
@@ -613,7 +624,6 @@ class KdbxFormat {
     } else {
       throw UnsupportedError('Unsupported version ${header.version}');
     }
-    file.onSaved();
     return output.toBytes();
   }
 

lib/src/kdbx_group.dart

@@ -159,7 +159,7 @@ class KdbxGroup extends KdbxObject {
           // item was moved.
           if (otherObj.wasMovedAfter(movedObj)) {
             // item was moved in the other file, so we have to move it here.
-            file!.move(movedObj, this);
+            file.move(movedObj, this);
             mergeContext.trackChange(movedObj, debug: 'moved to another group');
           } else {
             // item was moved in this file, so nothing to do.

lib/src/kdbx_object.dart

@@ -10,6 +10,7 @@ import 'package:kdbx/src/kdbx_group.dart';
 import 'package:kdbx/src/kdbx_meta.dart';
 import 'package:kdbx/src/kdbx_times.dart';
 import 'package:kdbx/src/kdbx_xml.dart';
+import 'package:kdbx/src/utils/sequence.dart';
 import 'package:logging/logging.dart';
 import 'package:meta/meta.dart';
 import 'package:quiver/iterables.dart';
@@ -37,7 +38,7 @@ mixin Changeable<T> {
 
   Stream<ChangeEvent<T>> get changes => _controller.stream;
 
-  bool _isDirty = false;
+  TimeSequence? _isDirty;
 
   /// allow recursive calls to [modify]
   bool _isInModify = false;
@@ -60,10 +61,11 @@ mixin Changeable<T> {
   void onAfterAnyModify() {}
 
   RET modify<RET>(RET Function() modify) {
-    if (_isDirty || _isInModify) {
+    if (isDirty || _isInModify) {
       try {
         return modify();
       } finally {
+        _isDirty = TimeSequence.now();
         onAfterAnyModify();
       }
     }
@@ -72,23 +74,30 @@ mixin Changeable<T> {
     try {
       return modify();
     } finally {
-      _isDirty = true;
+      _isDirty = TimeSequence.now();
       _isInModify = false;
       onAfterModify();
       onAfterAnyModify();
-      _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty));
+      _controller.add(ChangeEvent(object: this as T, isDirty: isDirty));
     }
   }
 
-  void clean() {
-    if (!_isDirty) {
-      return;
+  bool clean(TimeSequence savedAt) {
+    final dirty = _isDirty;
+    if (dirty == null) {
+      _logger.warning('clean() called, even though we are not even dirty.');
+      return false;
+    }
+    if (savedAt.isBefore(dirty)) {
+      _logger.fine('We got dirty after save was invoked. so we are not clean.');
+      return false;
     }
-    _isDirty = false;
-    _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty));
+    _isDirty = null;
+    _controller.add(ChangeEvent(object: this as T, isDirty: isDirty));
+    return true;
   }
 
-  bool get isDirty => _isDirty;
+  bool get isDirty => _isDirty != null;
 }
 
 abstract class KdbxNodeContext implements KdbxNode {
@@ -97,7 +106,7 @@ abstract class KdbxNodeContext implements KdbxNode {
 
 abstract class KdbxNode with Changeable<KdbxNode> {
   KdbxNode.create(String nodeName) : node = XmlElement(XmlName(nodeName)) {
-    _isDirty = true;
+    _isDirty = TimeSequence.now();
   }
 
   KdbxNode.read(this.node);
@@ -111,10 +120,8 @@ abstract class KdbxNode with Changeable<KdbxNode> {
 //  String text(String nodeName) => _opt(nodeName)?.text;
 
   /// must only be called to save this object.
-  /// will mark this object as not dirty.
   @mustCallSuper
   XmlElement toXml() {
-    clean();
     return node.copy();
   }
 }
@@ -158,7 +165,7 @@ extension KdbxObjectInternal on KdbxObject {
 abstract class KdbxObject extends KdbxNode {
   KdbxObject.create(
     this.ctx,
-    this.file,
+    this._file,
     String nodeName,
     KdbxGroup? parent,
   )   : times = KdbxTimes.create(ctx),
@@ -173,8 +180,11 @@ abstract class KdbxObject extends KdbxNode {
         super.read(node);
 
   /// the file this object is part of. will be set AFTER loading, etc.
+  KdbxFile get file => _file!;
+  set file(KdbxFile file) => _file = file;
+
   /// TODO: We should probably get rid of this `file` reference.
-  KdbxFile? file;
+  KdbxFile? _file;
 
   final KdbxReadWriteContext ctx;
 
@@ -196,11 +206,11 @@ abstract class KdbxObject extends KdbxNode {
       UuidNode(this, 'PreviousParentGroup');
 
   KdbxCustomIcon? get customIcon =>
-      customIconUuid.get()?.let((uuid) => file!.body.meta.customIcons[uuid]);
+      customIconUuid.get()?.let((uuid) => file.body.meta.customIcons[uuid]);
 
   set customIcon(KdbxCustomIcon? icon) {
     if (icon != null) {
-      file!.body.meta.addCustomIcon(icon);
+      file.body.meta.addCustomIcon(icon);
       customIconUuid.set(icon.uuid);
     } else {
       customIconUuid.set(null);
@@ -220,7 +230,7 @@ abstract class KdbxObject extends KdbxNode {
     super.onAfterAnyModify();
     times.modifiedNow();
     // during initial `create` the file will be null.
-    file?.dirtyObject(this);
+    _file?.dirtyObject(this);
   }
 
   bool wasModifiedAfter(KdbxObject other) => times.lastModificationTime
@@ -249,7 +259,7 @@ abstract class KdbxObject extends KdbxNode {
   void merge(MergeContext mergeContext, covariant KdbxObject other);
 
   bool isInRecycleBin() {
-    final targetGroup = file!.recycleBin;
+    final targetGroup = file.recycleBin;
     if (targetGroup == null) {
       return false;
     }

lib/src/kdbx_xml.dart

@@ -6,7 +6,6 @@ import 'package:collection/collection.dart' show IterableExtension;
 import 'package:kdbx/src/kdbx_consts.dart';
 import 'package:kdbx/src/kdbx_exceptions.dart';
 import 'package:kdbx/src/kdbx_format.dart';
-import 'package:kdbx/src/kdbx_header.dart';
 import 'package:kdbx/src/kdbx_object.dart';
 import 'package:kdbx/src/utils/byte_utils.dart';
 import 'package:logging/logging.dart';

lib/src/utils/sequence.dart

@@ -0,0 +1,28 @@
+import 'package:clock/clock.dart';
+
+/// Simple class to assign a unique integer for any point in time.
+/// This is basically to ensure that even if two events happen at the
+/// same millisecond we know which came first.
+/// (realistically this will only make a difference in tests).
+class TimeSequence {
+  TimeSequence._(this._sequenceIndex);
+  factory TimeSequence.now() => TimeSequence._(_sequenceCounter++);
+
+  static int _sequenceCounter = 0;
+
+  final int _sequenceIndex;
+  final DateTime _date = clock.now();
+
+  bool isAfter(TimeSequence other) {
+    return _sequenceIndex > other._sequenceIndex;
+  }
+
+  bool isBefore(TimeSequence other) {
+    return _sequenceIndex < other._sequenceIndex;
+  }
+
+  @override
+  String toString() {
+    return '{Sequence: $_sequenceIndex time: $_date}';
+  }
+}

test/kdbx_dirty_save_test.dart

@@ -0,0 +1,58 @@
+import 'package:kdbx/kdbx.dart';
+import 'package:test/test.dart';
+
+import 'internal/test_utils.dart';
+
+void main() {
+  final testUtil = TestUtil();
+  group('test save with dirty objects', () {
+    test('modify object after save', () async {
+      final file = testUtil.createEmptyFile();
+      final group = file.body.rootGroup;
+      final entry = testUtil.createEntry(file, group, 'user', 'pass');
+      final entry2 = testUtil.createEntry(file, group, 'user', 'pass');
+      await file.save();
+
+      const value1 = 'new';
+      const value2 = 'new2';
+      entry.setString(TestUtil.keyTitle, PlainValue(value1));
+      entry2.setString(TestUtil.keyTitle, PlainValue(value1));
+      expect(file.isDirty, isTrue);
+
+      await file.save((bytes) async {
+        // must still be dirty as long as we are not finished saving.
+        expect(file.isDirty, isTrue);
+        expect(entry.isDirty, isTrue);
+        expect(entry2.isDirty, isTrue);
+        return 1;
+      });
+      expect(file.isDirty, isFalse);
+      expect(entry.isDirty, isFalse);
+      expect(entry2.isDirty, isFalse);
+    });
+    test('parallel modify', () async {
+      final file = testUtil.createEmptyFile();
+      final group = file.body.rootGroup;
+      final entry = testUtil.createEntry(file, group, 'user', 'pass');
+      final entry2 = testUtil.createEntry(file, group, 'user', 'pass');
+      await file.save();
+
+      const value1 = 'new';
+      const value2 = 'new2';
+
+      entry.setString(TestUtil.keyTitle, PlainValue(value2));
+      entry2.setString(TestUtil.keyTitle, PlainValue(value2));
+      await file.save((bytes) async {
+        // must still be dirty as long as we are not finished saving.
+        expect(file.isDirty, isTrue);
+        expect(entry.isDirty, isTrue);
+        expect(entry2.isDirty, isTrue);
+        entry2.setString(TestUtil.keyTitle, PlainValue(value1));
+        return 1;
+      });
+      expect(file.isDirty, isTrue);
+      expect(entry.isDirty, isFalse);
+      expect(entry2.isDirty, isTrue);
+    });
+  });
+}