Repository: superlistapp/super_editor (dart)

Issue:
[attributed_text] AttributedSpans.hasAttributionsWithin incorrectly throws as it incorrectly iterates over a Set of attributes
The method AttributedSpans.hasAttributionsWithin iterates over a Set of attributes it want's to check and removes removes entries from this Set while iterating. This throws an exception (ConcurrentModificationError) as the iterator cannot be used after removing entries from the underlying it. This is a simple algorithmic bug / invalid usage of a set and it's iterator.

Here is the fix for this issue: https://github.com/superlistapp/super_editor/pull/1202
(previous pull request without a unit test was: https://github.com/superlistapp/super_editor/pull/1188)

Pull request description:
[attributed_text] Fix exception in hasAttributionsWithin. Resolves #1201.

This PR is built on top of https://github.com/superlistapp/super_editor/pull/1202

`hasAttributionsWithin` incorrectly removes attributions while iterating over a `Set`. This causes an exception when looking for multiple attributions at the same time.

This PR changes the method to only remove the attributions after iterating.

Patch:
diff --git a/attributed_text/lib/src/attributed_spans.dart b/attributed_text/lib/src/attributed_spans.dart
index d294a99b91..3b6545b03a 100644
--- a/attributed_text/lib/src/attributed_spans.dart
+++ b/attributed_text/lib/src/attributed_spans.dart
@@ -65,15 +65,21 @@ class AttributedSpans {
   }) {
     final attributionsToFind = Set.from(attributions);
     for (int i = start; i <= end; ++i) {
+      final foundAttributions = <Attribution>{};
       for (final attribution in attributionsToFind) {
         if (hasAttributionAt(i, attribution: attribution)) {
-          attributionsToFind.remove(attribution);
-        }
-
-        if (attributionsToFind.isEmpty) {
-          return true;
+          // Store the attributions we found so far to remove them
+          // from attributionsToFind after the loop.
+          //
+          // Removing from the set while iterating throws an exception.
+          foundAttributions.add(attribution);
         }
       }
+      attributionsToFind.removeAll(foundAttributions);
+      if (attributionsToFind.isEmpty) {
+        // We found all attributions.
+        return true;
+      }
     }
     return false;
   }


Test patch:
diff --git a/attributed_text/test/attributed_spans_test.dart b/attributed_text/test/attributed_spans_test.dart
index f1053ac662..d359805987 100644
--- a/attributed_text/test/attributed_spans_test.dart
+++ b/attributed_text/test/attributed_spans_test.dart
@@ -157,6 +157,53 @@ void main() {
         );
       });
 
+      test('hasAttributionsWithin can look for multiple attributions at the same time', () {
+        final spans = AttributedSpans()
+          ..addAttribution(newAttribution: ExpectedSpans.bold, start: 0, end: 8)
+          ..addAttribution(newAttribution: ExpectedSpans.italics, start: 1, end: 5)
+          ..addAttribution(newAttribution: ExpectedSpans.strikethrough, start: 5, end: 9);
+
+        ExpectedSpans(
+          [
+            'bbbbbbbbb_',
+            '_iiiii____',
+            '_____sssss',
+          ],
+        ).expectSpans(spans);
+
+        expect(
+          spans.hasAttributionsWithin(attributions: {
+            ExpectedSpans.bold,
+            ExpectedSpans.italics,
+            ExpectedSpans.strikethrough,
+          }, start: 0, end: 9),
+          true,
+        );
+
+        expect(
+          spans.hasAttributionsWithin(attributions: {
+            ExpectedSpans.bold,
+            ExpectedSpans.italics,
+          }, start: 0, end: 9),
+          true,
+        );
+
+        expect(
+            spans.hasAttributionsWithin(attributions: {
+              ExpectedSpans.bold,
+              ExpectedSpans.italics,
+            }, start: 0, end: 4),
+            true);
+
+        expect(
+          spans.hasAttributionsWithin(attributions: {
+            ExpectedSpans.bold,
+            ExpectedSpans.strikethrough,
+          }, start: 0, end: 4),
+          false,
+        );
+      });
+
       group('getAttributedRange', () {
         test('returns the range of a single attribution for an offset in the middle of a span', () {
           final spans = AttributedSpans(