Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AttributedText: collapsedSpans returns overly large MultiAttributionSpan for unattributed string #632

Closed
domesticmouse opened this issue Jun 20, 2022 · 5 comments · Fixed by #1765 · May be fixed by #642
Closed

AttributedText: collapsedSpans returns overly large MultiAttributionSpan for unattributed string #632

domesticmouse opened this issue Jun 20, 2022 · 5 comments · Fixed by #1765 · May be fixed by #642
Assignees
@angelosilvestre
Labels
area_attributed_text Related to attributed_text bounty_junior f:superlist Funded by Superlist time: 1 1hr or less

Comments

@domesticmouse
Copy link
Contributor

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
Copy link
Contributor Author

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.

@domesticmouse domesticmouse mentioned this issue Jun 20, 2022
Open
@jmatth
Copy link
Contributor

jmatth commented Jun 23, 2022

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.

@jmatth jmatth mentioned this issue Jun 23, 2022
Open
@domesticmouse
Copy link
Contributor Author

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

@jmatth
Copy link
Contributor

jmatth commented Jun 24, 2022

@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
Copy link
Contributor Author

Sgtm!

@matthew-carroll matthew-carroll added bounty_junior f:superlist Funded by Superlist time: 1 1hr or less area_attributed_text Related to attributed_text labels Dec 13, 2023
angelosilvestre added a commit that referenced this issue Jan 8, 2024
@angelosilvestre
[attributed_text] Fix invalid end index with an marker that exceeds t…
44589c0 
…ext length (Resolves #632)
@angelosilvestre angelosilvestre mentioned this issue Jan 8, 2024
Merged
matthew-carroll pushed a commit that referenced this issue Jan 9, 2024
@angelosilvestre
[attributed_text] Fix invalid end index with an marker that exceeds t…
06b89b4 
…ext length (Resolves #632) (#1765)
github-actions bot pushed a commit that referenced this issue Jan 9, 2024
@angelosilvestre @web-flow
[attributed_text] Fix invalid end index with an marker that exceeds t…
5385143 
…ext length (Resolves #632) (#1765)
matthew-carroll pushed a commit that referenced this issue Jan 9, 2024
@angelosilvestre @matthew-carroll
[attributed_text] Fix invalid end index with an marker that exceeds t…
5c60045 
…ext length (Resolves #632) (#1765)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@angelosilvestre angelosilvestre

Labels
area_attributed_text Related to attributed_text bounty_junior f:superlist Funded by Superlist time: 1 1hr or less
Projects
None yet
Milestone
No milestone
4 participants
@domesticmouse @jmatth @matthew-carroll @angelosilvestre