Fix crash when cursor ends up at invalid position #8747
Conversation
|
/cc @dansunhappy @songfei |
|
To be clear: we still need to fix the cursor positioning logic in Dart side, but this makes it slightly less bad. |
|
|
||
| // Helper to get the correct boundary of a character position in an NSString | ||
| // given a byte index. | ||
| NSRange rangeForCharacterAtIndex(NSString* text, NSUInteger index); |
There was a problem hiding this comment.
Here and elsewhere, please use CapitalCase.
|
|
||
| namespace fml { | ||
|
|
||
| // Helper to get the correct boundary of a character position in an NSString |
There was a problem hiding this comment.
Nit: here and below |NSString| etc. for identifiers.
| namespace fml { | ||
|
|
||
| // Helper to get the correct boundary of a character position in an NSString | ||
| // given a byte index. |
There was a problem hiding this comment.
Consider rephrasing as Returns the range of the grapheme cluster in which |index| is located.
Generally, starting with a third-person verb tends to encourage comments that focus on what the code does. I'd also avoid the use of the word 'character' as it's pretty ambiguous. Similar below.
I'd also document behaviour in cases where index is invalid.
There was a problem hiding this comment.
The underlying impelementation throws an exception (NSInvalidArgumentException). Is that fair for us to just do?
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
LGTM modulo nits + a test for naughty indices.
| [view setSelectedTextRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(1, 0)]]; | ||
| FlutterTextRange* selectedRange = (FlutterTextRange*)[view selectedTextRange]; | ||
| EXPECT_EQ(selectedRange.range.location, 2); | ||
| } |
| namespace fml { | ||
|
|
||
| NSRange RangeForCharacterAtIndex(NSString* text, NSUInteger index) { | ||
| if (text == nil || index > text.length) { |
There was a problem hiding this comment.
I'd be tempted to use index >= text.length for consistency with Apple's APIs. It also feels a bit more intuitive: the underlying string has no character at index == length.
That said, text positions do allow for end-of-string positions. If every call to this is expected to have to defend against end-of-string positions, then it probably makes sense as-is, but I'd definitely document the range of valid indices in the header.
There was a problem hiding this comment.
I'll go with >=, good catch.
flutter/engine@e0bda06...7f753f8 git log e0bda06..7f753f8 --no-merges --oneline 7f753f8 Roll src/third_party/dart 6c3a91f281..c46deebfb6 (3 commits) 57bf1c0 Fix crash when cursor ends up at invalid position (flutter/engine#8747) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
4 participants
Factored out a couple sanitization methods into FML so that Desktop can use them and we can unit test them more easily.
See flutter/flutter#31468 and the linked issues from that for an attempt to fix this on the framework side.
Root cause:
This fixes the input handling on iOS so we don't crash, and makes flutter/flutter#24802 not be a crasher.
Fixes flutter/flutter#31525