Merge pull request #825 from AppFlowy-IO/fix/undo-attributes

Fix: undo attributes

frontend/app_flowy/packages/flowy_editor/lib/src/document/attributes.dart

@@ -22,11 +22,15 @@ Attributes invertAttributes(Attributes? attr, Attributes? base) {
   });
 }
 
-Attributes? composeAttributes(Attributes? a, Attributes? b) {
+Attributes? composeAttributes(Attributes? a, Attributes? b,
+    [bool keepNull = false]) {
   a ??= {};
   b ??= {};
-  final Attributes attributes = {};
-  attributes.addAll(Map.from(b)..removeWhere((_, value) => value == null));
+  Attributes attributes = {...b};
+
+  if (!keepNull) {
+    attributes = Map.from(attributes)..removeWhere((_, value) => value == null);
+  }
 
   for (final entry in a.entries) {
     if (!b.containsKey(entry.key)) {
@@ -34,9 +38,5 @@ Attributes? composeAttributes(Attributes? a, Attributes? b) {
     }
   }
 
-  if (attributes.isEmpty) {
-    return null;
-  }
-
-  return attributes;
+  return attributes.isNotEmpty ? attributes : null;
 }

frontend/app_flowy/packages/flowy_editor/lib/src/document/node.dart

@@ -8,7 +8,7 @@ class Node extends ChangeNotifier with LinkedListEntry<Node> {
   Node? parent;
   final String type;
   final LinkedList<Node> children;
-  final Attributes attributes;
+  Attributes _attributes;
 
   GlobalKey? key;
   // TODO: abstract a selectable node??
@@ -16,22 +16,24 @@ class Node extends ChangeNotifier with LinkedListEntry<Node> {
 
   String? get subtype {
     // TODO: make 'subtype' as a const value.
-    if (attributes.containsKey('subtype')) {
-      assert(attributes['subtype'] is String?,
+    if (_attributes.containsKey('subtype')) {
+      assert(_attributes['subtype'] is String?,
           'subtype must be a [String] or [null]');
-      return attributes['subtype'] as String?;
+      return _attributes['subtype'] as String?;
     }
     return null;
   }
 
   Path get path => _path();
 
+  Attributes get attributes => _attributes;
+
   Node({
     required this.type,
     required this.children,
-    required this.attributes,
+    required Attributes attributes,
     this.parent,
-  }) {
+  }) : _attributes = attributes {
     for (final child in children) {
       child.parent = this;
     }
@@ -84,16 +86,9 @@ class Node extends ChangeNotifier with LinkedListEntry<Node> {
   }
 
   void updateAttributes(Attributes attributes) {
-    bool shouldNotifyParent =
-        this.attributes['subtype'] != attributes['subtype'];
-
-    for (final attribute in attributes.entries) {
-      if (attribute.value == null) {
-        this.attributes.remove(attribute.key);
-      } else {
-        this.attributes[attribute.key] = attribute.value;
-      }
-    }
+    bool shouldNotifyParent = _attributes['subtype'] != attributes['subtype'];
+
+    _attributes = composeAttributes(_attributes, attributes) ?? {};
     // Notify the new attributes
     // if attributes contains 'subtype', should notify parent to rebuild node
     // else, just notify current node.
@@ -149,8 +144,8 @@ class Node extends ChangeNotifier with LinkedListEntry<Node> {
     if (children.isNotEmpty) {
       map['children'] = children.map((node) => node.toJson());
     }
-    if (attributes.isNotEmpty) {
-      map['attributes'] = attributes;
+    if (_attributes.isNotEmpty) {
+      map['attributes'] = _attributes;
     }
     return map;
   }
@@ -214,7 +209,7 @@ class TextNode extends Node {
       TextNode(
         type: type ?? this.type,
         children: children ?? this.children,
-        attributes: attributes ?? this.attributes,
+        attributes: attributes ?? _attributes,
         delta: delta ?? this.delta,
       );
 

frontend/app_flowy/packages/flowy_editor/lib/src/document/state_tree.dart

@@ -65,16 +65,15 @@ class StateTree {
     }
   }
 
-  Attributes? update(Path path, Attributes attributes) {
+  bool update(Path path, Attributes attributes) {
     if (path.isEmpty) {
-      return null;
+      return false;
     }
     final updatedNode = root.childAtPath(path);
     if (updatedNode == null) {
-      return null;
+      return false;
     }
-    final previousAttributes = Attributes.from(updatedNode.attributes);
     updatedNode.updateAttributes(attributes);
-    return previousAttributes;
+    return true;
   }
 }

frontend/app_flowy/packages/flowy_editor/lib/src/document/text_delta.dart

@@ -383,8 +383,8 @@ class Delta extends Iterable<TextOperation> {
         final length = min(thisIter.peekLength(), otherIter.peekLength());
         final thisOp = thisIter.next(length);
         final otherOp = otherIter.next(length);
-        final attributes =
-            composeAttributes(thisOp.attributes, otherOp.attributes);
+        final attributes = composeAttributes(
+            thisOp.attributes, otherOp.attributes, thisOp is TextRetain);
         if (otherOp is TextRetain && otherOp.length > 0) {
           TextOperation? newOp;
           if (thisOp is TextRetain) {

frontend/app_flowy/packages/flowy_editor/lib/src/operation/transaction_builder.dart

@@ -40,10 +40,12 @@ class TransactionBuilder {
 
   updateNode(Node node, Attributes attributes) {
     beforeSelection = state.cursorSelection;
+
+    final inverted = invertAttributes(attributes, node.attributes);
     add(UpdateOperation(
       node.path,
-      Attributes.from(node.attributes)..addAll(attributes),
-      node.attributes,
+      {...attributes},
+      inverted,
     ));
   }
 

frontend/app_flowy/packages/flowy_editor/test/delta_test.dart

@@ -1,3 +1,4 @@
+import 'package:flowy_editor/src/document/attributes.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:flowy_editor/src/document/text_delta.dart';
 
@@ -240,7 +241,8 @@ void main() {
         ..retain(3, {'bold': null});
       final inverted = delta.invert(base);
       expect(expected, inverted);
-      expect(base.compose(delta).compose(inverted), base);
+      final t = base.compose(delta).compose(inverted);
+      expect(t, base);
     });
   });
   group('json', () {
@@ -314,4 +316,14 @@ void main() {
       expect(delta.prevRunePosition(0), -1);
     });
   });
+  group("attributes", () {
+    test("compose", () {
+      final attrs = composeAttributes({"a": null}, {"b": null}, true);
+      expect(attrs != null, true);
+      expect(attrs!.containsKey("a"), true);
+      expect(attrs.containsKey("b"), true);
+      expect(attrs["a"], null);
+      expect(attrs["b"], null);
+    });
+  });
 }

frontend/app_flowy/packages/flowy_editor/test/flowy_editor_test.dart

@@ -68,9 +68,8 @@ void main() {
     final String response = await rootBundle.loadString('assets/document.json');
     final data = Map<String, Object>.from(json.decode(response));
     final stateTree = StateTree.fromJson(data);
-    final attributes = stateTree.update([1, 1], {'text-type': 'heading1'});
-    expect(attributes != null, true);
-    expect(attributes!['text-type'], 'checkbox');
+    final test = stateTree.update([1, 1], {'text-type': 'heading1'});
+    expect(test, true);
     final updatedNode = stateTree.nodeAtPath([1, 1]);
     expect(updatedNode != null, true);
     expect(updatedNode!.attributes['text-type'], 'heading1');