Skip to content

fix: clear mistakes that fall out of new text range#97

Open
andrew-bekhiet-solid wants to merge 4 commits into
solid-software:mainfrom
andrew-bekhiet-solid:fix-filtering-mistakes-on-shorter-text
Open

fix: clear mistakes that fall out of new text range#97
andrew-bekhiet-solid wants to merge 4 commits into
solid-software:mainfrom
andrew-bekhiet-solid:fix-filtering-mistakes-on-shorter-text

Conversation

@andrew-bekhiet-solid
Copy link
Copy Markdown
Contributor

fix: prevent notifying listeners after dispose

Fixes a bug where changing text programmatically throws a RangeError
For example changing text from "This text has a mistke" to "abc" throws RangeError because the offset of "mistke" is out of range on "abc"

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a _isDisposed flag to prevent operations on a disposed LanguageToolController and refactors _filterMistakesOnChanged to return a list while filtering out-of-bounds mistakes. The review feedback points out a potential bug where an invalid text selection causes incorrect mistake offset adjustments, and suggests removing a redundant .toList() call on the refactored method's result.

Comment on lines +311 to 336
List<Mistake> _filterMistakesOnChanged(
List<Mistake> mistakes,
String newText,
) {
final isSelectionRangeEmpty = selection.end == selection.start;
final lengthDiscrepancy = newText.length - text.length;

for (final mistake in _mistakes) {
Mistake? newMistake;

newMistake = isSelectionRangeEmpty
? _adjustMistakeOffsetWithCaretCursor(
mistake: mistake,
lengthDiscrepancy: lengthDiscrepancy,
)
: _adjustMistakeOffsetWithSelectionRange(
mistake: mistake,
lengthDiscrepancy: lengthDiscrepancy,
);

if (newMistake != null) yield newMistake;
}
return mistakes
.map(
(mistake) => isSelectionRangeEmpty
? _adjustMistakeOffsetWithCaretCursor(
mistake: mistake,
lengthDiscrepancy: lengthDiscrepancy,
)
: _adjustMistakeOffsetWithSelectionRange(
mistake: mistake,
lengthDiscrepancy: lengthDiscrepancy,
),
)
.nonNulls
.where(
(mistake) =>
mistake.offset >= 0 && mistake.endOffset <= newText.length,
)
.toList();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When the text is changed programmatically or before the text field is focused, selection can be invalid (i.e., selection.isValid is false, with offsets of -1).

If selection is invalid, isSelectionRangeEmpty is true, and _adjustMistakeOffsetWithCaretCursor is called with caretLocation = -1. Since -1 <= mistake.offset is always true, shouldAdjustOffset evaluates to true, causing all mistakes to be incorrectly shifted by lengthDiscrepancy (for example, when appending text).

To prevent this incorrect shifting, we should check if selection.isValid is false and return the mistakes without adjustment, only filtering out those that exceed the new text range.

  List<Mistake> _filterMistakesOnChanged(
    List<Mistake> mistakes,
    String newText,
  ) {
    if (!selection.isValid) {
      return mistakes
          .where((mistake) => mistake.endOffset <= newText.length)
          .toList();
    }

    final isSelectionRangeEmpty = selection.end == selection.start;
    final lengthDiscrepancy = newText.length - text.length;

    return mistakes
        .map(
          (mistake) => isSelectionRangeEmpty
              ? _adjustMistakeOffsetWithCaretCursor(
                  mistake: mistake,
                  lengthDiscrepancy: lengthDiscrepancy,
                )
              : _adjustMistakeOffsetWithSelectionRange(
                  mistake: mistake,
                  lengthDiscrepancy: lengthDiscrepancy,
                ),
        )
        .nonNulls
        .where(
          (mistake) =>
              mistake.offset >= 0 && mistake.endOffset <= newText.length,
        )
        .toList();
  }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it will be handled by the .where filter anyway?

Comment thread lib/src/core/controllers/language_tool_controller.dart Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant