This repository was archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
[iOS] text input methods to only call updateEditState once #19161
Merged
LongCatIsLooong
merged 4 commits into
flutter-team-archive:master
from
LongCatIsLooong:no-implicit-updateEditingState
Jun 22, 2020
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
92b50c2
remove redundant framework sync from iOS text input plugin
LongCatIsLooong 535959d
add tests
LongCatIsLooong ac89f3e
renaming private methods
LongCatIsLooong ce8339f
Merge remote-tracking branch 'upstream/master' into no-implicit-updat…
LongCatIsLooong File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,7 +325,8 @@ - (void)setTextInputClient:(int)client { | |
| _textInputClient = client; | ||
| } | ||
|
|
||
| - (void)setTextInputState:(NSDictionary*)state { | ||
| // Return true if the new input state needs to be synced back to the framework. | ||
| - (BOOL)setTextInputState:(NSDictionary*)state { | ||
| NSString* newText = state[@"text"]; | ||
| BOOL textChanged = ![self.text isEqualToString:newText]; | ||
| if (textChanged) { | ||
|
|
@@ -356,8 +357,7 @@ - (void)setTextInputState:(NSDictionary*)state { | |
| selectedRange.length != oldSelectedRange.length) { | ||
| needsEditingStateUpdate = YES; | ||
| [self.inputDelegate selectionWillChange:self]; | ||
| [self setSelectedTextRange:[FlutterTextRange rangeWithNSRange:selectedRange] | ||
| updateEditingState:NO]; | ||
| [self _setSelectedTextRange:[FlutterTextRange rangeWithNSRange:selectedRange]]; | ||
| _selectionAffinity = _kTextAffinityDownstream; | ||
| if ([state[@"selectionAffinity"] isEqualToString:@(_kTextAffinityUpstream)]) | ||
| _selectionAffinity = _kTextAffinityUpstream; | ||
|
|
@@ -367,10 +367,9 @@ - (void)setTextInputState:(NSDictionary*)state { | |
| if (textChanged) { | ||
| [self.inputDelegate textDidChange:self]; | ||
| } | ||
| if (needsEditingStateUpdate) { | ||
| // For consistency with Android behavior, send an update to the framework. | ||
| [self updateEditingState]; | ||
| } | ||
|
|
||
| // For consistency with Android behavior, send an update to the framework if anything changed. | ||
| return needsEditingStateUpdate; | ||
| } | ||
|
|
||
| - (NSRange)clampSelection:(NSRange)range forText:(NSString*)text { | ||
|
|
@@ -401,11 +400,7 @@ - (UITextRange*)selectedTextRange { | |
| return [[_selectedTextRange copy] autorelease]; | ||
| } | ||
|
|
||
| - (void)setSelectedTextRange:(UITextRange*)selectedTextRange { | ||
| [self setSelectedTextRange:selectedTextRange updateEditingState:YES]; | ||
| } | ||
|
|
||
| - (void)setSelectedTextRange:(UITextRange*)selectedTextRange updateEditingState:(BOOL)update { | ||
| - (void)_setSelectedTextRange:(UITextRange*)selectedTextRange { | ||
| if (_selectedTextRange != selectedTextRange) { | ||
| UITextRange* oldSelectedRange = _selectedTextRange; | ||
| if (self.hasText) { | ||
|
|
@@ -416,12 +411,14 @@ - (void)setSelectedTextRange:(UITextRange*)selectedTextRange updateEditingState: | |
| _selectedTextRange = [selectedTextRange copy]; | ||
| } | ||
| [oldSelectedRange release]; | ||
|
|
||
| if (update) | ||
| [self updateEditingState]; | ||
| } | ||
| } | ||
|
|
||
| - (void)setSelectedTextRange:(UITextRange*)selectedTextRange { | ||
| [self _setSelectedTextRange:selectedTextRange]; | ||
| [self updateEditingState]; | ||
| } | ||
|
|
||
| - (id)insertDictationResultPlaceholder { | ||
| return @""; | ||
| } | ||
|
|
@@ -440,26 +437,29 @@ - (NSString*)textInRange:(UITextRange*)range { | |
| return [self.text substringWithRange:textRange]; | ||
| } | ||
|
|
||
| - (void)replaceRange:(UITextRange*)range withText:(NSString*)text { | ||
| NSRange replaceRange = ((FlutterTextRange*)range).range; | ||
| - (void)_replaceRange:(NSRange)range withText:(NSString*)text { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here with |
||
| NSRange selectedRange = _selectedTextRange.range; | ||
|
|
||
| // Adjust the text selection: | ||
| // * reduce the length by the intersection length | ||
| // * adjust the location by newLength - oldLength + intersectionLength | ||
| NSRange intersectionRange = NSIntersectionRange(replaceRange, selectedRange); | ||
| if (replaceRange.location <= selectedRange.location) | ||
| selectedRange.location += text.length - replaceRange.length; | ||
| NSRange intersectionRange = NSIntersectionRange(range, selectedRange); | ||
| if (range.location <= selectedRange.location) | ||
| selectedRange.location += text.length - range.length; | ||
| if (intersectionRange.location != NSNotFound) { | ||
| selectedRange.location += intersectionRange.length; | ||
| selectedRange.length -= intersectionRange.length; | ||
| } | ||
|
|
||
| [self.text replaceCharactersInRange:[self clampSelection:replaceRange forText:self.text] | ||
| [self.text replaceCharactersInRange:[self clampSelection:range forText:self.text] | ||
| withString:text]; | ||
| [self setSelectedTextRange:[FlutterTextRange rangeWithNSRange:[self clampSelection:selectedRange | ||
| forText:self.text]] | ||
| updateEditingState:NO]; | ||
| [self _setSelectedTextRange:[FlutterTextRange rangeWithNSRange:[self clampSelection:selectedRange | ||
| forText:self.text]]]; | ||
| } | ||
|
|
||
| - (void)replaceRange:(UITextRange*)range withText:(NSString*)text { | ||
| NSRange replaceRange = ((FlutterTextRange*)range).range; | ||
| [self _replaceRange:replaceRange withText:text]; | ||
| [self updateEditingState]; | ||
| } | ||
|
|
||
|
|
@@ -522,11 +522,11 @@ - (void)setMarkedText:(NSString*)markedText selectedRange:(NSRange)markedSelecte | |
|
|
||
| if (markedTextRange.length > 0) { | ||
| // Replace text in the marked range with the new text. | ||
| [self replaceRange:self.markedTextRange withText:markedText]; | ||
| [self _replaceRange:markedTextRange withText:markedText]; | ||
| markedTextRange.length = markedText.length; | ||
| } else { | ||
| // Replace text in the selected range with the new text. | ||
| [self replaceRange:_selectedTextRange withText:markedText]; | ||
| [self _replaceRange:selectedRange withText:markedText]; | ||
| markedTextRange = NSMakeRange(selectedRange.location, markedText.length); | ||
| } | ||
|
|
||
|
|
@@ -535,9 +535,9 @@ - (void)setMarkedText:(NSString*)markedText selectedRange:(NSRange)markedSelecte | |
|
|
||
| NSUInteger selectionLocation = markedSelectedRange.location + markedTextRange.location; | ||
| selectedRange = NSMakeRange(selectionLocation, markedSelectedRange.length); | ||
| [self setSelectedTextRange:[FlutterTextRange rangeWithNSRange:[self clampSelection:selectedRange | ||
| forText:self.text]] | ||
| updateEditingState:YES]; | ||
| [self _setSelectedTextRange:[FlutterTextRange rangeWithNSRange:[self clampSelection:selectedRange | ||
| forText:self.text]]]; | ||
| [self updateEditingState]; | ||
| } | ||
|
|
||
| - (void)unmarkText { | ||
|
|
@@ -1002,7 +1002,9 @@ + (void)setupInputView:(FlutterTextInputView*)inputView | |
| } | ||
|
|
||
| - (void)setTextInputEditingState:(NSDictionary*)state { | ||
| [_activeView setTextInputState:state]; | ||
| if ([_activeView setTextInputState:state]) { | ||
| [_activeView updateEditingState]; | ||
| } | ||
| } | ||
|
|
||
| - (void)clearTextInputClient { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,13 +17,27 @@ @interface FlutterTextInputView () | |
| - (void)setTextInputState:(NSDictionary*)state; | ||
| @end | ||
|
|
||
| @implementation FlutterTextInputPluginTest | ||
| - (void)testSecureInput { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
| FlutterTextInputPlugin* textInputPlugin = [[FlutterTextInputPlugin alloc] init]; | ||
| @implementation FlutterTextInputPluginTest { | ||
| id engine; | ||
| FlutterTextInputPlugin* textInputPlugin; | ||
| } | ||
|
|
||
| - (void)setUp { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice cleanup 👍 |
||
| [super setUp]; | ||
|
|
||
| engine = OCMClassMock([FlutterEngine class]); | ||
| textInputPlugin = [[FlutterTextInputPlugin alloc] init]; | ||
| textInputPlugin.textInputDelegate = engine; | ||
| } | ||
|
|
||
| - (void)tearDown { | ||
| [engine stopMocking]; | ||
| [[[[textInputPlugin textInputView] superview] subviews] | ||
| makeObjectsPerformSelector:@selector(removeFromSuperview)]; | ||
| [super tearDown]; | ||
| } | ||
|
|
||
| - (void)testSecureInput { | ||
| NSDictionary* config = @{ | ||
| @"inputType" : @{@"name" : @"TextInuptType.text"}, | ||
| @"keyboardAppearance" : @"Brightness.light", | ||
|
|
@@ -61,17 +75,9 @@ - (void)testSecureInput { | |
| // The one FlutterTextInputView we inserted into the view hierarchy should be the text input | ||
| // plugin's active text input view. | ||
| XCTAssertEqual(inputView, textInputPlugin.textInputView); | ||
|
|
||
| // Clean up. | ||
| [engine stopMocking]; | ||
| [[[[textInputPlugin textInputView] superview] subviews] | ||
| makeObjectsPerformSelector:@selector(removeFromSuperview)]; | ||
| } | ||
|
|
||
| - (void)testTextChangesTriggerUpdateEditingClient { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; | ||
| inputView.textInputDelegate = engine; | ||
|
|
||
|
|
@@ -86,15 +92,9 @@ - (void)testTextChangesTriggerUpdateEditingClient { | |
| // Don't send anything if there's nothing new. | ||
| [inputView setTextInputState:@{@"text" : @"AFTER"}]; | ||
| OCMReject([engine updateEditingClient:0 withState:[OCMArg any]]); | ||
|
|
||
| // Clean up. | ||
| [engine stopMocking]; | ||
| } | ||
|
|
||
| - (void)testSelectionChangeTriggersUpdateEditingClient { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; | ||
| inputView.textInputDelegate = engine; | ||
|
|
||
|
|
@@ -118,15 +118,9 @@ - (void)testSelectionChangeTriggersUpdateEditingClient { | |
| [inputView | ||
| setTextInputState:@{@"text" : @"SELECTION", @"selectionBase" : @1, @"selectionExtent" : @2}]; | ||
| OCMReject([engine updateEditingClient:0 withState:[OCMArg any]]); | ||
|
|
||
| // Clean up. | ||
| [engine stopMocking]; | ||
| } | ||
|
|
||
| - (void)testComposingChangeTriggersUpdateEditingClient { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; | ||
| inputView.textInputDelegate = engine; | ||
|
|
||
|
|
@@ -151,17 +145,9 @@ - (void)testComposingChangeTriggersUpdateEditingClient { | |
| [inputView | ||
| setTextInputState:@{@"text" : @"COMPOSING", @"composingBase" : @1, @"composingExtent" : @2}]; | ||
| OCMReject([engine updateEditingClient:0 withState:[OCMArg any]]); | ||
|
|
||
| // Clean up. | ||
| [engine stopMocking]; | ||
| } | ||
|
|
||
| - (void)testAutofillInputViews { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
| FlutterTextInputPlugin* textInputPlugin = [[FlutterTextInputPlugin alloc] init]; | ||
| textInputPlugin.textInputDelegate = engine; | ||
|
|
||
| NSDictionary* template = @{ | ||
| @"inputType" : @{@"name" : @"TextInuptType.text"}, | ||
| @"keyboardAppearance" : @"Brightness.light", | ||
|
|
@@ -214,26 +200,15 @@ - (void)testAutofillInputViews { | |
|
|
||
| // Verify behavior. | ||
| OCMVerify([engine updateEditingClient:0 withState:[OCMArg isNotNil] withTag:@"field2"]); | ||
|
|
||
| // Clean up. | ||
| [engine stopMocking]; | ||
| [[[[textInputPlugin textInputView] superview] subviews] | ||
| makeObjectsPerformSelector:@selector(removeFromSuperview)]; | ||
| } | ||
|
|
||
| - (void)testAutocorrectionPromptRectAppears { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| FlutterTextInputView* inputView = [[FlutterTextInputView alloc] initWithFrame:CGRectZero]; | ||
| inputView.textInputDelegate = engine; | ||
| [inputView firstRectForRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([engine showAutocorrectionPromptRectForStart:0 end:1 withClient:0]); | ||
|
|
||
| // Clean up mocks | ||
| [engine stopMocking]; | ||
| } | ||
|
|
||
| - (void)testTextRangeFromPositionMatchesUITextViewBehavior { | ||
|
|
@@ -248,4 +223,35 @@ - (void)testTextRangeFromPositionMatchesUITextViewBehavior { | |
| XCTAssertEqual(range.location, 0); | ||
| XCTAssertEqual(range.length, 2); | ||
| } | ||
|
|
||
| - (void)testUITextInputCallsUpdateEditingStateOnce { | ||
| FlutterTextInputView* inputView = [[FlutterTextInputView alloc] init]; | ||
| inputView.textInputDelegate = engine; | ||
|
|
||
| __block int updateCount = 0; | ||
| OCMStub([engine updateEditingClient:0 withState:[OCMArg isNotNil]]) | ||
| .andDo(^(NSInvocation* invocation) { | ||
| updateCount++; | ||
| }); | ||
|
|
||
| [inputView insertText:@"text to insert"]; | ||
| // Update the framework exactly once. | ||
| XCTAssertEqual(updateCount, 1); | ||
|
|
||
| [inputView deleteBackward]; | ||
| XCTAssertEqual(updateCount, 2); | ||
|
|
||
| inputView.selectedTextRange = [FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)]; | ||
| XCTAssertEqual(updateCount, 3); | ||
|
|
||
| [inputView replaceRange:[FlutterTextRange rangeWithNSRange:NSMakeRange(0, 1)] | ||
| withText:@"replace text"]; | ||
| XCTAssertEqual(updateCount, 4); | ||
|
|
||
| [inputView setMarkedText:@"marked text" selectedRange:NSMakeRange(0, 1)]; | ||
| XCTAssertEqual(updateCount, 5); | ||
|
|
||
| [inputView unmarkText]; | ||
| XCTAssertEqual(updateCount, 6); | ||
| } | ||
| @end | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In objc we don't prefix method names with
_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.
Changed to
setSelectedTextRangeLocal