Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

fix voice control delete line command does not delete line correctly #24831

Merged
merged 4 commits into from
Mar 10, 2021
Merged

fix voice control delete line command does not delete line correctly #24831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53a0527
fix voice control delete line command does not delete line correctly
chunhtai Mar 5, 2021
6cbb4d3
fix typo
chunhtai Mar 6, 2021
2feb124
addressing comment
chunhtai Mar 9, 2021
2f8b5fe
addressing comment
chunhtai Mar 9, 2021
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
4 changes: 4 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@

@end

/** A tokenizer used by `FlutterTextInputView` to customize string parsing. */
@interface FlutterTokenizer : UITextInputStringTokenizer
@end

#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG
FLUTTER_DARWIN_EXPORT
#endif
Expand Down
69 changes: 68 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,73 @@ - (BOOL)isEqualTo:(FlutterTextRange*)other {
}
@end

#pragma mark - FlutterTokenizer

@interface FlutterTokenizer ()

@property(nonatomic, assign) FlutterTextInputView* textInputView;
Copy link
Member

@gaaclarke gaaclarke Mar 9, 2021

Choose a reason for hiding this comment

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

s/assign/weak for more modern objc and forward compatibility with ARC

Copy link
Contributor Author

@chunhtai chunhtai Mar 9, 2021

Choose a reason for hiding this comment

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

I tried to use weak, but it complains this file is MRC and can't use weak


@end

@implementation FlutterTokenizer

- (instancetype)initWithTextInput:(UIResponder<UITextInput>*)textInput {
NSAssert([textInput isKindOfClass:[FlutterTextInputView class]],
@"The FlutterTokenizer can only be used in a FlutterTextInputView");
self = [super initWithTextInput:textInput];
if (self) {
_textInputView = (FlutterTextInputView*)textInput;
Copy link
Member

@gaaclarke gaaclarke Mar 9, 2021

Choose a reason for hiding this comment

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

Why cast here? Why not just pass in a FlutterTextInputView? If you want to keep the cast you should have an assert that it a valid cast (isKindOf:).

Copy link
Contributor Author

@chunhtai chunhtai Mar 9, 2021

Choose a reason for hiding this comment

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

yes i have assert it in the beginning, line 392.

This is an override of https://developer.apple.com/documentation/uikit/uitextinputstringtokenizer/1614469-initwithtextinput?language=objc

Copy link
Member

@gaaclarke gaaclarke Mar 9, 2021

Choose a reason for hiding this comment

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

I thought i saw it somewhere, thanks.

}
return self;
}

- (UITextRange*)rangeEnclosingPosition:(UITextPosition*)position
Copy link
Member

@gaaclarke gaaclarke Mar 9, 2021

Choose a reason for hiding this comment

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

If you just pass in the FlutterTextInputView as an argument to this method you wouldn't have to keep an instance variable around. The instance variable is a bit problematic because if the class is used differently it can cause a crash.

Copy link
Contributor Author

@chunhtai chunhtai Mar 9, 2021

Choose a reason for hiding this comment

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

this is an override. https://developer.apple.com/documentation/uikit/uitextinputtokenizer/1614464-rangeenclosingposition?language=objc

I can't change the function signature

Copy link
Contributor Author

@chunhtai chunhtai Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I figured it is quite hard to know whether this is an override or private method for a reviewer. is there a best practice way to annotate an override method?

Copy link
Member

@gaaclarke gaaclarke Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You almost never use inheritance in objc. I didn't see this was inheriting UITextInputStringTokenizer. Is there a reason it has to inherit from UITextInputStringTokenizer?

The documentation says this for subclassing UITextInputString:

When you subclass UITextInputStringTokenizer, override all UITextInputTokenizer methods, calling the superclass implementation (super) when method parameters are not affected by layout. For example, the subclass needs a custom implementation of all methods for line granularity. For the left direction, it needs to decide whether left corresponds at a given position to forward or backward, and then call super passing in the storage direction (UITextStorageDirection).

Sounds like it might be a problem if you aren't calling super.

edit: You should probably be implementing UITextInputTokenizer then delegating to an instance of UITextInputStringTokenizer when you need to.

Copy link
Contributor Author

@chunhtai chunhtai Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

calling the superclass implementation (super) when method parameters are not affected by layout.

it does not seems to be a hard requirement? a little background on this change.

If you have text = "how are you"
and you call

[UITextInputStringTokenizer  rangeEnclosingPosition:<at the end> withGranularity:line inDirection:forward]

it returns NSRange(location =8, length =3) which corresponds to "you". It is same result of Granularity word

The result is completely unusable. That is why i have to complete rewrite the line logic.

Copy link
Member

@gaaclarke gaaclarke Mar 9, 2021

Choose a reason for hiding this comment

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

Yea, maybe you are right. I have a hard time understanding if that warning in the documentation is actually applicable. We do have tests I guess.

withGranularity:(UITextGranularity)granularity
inDirection:(UITextDirection)direction {
UITextRange* result;
switch (granularity) {
case UITextGranularityLine:
// The default UITextInputStringTokenizer does not handle line granularity
// correctly. We need to implement our own line tokenizer.
result = [self lineEnclosingPosition:position];
break;
case UITextGranularityCharacter:
case UITextGranularityWord:
case UITextGranularitySentence:
case UITextGranularityParagraph:
case UITextGranularityDocument:
// The UITextInputStringTokenizer can handle all these cases correctly.
result = [super rangeEnclosingPosition:position
withGranularity:granularity
inDirection:direction];
break;
}
return result;
}

- (UITextRange*)lineEnclosingPosition:(UITextPosition*)position {
// Gets the first line break position after the input position.
NSString* textAfter = [_textInputView
textInRange:[_textInputView textRangeFromPosition:position
toPosition:[_textInputView endOfDocument]]];
NSArray<NSString*>* linesAfter = [textAfter componentsSeparatedByString:@"\n"];
NSInteger offSetToLineBreak = [linesAfter firstObject].length;
UITextPosition* lineBreakAfter = [_textInputView positionFromPosition:position
offset:offSetToLineBreak];
// Gets the first line break position before the input position.
NSString* textBefore = [_textInputView
textInRange:[_textInputView textRangeFromPosition:[_textInputView beginningOfDocument]
toPosition:position]];
NSArray<NSString*>* linesBefore = [textBefore componentsSeparatedByString:@"\n"];
NSInteger offSetFromLineBreak = [linesBefore lastObject].length;
UITextPosition* lineBreakBefore = [_textInputView positionFromPosition:position
offset:-offSetFromLineBreak];

return [_textInputView textRangeFromPosition:lineBreakBefore toPosition:lineBreakAfter];
}

@end

// A FlutterTextInputView that masquerades as a UITextField, and forwards
// selectors it can't respond to to a shared UITextField instance.
//
Expand Down Expand Up @@ -629,7 +696,7 @@ - (BOOL)canBecomeFirstResponder {

- (id<UITextInputTokenizer>)tokenizer {
if (_tokenizer == nil) {
_tokenizer = [[UITextInputStringTokenizer alloc] initWithTextInput:self];
_tokenizer = [[FlutterTokenizer alloc] initWithTextInput:self];
}
return _tokenizer;
}
Expand Down
47 changes: 47 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,16 @@ - (NSMutableDictionary*)mutableTemplateCopy {
[FlutterTextInputView class]]];
}

- (FlutterTextRange*)getLineRangeFromTokenizer:(id<UITextInputTokenizer>)tokenizer
atIndex:(NSInteger)index {
UITextRange* range =
[tokenizer rangeEnclosingPosition:[FlutterTextPosition positionWithIndex:index]
withGranularity:UITextGranularityLine
inDirection:UITextLayoutDirectionRight];
XCTAssertTrue([range isKindOfClass:[FlutterTextRange class]]);
return (FlutterTextRange*)range;
}

#pragma mark - Tests

- (void)testSecureInput {
Expand Down Expand Up @@ -818,4 +828,41 @@ - (void)testFlutterTextInputViewDirectFocusToBackingTextInput {
XCTAssertEqual(inputView.receivedNotificationTarget, backing);
}

- (void)testFlutterTokenizerCanParseLines {
FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init];
inputView.textInputDelegate = engine;
id<UITextInputTokenizer> tokenizer = [inputView tokenizer];

// The tokenizer returns zero range When text is empty.
FlutterTextRange* range = [self getLineRangeFromTokenizer:tokenizer atIndex:0];
XCTAssertEqual(range.range.location, 0u);
XCTAssertEqual(range.range.length, 0u);

[inputView insertText:@"how are you\nI am fine, Thank you"];

range = [self getLineRangeFromTokenizer:tokenizer atIndex:0];
XCTAssertEqual(range.range.location, 0u);
XCTAssertEqual(range.range.length, 11u);

range = [self getLineRangeFromTokenizer:tokenizer atIndex:2];
XCTAssertEqual(range.range.location, 0u);
XCTAssertEqual(range.range.length, 11u);

range = [self getLineRangeFromTokenizer:tokenizer atIndex:11];
XCTAssertEqual(range.range.location, 0u);
XCTAssertEqual(range.range.length, 11u);

range = [self getLineRangeFromTokenizer:tokenizer atIndex:12];
XCTAssertEqual(range.range.location, 12u);
XCTAssertEqual(range.range.length, 20u);

range = [self getLineRangeFromTokenizer:tokenizer atIndex:15];
XCTAssertEqual(range.range.location, 12u);
XCTAssertEqual(range.range.length, 20u);

range = [self getLineRangeFromTokenizer:tokenizer atIndex:32];
XCTAssertEqual(range.range.location, 12u);
XCTAssertEqual(range.range.length, 20u);
}

@end