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

Add autofill support to ios text input plugin#17493

Merged
LongCatIsLooong merged 13 commits intoflutter:masterfrom
LongCatIsLooong:autofill-ios
Apr 16, 2020
Merged

Add autofill support to ios text input plugin#17493
LongCatIsLooong merged 13 commits intoflutter:masterfrom
LongCatIsLooong:autofill-ios

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Apr 3, 2020

Framework PR: flutter/flutter#52126

Moved FlutterTextInputView's interface to the header file in order to expose it in unit tests.

@auto-assign auto-assign bot requested a review from gaaclarke April 3, 2020 02:51
@LongCatIsLooong LongCatIsLooong mentioned this pull request Apr 3, 2020
Merged
13 tasks
@ganttastic ganttastic mentioned this pull request Apr 5, 2020
Closed
gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
}

- (void)updateEditingClient:(int)client withState:(NSDictionary*)state withTag:(NSString*)tag {
[_textInputChannel.get() invokeMethod:@"TextInputClient.updateEditingStateWithTag"
Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

This would be safer if you used performSelector:withObject:withObject: https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418667-performselector?language=objc

That will give you static analysis that the method you are invoking exists.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Apr 13, 2020

Choose a reason for hiding this comment

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

I think @"TextInputClient.updateEditingStateWithTag" does not represent an objective-c selector but a platform message (that will be received and handled by some dart code)?

Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

sorry, you're right.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h Outdated
#include "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputDelegate.h"

#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG
Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

FYI: This will be unnecessary after #17624 lands

Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

I was talking about the "FLUTTER_EXPORT" not being needed once that lands. We used to have to export things we wanted to tests, now tests live with the code so it isn't necessary.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Please date the PR with a more descriptive title.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm Outdated
}

- (void)setAutofillId:(NSString*)autofillId {
_autofillId = [autofillId copy];
Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

This doesn't release the old value. Instead of writing this you can just do@property(nonatomic, copy) NSString* autofillId to have one automatically generated. Put it in the .mm file to make it private.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Apr 13, 2020

Choose a reason for hiding this comment

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

Thank you for the review! I have to put the interface in the header file in order to be able to import that in a unit test (at least for now). Does adding autorelease after copy work here too?

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Apr 13, 2020

Choose a reason for hiding this comment

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

Oh your pull request is already merged. I guess that means I can move the interface back to the .mm file?

Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

Yep, autorelease can be used on any object.

gaaclarke
gaaclarke reviewed Apr 13, 2020
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm Outdated
} else {
_activeView = _view;
NSAssert(clientUniqueId != nil, @"The client's unique id can't be null");
for (FlutterTextInputView* v in _inputViews)
Copy link
Member

@gaaclarke gaaclarke Apr 13, 2020

Choose a reason for hiding this comment

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

for (int i = 0; i < 10; i++)
  BlowTheHorn();                // AVOID.

http://google.github.io/styleguide/objcguide.html#conditionals

gaaclarke
gaaclarke suggested changes Apr 13, 2020
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Sorry i didn't put all my comments all together in one review, i'm done now =)

@LongCatIsLooong LongCatIsLooong changed the title Autofill ios Add autofill support to ios text input plugin Apr 13, 2020
gaaclarke
gaaclarke approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

I just threw on a change with my outstanding comments into a patch. LGTM now.

@LongCatIsLooong LongCatIsLooong merged commit 104df6b into flutter:master Apr 16, 2020
@LongCatIsLooong LongCatIsLooong deleted the autofill-ios branch April 16, 2020 09:48
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 16, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 16, 2020
@engine-flutter-autoroll
104df6b Add autofill support to ios text input plugin (flutter/engine…
012217b 
…#17493)
@xster xster mentioned this pull request Apr 18, 2020
Merged
xster added a commit to xster/engine that referenced this pull request Apr 22, 2020
@xster
review and add more functional change to match flutter#17493
636285d
mikkokaar pushed a commit to mikkokaar/engine that referenced this pull request Apr 24, 2020
@xster @mikkokaar
review and add more functional change to match flutter#17493
ba28fee
xster added a commit to xster/engine that referenced this pull request Apr 24, 2020
@xster
review and add more functional change to match flutter#17493
279d239
@jmagman jmagman mentioned this pull request May 25, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gaaclarke gaaclarke gaaclarke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LongCatIsLooong @gaaclarke @googlebot