[SuperEditor] Split incompatible attributions (Resolves #1983) #2035

angelosilvestre · 2024-05-25T16:40:42Z

[SuperEditor] Split incompatible attributions. Resolves #1983

Valued attributions, like ColorAttribution can't be applied in a range where another ColorAttribution exists.

Although it doesn't make sense to have two ColorAttributions in the same range, users should be able to change the color of an already attributed range.

For example, consider the following range, fully attributed with blue:

   blue green blue
b [xxxxxxxxxxxxxxx]

Users should be able to apply green color to the word "green", causing the blue attributed range to be split:

   blue green blue
b [xxxxx-----xxxxx]
g [-----xxxxx-----]

This PR changes AddTextAttributionsCommand and ToggleTextAttributionsCommand to find and remove any conflicting attributions in the range before trying to apply the desired attribution.

The following changes were made:

Introduce AttributedSpans.findConflictingAttributions to return a list of conflicting attributions.
Change AddTextAttributionsCommand and ToggleTextAttributionsCommand to remove conflicting attributions
Update the tests that previously ensured we couldn't apply conflicting attributions

Also, I found a bug in AttributedSpans.getMatchingAttributionsWithin, where we always tried to get the attributions from the same index. This PR fixes this bug.

We'll need to release a new version of attributed_text because of this new method.

angelosilvestre · 2024-05-25T16:51:15Z

@KevinBrendel Could you please try this PR?

matthew-carroll · 2024-05-25T23:30:32Z

@angelosilvestre I'm wondering if an AttributedText user should be able to insert a span of green within blue and have that issue automatically handled by AttributedText. What do you think?

Requiring explicit intervention on this point might proliferate these same behaviors through different commands. Also every app that uses AttributedText would have to do the same thing for a similar situation.

Being able to find conflicting attribution spans is probably fine either way, but I'm thinking it might make more sense to do something like splitting blue, and adding green, within AttributedText, itself.

angelosilvestre · 2024-05-26T11:00:37Z

@matthew-carroll I guess it should be fine to handle that automatically.

Can you think of an use case where an AttributedText should prevent an attribution addition to cause a conflicting attribution to be split?

KevinBrendel · 2024-05-26T11:47:39Z

@angelosilvestre Thanks for the PR!
I tried it and it generally does exactly what is needed.

For my purposes, the editor-level commands are what is needed, but I can see the argument for adding this feature directly into AttributedText.

I also found one issue, which is likely related to this:
If the SuperEditor has a selectedTextColorStrategy which modifies the color, there can be exceptions triggered by addAttribution being called on AttributedSpans.

matthew-carroll · 2024-05-26T19:56:38Z

Can you think of an use case where an AttributedText should prevent an attribution addition to cause a conflicting attribution to be split?

Not off the top of my head. But you could add a boolean to whichever methods are relevant. Maybe by default we automatically split, but if you set the flag we'll throw an exception when you try to do that.

matthew-carroll · 2024-05-26T19:57:33Z

If the SuperEditor has a selectedTextColorStrategy which modifies the color, there can be exceptions triggered by addAttribution being called on AttributedSpans.

@KevinBrendel - Please be sure to provide reproduction steps and a code sample if you run into issues so that @angelosilvestre can reproduce it and fix it.

KevinBrendel · 2024-05-26T22:09:51Z

@KevinBrendel - Please be sure to provide reproduction steps and a code sample if you run into issues so that @angelosilvestre can reproduce it and fix it.

Will try to create a minimal sample when I have some more time tomorrow. Doesn't seem to be a very special case that triggers it.

KevinBrendel · 2024-05-27T08:55:04Z

Here is the reproduction. Just try to select the red text ("Hello"):

import 'package:flutter/material.dart';
import 'package:super_editor/super_editor.dart';

void main() {
  runApp(const MainApp());
}

class MainApp extends StatefulWidget {
  const MainApp({super.key});

  @override
  State<MainApp> createState() => _MainAppState();
}

class _MainAppState extends State<MainApp> {
  static const _colorAttribution = ColorAttribution(Colors.red);
  final _document = MutableDocument(nodes: [
    ParagraphNode(
      id: Editor.createNodeId(),
      text: AttributedText(
          'Hello, world!',
          AttributedSpans(attributions: const [
            SpanMarker(attribution: _colorAttribution, offset: 0, markerType: SpanMarkerType.start),
            SpanMarker(attribution: _colorAttribution, offset: 4, markerType: SpanMarkerType.end),
          ])),
    ),
  ]);
  final _composer = MutableDocumentComposer();
  late final Editor _editor;

  @override
  void initState() {
    super.initState();
    _editor = createDefaultDocumentEditor(document: _document, composer: _composer);
  }

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: Center(
          child: SuperEditor(
            editor: _editor,
            document: _document,
            composer: _composer,
            stylesheet: defaultStylesheet.copyWith(
              selectedTextColorStrategy: ({required originalTextColor, required selectionHighlightColor}) =>
                  Colors.grey,
            ),
          ),
        ),
      ),
    );
  }
}

angelosilvestre · 2024-05-27T22:38:49Z

@matthew-carroll Modified the implementation to handle attribution splitting directly in AttributedSpans.

angelosilvestre · 2024-05-27T22:45:46Z

@matthew-carroll This flag might be a bit unintuitive, since we already have a allowMerge parameter.

With allowMerge == false, even mergeable attributions cause the add method to throw.

attributed_text/lib/src/attributed_spans.dart

matthew-carroll · 2024-05-28T02:28:31Z

attributed_text/lib/src/attributed_spans.dart

+  ///
+  /// If [allowMerging] is `false`, attributions with the same id will always be reported
+  /// as conflicting, even if [Attribution.canMergeWith] returns `true`.
+  List<AttributionConflict> findConflictingAttributions({


To verify, do you want this to be a public API? Do you think it should be?

It might be a good idea not to hide too much functionality from the user. Some use-cases require some more low level knowledge/control and I have had to copy SuperEditor code into my app code on multiple occasions because it was private.

If you find yourself doing that, you should file issue tickets to let us know where/why that's happening. Otherwise, the only predictable result is that it will continue.

I haven't heard about anyone needing to find conflicting attributions in their own apps. Have you needed to do that? If so, what was the use-case?

There's obviously a middle ground that needs to be maintained. Making everything public creates a large maintenance space and makes every change a breaking change. Making everything private forces developers to request every little thing they want, and also to copy lots of code. We need to operate in a middle ground where we use some level of assessment as to what we publish and what we don't.

I am not 100% sure yet wether we will need this specific function, as implementation of the related features will only commence after this PR is merged.

It is just the kind of helper function that could be useful for some probably unforeseen use-cases and is currently often private. E.g. _textInSelection from CommonEditorOperations was very useful to build a search feature that already pre-populates the search field with the currently selected text upon pressing CMD+F.

I can create issues/pull requests for such things, but I also understand that it increases the maintenance burden. And I expect that dealing with all kinds of minor issues/pull requests would also serve as a distraction from the more important issues you guys are working on.

Currently I am trying to only create issues/pull requests when it seems something is more generally applicable, as our use-case is probably a bit more exotic (a hybrid flowchart/mind mapping app where there can be hundreds of nodes on a canvas, each containing a SuperEditor which has to interact with our node and graph-level systems for styling, undo/redo, etc. and which has to be integrated into often complex interactions requiring fine-grained control).

Filing issues is pretty much free. It only takes the time needed to explain the use-case/motivation. No need to submit PRs for those things. We can consider the motivation and decide whether we think it warrants a public API.

I guess we don't need to expose this method.

matthew-carroll · 2024-05-28T02:29:00Z

attributed_text/lib/src/attributed_spans.dart

+/// A conflict between the [newAttribution] and [existingAttribution] between [conflictStart] and [conflictEnd] (inclusive).
+///
+/// This means [newAttribution] and [existingAttribution] have the same id, but they can't be merged.
+class AttributionConflict {


Verifying that you think this should be public.

You resolved this but there's no comment here on why this should be public, nor do I think I saw any comment on the PR about this...

It seems I forgot to make it private.

attributed_text/test/attributed_spans_test.dart

matthew-carroll · 2024-05-28T02:33:53Z

super_editor/test/super_editor/supereditor_attributions_test.dart

@@ -1276,108 +1276,201 @@ void main() {
      });
    });

-    testWidgetsOnArbitraryDesktop('does not merge different colors', (tester) async {
-      final context = await tester //
+    testWidgetsOnArbitraryDesktop('splits spans with different colors', (tester) async {


Let's also revisit the use of the term "split" in this test suite to make sure it's always the best name for the given situation.

I think "overwrite" is a better name for all of those tests.

attributed_text/lib/src/attributed_spans.dart

matthew-carroll · 2024-05-30T20:00:42Z

attributed_text/lib/src/attributed_spans.dart

+      );
+    }
+
+    if (!ignoreMergeableOverlaps) {


At this point in the code, why does ignoreMergeableOverlaps matter? Either there aren't any overlaps, in which case this value doesn't matter, or there are overlaps and you've already thrown an exception, right?

This code is needed for the cases where we try to apply a mergeable attribution right after another one. For example:

|one two| |bbb----|

When ignoreMergeableOverlaps is true, applying a bold attribution to the span " two" produces a single span covering the entire text:

|one two| |bbbbbbb|

When ignoreMergeableOverlaps is false, applying a bold attribution to the span " two " produces two spans:

|one two| |bbb----| |---bbbb|

matthew-carroll · 2024-05-30T20:01:13Z

attributed_text/lib/src/attributed_spans.dart

+/// A conflict between the [newAttribution] and [existingAttribution] between [conflictStart] and [conflictEnd] (inclusive).
+///
+/// This means [newAttribution] and [existingAttribution] have the same id, but they can't be merged.
+class AttributionConflict {


You resolved this but there's no comment here on why this should be public, nor do I think I saw any comment on the PR about this...

attributed_text/test/attributed_spans_test.dart

matthew-carroll · 2024-05-30T20:04:35Z

attributed_text/test/attributed_spans_test.dart

+  required int startOffset,
+  required int endOffset,
+}) {
+  return AttributedSpans(


Should we add a constructor that's more concise? For example: AttributedSpans.single(attribution, start, end)?

It seems that after this isn't being used anymore... I removed it

super_editor/lib/src/default_editor/text.dart

matthew-carroll · 2024-05-30T20:08:18Z

super_editor/lib/src/default_editor/text.dart

-            );
+            node.text.spans.copy()
+              ..addAttribution(
+                newAttribution: attribution,


Let's also leave this parameter called attribution to avoid unnecessary breaking changes.

The addAttribution used before this PR is from AttributedText. Now we are using the method from AttributedSpans. We are not changing the parameter names.

matthew-carroll

LGTM

…1983) (superlistapp#2035)

KevinBrendel · 2024-06-11T18:37:08Z

Here is the reproduction. Just try to select the red text ("Hello"):

import 'package:flutter/material.dart';
import 'package:super_editor/super_editor.dart';

void main() {
  runApp(const MainApp());
}

class MainApp extends StatefulWidget {
  const MainApp({super.key});

  @override
  State<MainApp> createState() => _MainAppState();
}

class _MainAppState extends State<MainApp> {
  static const _colorAttribution = ColorAttribution(Colors.red);
  final _document = MutableDocument(nodes: [
    ParagraphNode(
      id: Editor.createNodeId(),
      text: AttributedText(
          'Hello, world!',
          AttributedSpans(attributions: const [
            SpanMarker(attribution: _colorAttribution, offset: 0, markerType: SpanMarkerType.start),
            SpanMarker(attribution: _colorAttribution, offset: 4, markerType: SpanMarkerType.end),
          ])),
    ),
  ]);
  final _composer = MutableDocumentComposer();
  late final Editor _editor;

  @override
  void initState() {
    super.initState();
    _editor = createDefaultDocumentEditor(document: _document, composer: _composer);
  }

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: Center(
          child: SuperEditor(
            editor: _editor,
            document: _document,
            composer: _composer,
            stylesheet: defaultStylesheet.copyWith(
              selectedTextColorStrategy: ({required originalTextColor, required selectionHighlightColor}) =>
                  Colors.grey,
            ),
          ),
        ),
      ),
    );
  }
}

@matthew-carroll @angelosilvestre This was not fixed and there is not way around it as the corresponding implementation hardcodes overwriteConflictingSpans to false.

Is this intentional, and if so, is it possible to work around this?

Otherwise, selectedTextColorStrategy will stay unusable in combination with ColorAttribution.

matthew-carroll · 2024-06-11T19:11:06Z

@KevinBrendel can you please file that reproduction and the problem as a new issue? I'll have @angelosilvestre investigate it when you do.

angelosilvestre added 2 commits May 25, 2024 13:27

[SuperEditor] Split incompatible attributions (Resolves 1983)

eb1dd60

Remove commented code

0364bed

angelosilvestre requested a review from matthew-carroll May 25, 2024 16:51

Implement spliting in AttributedSpans

15ec498

Fix failing test

687d6e0

matthew-carroll reviewed May 28, 2024

View reviewed changes

angelosilvestre added 2 commits May 29, 2024 20:04

PR updates

688e5a4

Missing file

6c0224b

angelosilvestre requested a review from matthew-carroll May 29, 2024 23:17

matthew-carroll reviewed May 30, 2024

View reviewed changes

PR updates

e325c5a

angelosilvestre requested a review from matthew-carroll May 31, 2024 22:55

matthew-carroll approved these changes Jun 1, 2024

View reviewed changes

matthew-carroll merged commit 776dd4d into main Jun 1, 2024
11 checks passed

matthew-carroll deleted the 1983_split-attributions branch June 1, 2024 01:07

github-actions bot pushed a commit that referenced this pull request Jun 1, 2024

[SuperEditor] Split incompatible attributions (Resolves #1983) (#2035)

7807c5b

github-actions bot mentioned this pull request Jun 1, 2024

Cherry Pick: [SuperEditor] Split incompatible attributions (Resolves #1983) #2067

Merged

matthew-carroll pushed a commit that referenced this pull request Jun 1, 2024

[SuperEditor] Split incompatible attributions (Resolves #1983) (#2035)

86ca9d2

quaaantumdev pushed a commit to quaaantumdev/super_editor that referenced this pull request Jun 1, 2024

[SuperEditor] Split incompatible attributions (Resolves superlistapp#…

6c86abc

…1983) (superlistapp#2035)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperEditor] Split incompatible attributions (Resolves #1983) #2035

[SuperEditor] Split incompatible attributions (Resolves #1983) #2035

angelosilvestre commented May 25, 2024 •

edited

Loading

angelosilvestre commented May 25, 2024

matthew-carroll commented May 25, 2024

angelosilvestre commented May 26, 2024

KevinBrendel commented May 26, 2024 •

edited

Loading

matthew-carroll commented May 26, 2024

matthew-carroll commented May 26, 2024

KevinBrendel commented May 26, 2024

KevinBrendel commented May 27, 2024 •

edited

Loading

angelosilvestre commented May 27, 2024

angelosilvestre commented May 27, 2024

matthew-carroll May 28, 2024

KevinBrendel May 28, 2024

matthew-carroll May 28, 2024

KevinBrendel May 29, 2024

matthew-carroll May 29, 2024

angelosilvestre May 29, 2024

matthew-carroll May 28, 2024

matthew-carroll May 30, 2024

angelosilvestre May 31, 2024

matthew-carroll May 28, 2024

angelosilvestre May 29, 2024

matthew-carroll May 30, 2024

angelosilvestre May 31, 2024

matthew-carroll May 30, 2024

matthew-carroll May 30, 2024

angelosilvestre May 31, 2024

matthew-carroll May 30, 2024

angelosilvestre May 31, 2024

matthew-carroll left a comment

KevinBrendel commented Jun 11, 2024

matthew-carroll commented Jun 11, 2024

[SuperEditor] Split incompatible attributions (Resolves #1983) #2035

[SuperEditor] Split incompatible attributions (Resolves #1983) #2035

Conversation

angelosilvestre commented May 25, 2024 • edited Loading

angelosilvestre commented May 25, 2024

matthew-carroll commented May 25, 2024

angelosilvestre commented May 26, 2024

KevinBrendel commented May 26, 2024 • edited Loading

matthew-carroll commented May 26, 2024

matthew-carroll commented May 26, 2024

KevinBrendel commented May 26, 2024

KevinBrendel commented May 27, 2024 • edited Loading

angelosilvestre commented May 27, 2024

angelosilvestre commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll left a comment

Choose a reason for hiding this comment

KevinBrendel commented Jun 11, 2024

matthew-carroll commented Jun 11, 2024

angelosilvestre commented May 25, 2024 •

edited

Loading

KevinBrendel commented May 26, 2024 •

edited

Loading

KevinBrendel commented May 27, 2024 •

edited

Loading