-
-
Notifications
You must be signed in to change notification settings - Fork 80
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.
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
Ab fix out of bounds on set text #402
base: main
Are you sure you want to change the base?
Ab fix out of bounds on set text #402
Conversation
* defensive change for null check * Fix for indexOutOfBounds on setText() from custom historyState.
So after some investigations, the problem is this code in handleRemovingCharacters function: val minRemoveIndex =
(textFieldValue.selection.max - removedCharsCount)
.coerceAtLeast(0) The remove doesn't work correctly if the previous selection.max is not the end of the removed text range. Changing it to this should fix the issue: val minRemoveIndex =
tempTextFieldValue.selection.min
.coerceAtLeast(0) I will create a PR with the fix and with test coverage for this scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
There are some changes that need to be done before I can accept this PR, please check the comments.
@@ -1741,20 +1758,29 @@ public class RichTextState internal constructor( | |||
(!config.preserveStyleOnEmptyLine || richSpan.paragraph.isEmpty()) && | |||
isSelectionAtNewRichSpan | |||
) { | |||
newParagraphFirstRichSpan.spanStyle = SpanStyle() | |||
newParagraphFirstRichSpan.richSpanStyle = RichSpanStyle.Default | |||
if (newParagraphFirstRichSpan != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for this check, if isSelectionAtNewRichSpan
is true, it means that newParagraphFirstRichSpan
is not null.
} | ||
|
||
removeTextRange(textRange) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After fixing the issue with removing characters, it should work fine. I prefer to keep using removeTextRange + addTextAfterSelection to avoid code duplication. It's not causing any issues since removeTextRange will change the selection to textRange.min.
} | ||
|
||
// A helper function to safely slice strings without throwing exceptions | ||
private fun String.safeSubstring(startIndex: Int, endIndex: Int = this.length): String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper function is useful, you can keep it even if it's going to be unused.
@@ -304,20 +304,37 @@ public class RichTextState internal constructor( | |||
textRange: TextRange, | |||
text: String | |||
) { | |||
require(textRange.min >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really prefer to force the user to provide a correct textRange.
With this new check we will get some unexpected results, let's say you enter (-10, -5) range then you well get an error saying "The start index (0) must be less than or equal to the end index (-5)."
but the start index was -10 not 0.
Let's keep it the same.
This fixes a couple issues, replacing text range is independent from selection (cursor position) allowing for better programmatic replacement.
Add some defensive checks for nullable vars.
Finally, adding some safety around sub strings and slicing.