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

Ab fix out of bounds on set text #402

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+41 −15
Open

Ab fix out of bounds on set text #402

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba3f434
* #401 Fix for replaceTextRange
abarrafo Oct 22, 2024
7cd972b
Update with better safe handleing
abarrafo Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 41 additions & 15 deletions richeditor-compose/src/commonMain/kotlin/com/mohamedrejeb/richeditor/model/RichTextState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,20 +304,37 @@ public class RichTextState internal constructor(
textRange: TextRange,
text: String
) {
require(textRange.min >= 0) {
Copy link
Owner

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.

"The start index must be non-negative."
}
// Ensure that the start and end indices are within bounds
val start = textRange.min.coerceAtLeast(0) // Ensure the start is non-negative
val end = textRange.max.coerceAtMost(textFieldValue.text.length) // Ensure the end does not exceed text length

require(textRange.max <= textFieldValue.text.length) {
"The end index must be within the text bounds. " +
"The text length is ${textFieldValue.text.length}, " +
"but the end index is ${textRange.max}."
require(start <= end) {
"The start index ($start) must be less than or equal to the end index ($end)."
}

removeTextRange(textRange)
Copy link
Owner

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.

addTextAfterSelection(text = text)
// Perform the replacement with safe slicing
val beforeText = textFieldValue.text.safeSubstring(0, start)
val afterText = textFieldValue.text.safeSubstring(end)
val newText = beforeText + text + afterText

onTextFieldValueChange(
newTextFieldValue = textFieldValue.copy(
text = newText,
selection = TextRange(start + text.length), // Set cursor to the end of the inserted text
)
)
}

// A helper function to safely slice strings without throwing exceptions
private fun String.safeSubstring(startIndex: Int, endIndex: Int = this.length): String {
Copy link
Owner

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.

return if (startIndex in 0..this.length && endIndex in startIndex..this.length) {
this.substring(startIndex, endIndex)
} else {
"" // Return empty string if indices are out of bounds
}
}


/**
* Adds the provided text to the text field at the current selection.
*
Expand Down Expand Up @@ -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) {
Copy link
Owner

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.

newParagraphFirstRichSpan.spanStyle = SpanStyle()
}
if (newParagraphFirstRichSpan != null) {
newParagraphFirstRichSpan.richSpanStyle = RichSpanStyle.Default
}
} else if (
config.preserveStyleOnEmptyLine &&
isSelectionAtNewRichSpan
) {
newParagraphFirstRichSpan.spanStyle = currentSpanStyle
newParagraphFirstRichSpan.richSpanStyle = currentRichSpanStyle
if (newParagraphFirstRichSpan != null) {
newParagraphFirstRichSpan.spanStyle = currentSpanStyle
}
if (newParagraphFirstRichSpan != null) {
newParagraphFirstRichSpan.richSpanStyle = currentRichSpanStyle
}
}
}

// Get the text before and after the slice index
val beforeText = tempTextFieldValue.text.substring(0, sliceIndex + 1)
val afterText = tempTextFieldValue.text.substring(sliceIndex + 1)
val safeSliceIndex = sliceIndex.coerceAtMost(tempTextFieldValue.text.length - 1)
val beforeText = tempTextFieldValue.text.substring(0, safeSliceIndex + 1)
val afterText = tempTextFieldValue.text.substring(safeSliceIndex + 1)

// Update the text field value to include the new paragraph custom start text
tempTextFieldValue = tempTextFieldValue.copy(
Expand Down
Loading