AttributedText: collapsedSpans returns overly large MultiAttributionSpan for unattributed string

[domesticmouse]

Here's my expectation encoded as a test.

    test('Simple AttributedText', () {
      final text = AttributedText(text: 'Hello World');
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(1));
      expect(spans[0].attributions.isEmpty, equals(true));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length));
    });

The background use case is that I'm trying to convert an AttributedText back into TextSpans for display purposes. Halp?

[domesticmouse]

It gets more interesting:

  group('AttributedText collapsedSpans', () {
    const boldAttribution = NamedAttribution('bold');

    test('Simple AttributedText', () {
      final text =
          AttributedText(text: 'Hello World', spans: AttributedSpans());
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(1));
      expect(spans[0].attributions.isEmpty, equals(true));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length));
    });

    test('Bold AttributedText', () {
      const content = 'Hello World';
      final text = AttributedText(
        text: content,
        spans: AttributedSpans(
          attributions: const [
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.start,
              offset: 0,
            ),
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.end,
              offset: content.length,
            ),
          ],
        ),
      );
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(1));
      expect(spans[0].attributions.isEmpty, equals(false));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length));
    });

    test('Half Bold start AttributedText', () {
      const content = 'Hello World';
      final text = AttributedText(
        text: content,
        spans: AttributedSpans(
          attributions: const [
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.start,
              offset: 0,
            ),
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.end,
              offset: content.length ~/ 2,
            ),
          ],
        ),
      );
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(2));

      expect(spans[0].attributions.isEmpty, equals(false));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length ~/ 2));

      expect(spans[1].attributions.isEmpty, equals(true));
      expect(spans[1].start, equals(text.text.length ~/ 2 + 1));
      expect(spans[1].end, equals(text.text.length));
    });

    test('Half Bold end AttributedText', () {
      const content = 'Hello World';
      final text = AttributedText(
        text: content,
        spans: AttributedSpans(
          attributions: const [
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.start,
              offset: content.length ~/ 2,
            ),
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.end,
              offset: content.length,
            ),
          ],
        ),
      );
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(2));

      expect(spans[0].attributions.isEmpty, equals(true));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length ~/ 2 - 1));

      expect(spans[1].attributions.isEmpty, equals(false));
      expect(spans[1].start, equals(text.text.length ~/ 2));
      expect(spans[1].end, equals(text.text.length));
    });
  });

Results in:

flutter test
00:01 +12 -1: /Users/brettmorgan/flutter-projects/simplistic_editor2/test/integration_test.dart: AttributedText collapsedSpans Simple AttributedText [E]                             
  Expected: <11>
    Actual: <10>
  
  package:test_api                                    expect
  package:flutter_test/src/widget_tester.dart 455:16  expect
  test/integration_test.dart 148:7                    main.<fn>.<fn>
  
00:01 +13 -2: /Users/brettmorgan/flutter-projects/simplistic_editor2/test/integration_test.dart: AttributedText collapsedSpans Half Bold start AttributedText [E]                    
  Expected: <11>
    Actual: <10>
  
  package:test_api                                    expect
  package:flutter_test/src/widget_tester.dart 455:16  expect
  test/integration_test.dart 205:7                    main.<fn>.<fn>
  
00:02 +15 -2: Some tests failed.                        

Which appears to mean my expectations are met as long as there is Attribute coverage at the end of the string.

[jmatth]

I think there are two things happening here:

1. The behavior of collapseSpans is to return spans where end is inclusive.
2. Given a span that ends at i, collapseSpans(i+1) will return a span that ends at i+1. In the context of the previous point, this is a bug caused by this line. The condition should be > contentLength - 1.

At minimum we should fix the bug in 2 but I think the better option is to revise the method's behavior to return spans where end is exclusive. This is probably what most users expect and plays nice with every other string or list related method across Dart and Flutter.

I have a branch where I've already implemented the change and will open a PR shortly.

[domesticmouse]

Please feel free to adopt the above tests if it helps. Happy to open a PR with the content if that's appropriate.

[jmatth]

@domesticmouse I have what I think is a working version of collapseSpans with exclusive end indexes but a couple of your tests are failing. However, I think the issue is in the tests and not the new implementation. The failing lines (indicated below with trailing comments) are both applying - 1 to expected indexes, which I think is incorrect.

In this test the start index in the failing line is calculated based on the end of the previous attribution. With exclusive end indexes, I would expect the start index should be equal to the previous end index, not the previous end index - 1.

      test('Half Bold start AttributedText', () {
      const content = 'Hello World';
      final text = AttributedText(
        text: content,
        spans: AttributedSpans(
          attributions: const [
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.start,
              offset: 0,
            ),
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.end,
              offset: content.length ~/ 2,
            ),
          ],
        ),
      );
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(2));

      expect(spans[0].attributions.isEmpty, equals(false));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length ~/ 2));

      expect(spans[1].attributions.isEmpty, equals(true));
      expect(spans[1].start, equals(text.text.length ~/ 2 + 1));  // FAIL - Expected: <6>, Actual: <5>
      expect(spans[1].end, equals(text.text.length));
    });

In this one - 1 is being applied to the end index. I'm guessing this comes from some edge cases in the old behavior with inclusive end indexes, but with the new behavior no such index manipulation takes place.

    test('Half Bold end AttributedText', () {
      const content = 'Hello World';
      final text = AttributedText(
        text: content,
        spans: AttributedSpans(
          attributions: const [
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.start,
              offset: content.length ~/ 2,
            ),
            SpanMarker(
              attribution: boldAttribution,
              markerType: SpanMarkerType.end,
              offset: content.length,
            ),
          ],
        ),
      );
      final spans = text.spans.collapseSpans(contentLength: text.text.length);
      expect(spans.length, equals(2));

      expect(spans[0].attributions.isEmpty, equals(true));
      expect(spans[0].start, equals(0));
      expect(spans[0].end, equals(text.text.length ~/ 2 - 1));  // FAIL - Expected: <4>, Actual: <5>

      expect(spans[1].attributions.isEmpty, equals(false));
      expect(spans[1].start, equals(text.text.length ~/ 2));
      expect(spans[1].end, equals(text.text.length));
    });

Can you confirm whether removing those - 1 index modifications still match your expectations or is there something else I'm missing?

[domesticmouse]

Sgtm!