Repository: superlistapp/super_editor (dart)

Issue:
[AttributedText][Bug] - Wrong placeholder map order breaks constructor
It looks like our `AttributedText` constructor is order-dependent on the entries in the placeholder map. This is undocumented and unintentional. Make the constructor order agnostic.

Pull request:
[AttributedText] - Make constructor agnostic to placeholder order in placeholder map (Resolves #2786)

Before this PR, if someone passes in an inline placeholder map into `AttributedText`, and the entries in that map aren't in the same order as the placeholder text positions, the constructor will blow up. This PR sorts them in the constructor to avoid that issue.

Patch:
diff --git a/attributed_text/lib/src/attributed_text.dart b/attributed_text/lib/src/attributed_text.dart
index b6dfc42893..048b879b6c 100644
--- a/attributed_text/lib/src/attributed_text.dart
+++ b/attributed_text/lib/src/attributed_text.dart
@@ -51,8 +51,14 @@ class AttributedText {
     AttributedSpans? spans,
     Map<int, Object>? placeholders,
   ])  : _text = text ?? "",
-        spans = spans ?? AttributedSpans(),
-        placeholders = placeholders ?? <int, Object>{} {
+        spans = spans ?? AttributedSpans() {
+    // Sort the placeholders map so that we can always assume the entries
+    // iterate in the placeholder content order. This knowledge is relied upon
+    // in multiple places in this class.
+    this.placeholders = Map.fromEntries(
+      placeholders?.entries.sorted((a, b) => a.key - b.key) ?? [],
+    );
+
     assert(() {
       // ^ Run this in an assert with a callback so that the validation doesn't run in
       //   production and cost processor cycles.
@@ -70,6 +76,7 @@ class AttributedText {
       final buffer = StringBuffer();
       int start = 0;
       int insertedPlaceholders = 0;
+
       for (final entry in this.placeholders.entries) {
         final textSegment = _text.substring(start - insertedPlaceholders, entry.key - insertedPlaceholders);
         buffer.write(textSegment);
@@ -160,8 +167,11 @@ class AttributedText {
   /// Placeholders that represent non-text content, e.g., inline images, that
   /// should appear in the rendered text.
   ///
+  /// The entries in this map are in content-order, e.g., the entry for a placeholder
+  /// at index 3 comes before an entry whose placeholder is at index 5.
+  ///
   /// In terms of [length], each placeholder is treated as a single character.
-  final Map<int, Object> placeholders;
+  late final Map<int, Object> placeholders;
 
   /// Returns the `length` of this [AttributedText], which includes the length
   /// of the plain text `String`, and the number of [placeholders].


Test patch:
diff --git a/attributed_text/test/attributed_text_placeholders_test.dart b/attributed_text/test/attributed_text_placeholders_test.dart
index 1f0b283b23..562fd4ccdb 100644
--- a/attributed_text/test/attributed_text_placeholders_test.dart
+++ b/attributed_text/test/attributed_text_placeholders_test.dart
@@ -21,6 +21,40 @@ void main() {
           throwsA(isA<AssertionError>()),
         );
       });
+
+      test("is order agnostic", () {
+        // Ensure that constructors don't below up when the placeholder
+        // entry order isn't the same as content order, and also ensure that
+        // equality doesn't care about the map order.
+
+        final order1 = AttributedText("Hello, World!", null, {
+          3: const _FakePlaceholder("first"),
+          6: const _FakePlaceholder("second"),
+          9: const _FakePlaceholder("third"),
+        });
+
+        final order2 = AttributedText("Hello, World!", null, {
+          6: const _FakePlaceholder("second"),
+          3: const _FakePlaceholder("first"),
+          9: const _FakePlaceholder("third"),
+        });
+
+        final order3 = AttributedText("Hello, World!", null, {
+          6: const _FakePlaceholder("second"),
+          9: const _FakePlaceholder("third"),
+          3: const _FakePlaceholder("first"),
+        });
+
+        final order4 = AttributedText("Hello, World!", null, {
+          9: const _FakePlaceholder("third"),
+          6: const _FakePlaceholder("second"),
+          3: const _FakePlaceholder("first"),
+        });
+
+        expect(order1, equals(order2));
+        expect(order1, equals(order3));
+        expect(order1, equals(order4));
+      });
     });
 
     group("length >", () {