[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2387

angelosilvestre · 2024-10-29T23:29:04Z

[SuperEditor] Limit tag expansion to caret position. Resolves #2240, #2241, #2242

Steps to reproduce:

Go to the "Action Tags" section of the example app
Type some text
Place the cursor in the middle of the text
Type "/"
Press backspace
Type "/" again

The editor crashes with the following exception:

════════ Exception caught by services library ══════════════════════════════════
The following _Exception was thrown during method call TextInputClient.updateEditingStateWithDeltas:
Exception: removeAttribution() did not satisfy start < 0 and start > end, start: 6, end: 5

When the exception was thrown, this was the stack:
#0      AttributedSpans.removeAttribution (package:attributed_text/src/attributed_spans.dart:514:7)
attributed_spans.dart:514
#1      RemoveTextAttributionsCommand.execute (package:super_editor/src/default_editor/text.dart:1439:15)
text.dart:1439
#2      _DocumentEditorCommandExecutor.executeCommand (package:super_editor/src/core/editor.dart:701:15)
editor.dart:701
#3      Editor._executeCommand (package:super_editor/src/core/editor.dart:304:22)
editor.dart:304
#4      Editor.execute (package:super_editor/src/core/editor.dart:266:30)
editor.dart:266
#5      ActionTagComposingReaction._healCancelledTags (package:super_editor/src/default_editor/text_tokenizing/action_tags.dart:364:23)
action_tags.dart:364
#6      ActionTagComposingReaction.react (package:super_editor/src/default_editor/text_tokenizing/action_tags.dart:286:5)
action_tags.dart:286
#7      Editor._reactToChanges (package:super_editor/src/core/editor.dart:357:16)
editor.dart:357
#8      Editor.endTransaction (package:super_editor/src/core/editor.dart:222:5)
editor.dart:222
... many more

While investigating this, I noticed that typing "/" at the middle of the word causes all the text that comes after the "/" to be part of the action tag:

Screen.Recording.2024-10-29.at.20.23.32.mov

This doesn't look like is expected.

This PR changes this behavior. Now, the tag is expanded only until the caret position:

Screen.Recording.2024-10-29.at.20.25.46.mov

Changing this seems to already fix the issue, and also #2241 and #2242

angelosilvestre · 2024-10-29T23:29:55Z

@alterhuman @KevinBrendel Would you mind trying this PR?

alterhuman · 2024-10-30T10:47:07Z

@angelosilvestre thanks for the PR

Working correctly:

Not crashing when deleting and adding a slash in the middle of a word.
Automatically cancelling after a space
Matching action only till caret position

Issues:

Moving caret behind the / while still being on the same word doesn't cancel the dropdown. To reproduce:

Type Some Information
Add / after o: So/me Information
Dropdown shows up, move caret behind /, the dropdown hides, then move it again infront of the / the dropdown is visible again but it should've cancelled.

Action tag dropdown popping up after reopening a document and focus the caret on the text containing the tag rule /, after the slash. To reproduce:

Instead of having an empty document add this text:

_document = MutableDocument(nodes: [
      ParagraphNode(
        id: Editor.createNodeId(),
        text: AttributedText("Some Infor/mation"),
      ),
    ]);

Focus caret at n, the dropdown pops up.

angelosilvestre · 2024-10-30T23:18:48Z

@alterhuman I fixed the first issue you mentioned. For the second issue, I'm not sure I followed. It doesn't seem to be related to any of the existing issues.

alterhuman · 2024-10-31T12:52:08Z

@angelosilvestre tested the first issue, it's working correctly. This is the video of second issue, this text is already present in the document when it is opened:

Screen.Recording.2024-10-31.at.6.16.39.PM.mov

Focusing on a word which already has a / inside it when you open the editor, shows the dropdown with the value being the text between / and the caret. This should only happen when I type a slash till the time it's cancelled. It's also happening on opening an editor and then focusing the caret near a word which already has a slash.

angelosilvestre · 2024-10-31T13:11:14Z

@alterhuman Is this happening on latest main? It doesn't look like it's related to the changes of this PR. If so, can you file a separate issue for this?

alterhuman · 2024-10-31T14:43:04Z

@angelosilvestre yes this is also happening on main. Will file a separate issue for this. All good for this PR.

matthew-carroll · 2024-11-05T23:28:18Z

super_editor/lib/src/default_editor/text_tokenizing/tags.dart

@@ -10,11 +10,15 @@ import 'package:super_editor/src/default_editor/text.dart';
 class TagFinder {
  /// Finds a tag that touches the given [expansionPosition] and returns that tag,
  /// indexed within the document, along with the [expansionPosition].
+  ///
+  /// If [endPosition] is provided, the search will be limited to the range between


I don't understand this description of endPosition. This method was intended to only look for tags that touch expansionPosition, which is a singular position. There shouldn't be a range involved. So the idea of an endPosition doesn't make sense to me.

So should we keep this method as it was before and modify the reaction to adjust the reported tag?

My point was that I don't know what you're trying to do here because it doesn't seem to make sense. I can't answer that question until I first understand what exactly you were trying to do.

What I'm trying to do is to prevent the tag to expand beyond the caret position. For example, if we have the string "helloworld" and the user types a "/" at the middle, changing it to "hello/world", the string "world" shouldn't be attributed as a tag. The user can input a tag in the middle of a word, but the tag shouldn't expand beyond the caret position.

I understand the feature goal. What I don't understand is the code that I commented on. I don't understand how/why achieving the feature goal that you described requires a caller to pass some thing called an "end position". Can you explain what this approach is doing and why it makes sense for the feature goal?

@angelosilvestre just need an update whether this is in progress or stagnant.

Hi @alterhuman. I'm resuming the work on this PR.

@alterhuman Can you try this PR again?

@angelosilvestre tested everything. All working correctly like previous solution except the previous remaining issue:

Action tag dropdown popping up after reopening a document and focus the caret on the text containing the tag rule /, after the slash. To reproduce:
Instead of having an empty document add this text:

_document = MutableDocument(nodes: [ ParagraphNode( id: Editor.createNodeId(), text: AttributedText("Some Infor/mation"), ), ]);

Focus caret at n, the dropdown pops up.

I have already filed a separate issue for this #2392

@matthew-carroll been waiting for this PR to merge since a long time, can you please review this?

matthew-carroll

LGTM

…2241, #2242) (#2387)

alterhuman · 2024-12-24T21:24:52Z

@angelosilvestre found a crash/error while using it:

How to Reproduce

Select a paragraph node completely (all text) and then continue the selection to another paragraph node above it. Or simply select multiple nodes partially.

Error

Exception has occurred.
RangeError (RangeError (end): Invalid value: Not in inclusive range 10..56: 68)

at line 498 in action_tags.dart:
final charactersAfter = rawText.substring(splitIndex, endOffset).characters;

of method _findTag:

/// Finds a tag that touches the given [expansionPosition], constaining it
/// to not cross the [endPosition].
///
/// If [endPosition] is not a `TextNodePosition`, it will be ignored .
TagAroundPosition? _findTag({
  required TagRule tagRule,
  required String nodeId,
  required AttributedText text,
  required TextNodePosition expansionPosition,
  required NodePosition endPosition,
  required bool Function(Set<Attribution> tokenAttributions) isTokenCandidate,
}) {
  final rawText = text.text;
  if (rawText.isEmpty) {
    return null;
  }

  int splitIndex = min(expansionPosition.offset, rawText.length);
  splitIndex = max(splitIndex, 0);

  final endOffset = endPosition is TextNodePosition ? endPosition.offset : null;

  // Create 2 splits of characters to navigate upstream and downstream the caret position.
  // ex: "this is a very|long string"
  // -> split around the caret into charactersBefore="this is a very" and charactersAfter="long string"
  final charactersBefore = rawText.substring(0, splitIndex).characters;
  final iteratorUpstream = charactersBefore.iteratorAtEnd;

  final charactersAfter = rawText.substring(splitIndex, endOffset).characters;
  final iteratorDownstream = charactersAfter.iterator;

  if (charactersBefore.isNotEmpty && tagRule.excludedCharacters.contains(charactersBefore.last)) {
    // The character where we're supposed to begin our expansion is a
    // character that's not allowed in a tag. Therefore, no tag exists
    // around the search offset.
    return null;
  }

  // Move upstream until we find the trigger character or an excluded character.
  while (iteratorUpstream.moveBack()) {
    final currentCharacter = iteratorUpstream.current;
    if (tagRule.excludedCharacters.contains(currentCharacter)) {
      // The upstream character isn't allowed to appear in a tag. end the search.
      return null;
    }

    if (currentCharacter == tagRule.trigger) {
      // The character we are reading is the trigger.
      // We move the iteratorUpstream one last time to include the trigger in the tokenRange and stop looking any further upstream
      iteratorUpstream.moveBack();
      break;
    }
  }

  // Move downstream the caret position until we find excluded character or reach the end of the text.
  while (iteratorDownstream.moveNext()) {
    final current = iteratorDownstream.current;
    if (tagRule.excludedCharacters.contains(current)) {
      break;
    }
  }

  final tokenStartOffset = splitIndex - iteratorUpstream.stringAfterLength;
  final tokenRange = SpanRange(tokenStartOffset, splitIndex + iteratorDownstream.stringBeforeLength);

  final tagText = text.substringInRange(tokenRange);
  if (!tagText.startsWith(tagRule.trigger)) {
    return null;
  }

  final tokenAttributions = text.getAttributionSpansInRange(attributionFilter: (a) => true, range: tokenRange);
  if (!isTokenCandidate(tokenAttributions.map((span) => span.attribution).toSet())) {
    return null;
  }

  return TagAroundPosition(
    indexedTag: IndexedTag(
      Tag(tagRule.trigger, tagText.substring(1)),
      nodeId,
      tokenStartOffset,
    ),
    searchOffset: expansionPosition.offset,
  );
}

The stack trace points to the _findTag usage at line 324 in action_tags.dart:

if (tagAroundPosition == null && extent.nodePosition is TextNodePosition) {
      textNode = document.getNodeById(selection.extent.nodeId) as TextNode;
      tagAroundPosition = _findTag(
        tagRule: _tagRule,
        nodeId: textNode.id,
        text: textNode.text,
        expansionPosition: extent.nodePosition as TextNodePosition,
        endPosition: normalizedSelection.end.nodePosition,
        isTokenCandidate: (attributions) => !attributions.contains(actionTagCancelledAttribution),
      );
    }

@matthew-carroll would suggest reverting the merge until this issue is fixed.

matthew-carroll · 2024-12-26T02:35:44Z

@angelosilvestre can you please file a follow up issue?

angelosilvestre · 2024-12-26T22:24:05Z

@matthew-carroll Filed #2479

[SuperEditor] Limit tag expansion to caret position (Resolves #2240)

eacfcf8

Fix tag being re-applied after being canceled

23d2527

angelosilvestre changed the title ~~[SuperEditor] Limit tag expansion to caret position (Resolves #2240)~~ [SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) Oct 31, 2024

angelosilvestre requested a review from matthew-carroll October 31, 2024 22:57

matthew-carroll reviewed Nov 5, 2024

View reviewed changes

angelosilvestre added 5 commits December 17, 2024 10:43

PR updates

aea32be

PR updates

f9e5819

Extract findTagAroundPosition

bacfd11

PR updates

07e0e21

PR updates

d9278a2

angelosilvestre requested a review from matthew-carroll December 18, 2024 18:07

matthew-carroll approved these changes Dec 23, 2024

View reviewed changes

matthew-carroll merged commit 583d978 into main Dec 23, 2024
12 of 14 checks passed

matthew-carroll deleted the 2240_actions-tag-crash branch December 23, 2024 20:17

github-actions bot pushed a commit that referenced this pull request Dec 23, 2024

[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #…

a5fd02d

…2241, #2242) (#2387)

github-actions bot mentioned this pull request Dec 23, 2024

Cherry Pick: [SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2478

Merged

matthew-carroll pushed a commit that referenced this pull request Dec 23, 2024

[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #…

1f08366

…2241, #2242) (#2387)

angelosilvestre mentioned this pull request Dec 26, 2024

[BUG] - Crash when selecting across paragraphs with action tag plugin #2479

Closed

alterhuman mentioned this pull request Jan 20, 2025

[SuperEditor] - Exception when removing action tags slash character within some text #2373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2387

[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2387

angelosilvestre commented Oct 29, 2024 •

edited

Loading

angelosilvestre commented Oct 29, 2024

alterhuman commented Oct 30, 2024

angelosilvestre commented Oct 30, 2024

alterhuman commented Oct 31, 2024 •

edited

Loading

angelosilvestre commented Oct 31, 2024

alterhuman commented Oct 31, 2024

matthew-carroll Nov 5, 2024

angelosilvestre Nov 13, 2024

matthew-carroll Nov 16, 2024

angelosilvestre Nov 18, 2024

matthew-carroll Nov 18, 2024

alterhuman Dec 17, 2024

angelosilvestre Dec 17, 2024

angelosilvestre Dec 17, 2024

alterhuman Dec 19, 2024

alterhuman Dec 23, 2024

matthew-carroll left a comment

alterhuman commented Dec 24, 2024

matthew-carroll commented Dec 26, 2024

angelosilvestre commented Dec 26, 2024

[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2387

[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2387

Conversation

angelosilvestre commented Oct 29, 2024 • edited Loading

angelosilvestre commented Oct 29, 2024

alterhuman commented Oct 30, 2024

angelosilvestre commented Oct 30, 2024

alterhuman commented Oct 31, 2024 • edited Loading

angelosilvestre commented Oct 31, 2024

alterhuman commented Oct 31, 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

matthew-carroll left a comment

Choose a reason for hiding this comment

alterhuman commented Dec 24, 2024

How to Reproduce

Error

matthew-carroll commented Dec 26, 2024

angelosilvestre commented Dec 26, 2024

angelosilvestre commented Oct 29, 2024 •

edited

Loading

alterhuman commented Oct 31, 2024 •

edited

Loading