Android Autofill by LongCatIsLooong · Pull Request #17465 · flutter/engine

LongCatIsLooong · 2020-04-02T01:04:00Z

LongCatIsLooong · 2020-04-10T19:11:34Z

Hi @blasten could you take a look at this? I'm trying to add androidx.autofill to embedding_bundle, but not sure if I missed anything.

blasten · 2020-04-13T19:22:32Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

+      private static String translateAutofillHint(@NonNull String hint) {
+        switch (hint) {
+          case "addressCity":
+            return HintConstants.AUTOFILL_HINT_POSTAL_ADDRESS_LOCALITY;


We are very sensitive to adding dependencies to the embedding. This can cause conflict in the future if a plugin adds this dependency.
Since you are returning a String, is there any chance you can remove the dependency and return the raw string?

justinmc

LGTM. Take a look at my comment about fields, otherwise just nitpicks. 👍

justinmc · 2020-04-13T22:00:42Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

    channel.invokeMethod("TextInputClient.requestExistingInputState", null);
  }

+  private HashMap<Object, Object> createEditingStateJSON(


Could this method be static?

justinmc · 2020-04-13T22:06:29Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

+      Configuration[] fields = null;
+      if (!json.isNull("fields")) {
+        final JSONArray fields = json.getJSONArray("fields");
+        fields = new Configuration[fields.length()];


There are two declarations of fields above, maybe some code got mixed up or missed being deleted.

blasten · 2020-04-14T03:42:59Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

+                final double height = arguments.getDouble("height");
+                final JSONArray jsonMatrix = arguments.getJSONArray("transform");
+                final double[] matrix = new double[16];
+                for (int i = 0; i < 16; i++) matrix[i] = jsonMatrix.getDouble(i);


Suggested change

for (int i = 0; i < 16; i++) matrix[i] = jsonMatrix.getDouble(i);

for (int i = 0; i < 16; i++) {

matrix[i] = jsonMatrix.getDouble(i);

}

blasten · 2020-04-14T03:46:30Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

+
+      @NonNull
+      private static String translateAutofillHint(@NonNull String hint) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {


nit: it might be a bit more readable if you do:

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) { return hint; } // then switch

blasten · 2020-04-14T03:49:40Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O)
+      afm = view.getContext().getSystemService(AutofillManager.class);
+    else afm = null;


Suggested change

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O)

afm = view.getContext().getSystemService(AutofillManager.class);

else afm = null;

afm = Build.VERSION.SDK_INT >= Build.VERSION_CODES.O ? view.getContext().getSystemService(AutofillManager.class) : null;

SDK version check doesn't seem to work with ternary expressions, the linter started complaining when I applied the suggested change.

blasten · 2020-04-14T03:50:31Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

  }

+  private void notifyViewEntered() {
+    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return;


Suggested change

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return;

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {

return;

}

blasten · 2020-04-14T03:50:48Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+  }
+
+  private void notifyViewExited() {
+    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return;


Suggested change

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return;

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {

return;

}

blasten · 2020-04-14T03:51:04Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+  }
+
+  private void notifyValueChanged(String newValue) {
+    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return;


Suggested change

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return;

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {

return;

}

blasten · 2020-04-14T03:57:10Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+            if (tx < minMax[0]) minMax[0] = tx;
+            else if (tx > minMax[1]) minMax[1] = tx;
+
+            if (ty < minMax[2]) minMax[2] = ty;
+            else if (ty > minMax[3]) minMax[3] = ty;


I realized the code is missing braces in a few places. The Google style guide for Java suggests that braces are used even if there's only a single statement: https://google.github.io/styleguide/javaguide.html#s4.1.1-braces-always-used

Thank you for reviewing this!

All missing braces should be added now. Does the updated code look good to you?

blasten

LGTM

LongCatIsLooong added 8 commits March 26, 2020 17:01

wip

e6a7d23

wip

ff05c8d

update

056a393

WIP

34283bf

WIP

d3f71da

fix tests

4954bdf

add tests

2f31a97

indentation

0eaa510

auto-assign bot requested a review from iskakaushik April 2, 2020 01:04

googlebot added the cla: yes label Apr 2, 2020

LongCatIsLooong added 3 commits April 1, 2020 18:04

Merge branch 'master' into autofill

f121560

formatting

e87f57f

fix lint

a1967fd

LongCatIsLooong force-pushed the autofill branch from ab92aa4 to a1967fd Compare April 2, 2020 02:01

LongCatIsLooong mentioned this pull request Apr 3, 2020

Autofill Part 1 flutter/flutter#52126

Merged

13 tasks

LongCatIsLooong added 3 commits April 3, 2020 12:15

add "requestAutofill"

7b2ae2c

add androidx.autofill

086ab38

update DEPS

7197492

LongCatIsLooong force-pushed the autofill branch from d5d906a to 7197492 Compare April 10, 2020 19:02

rename allFields -> fields

e1c4334

blasten reviewed Apr 13, 2020

View reviewed changes

justinmc approved these changes Apr 13, 2020

View reviewed changes

LongCatIsLooong added 2 commits April 13, 2020 15:30

Merge remote-tracking branch 'upstream/master' into autofill

7423c08

Revert dependency changes

10a7a2b

LongCatIsLooong force-pushed the autofill branch from 404cffa to a1f3344 Compare April 13, 2020 23:57

review

480f843

LongCatIsLooong force-pushed the autofill branch from a1f3344 to 480f843 Compare April 14, 2020 00:22

blasten reviewed Apr 14, 2020

View reviewed changes

review

0ca379f

LongCatIsLooong force-pushed the autofill branch from 435ac06 to 0ca379f Compare April 14, 2020 04:55

blasten approved these changes Apr 15, 2020

View reviewed changes

LongCatIsLooong merged commit cf3d738 into flutter:master Apr 16, 2020

LongCatIsLooong deleted the autofill branch April 16, 2020 10:41

engine-flutter-autoroll mentioned this pull request Apr 16, 2020

Roll engine e6a2534b63ac..f4d6ce13dcc4 (32 commits) flutter/flutter#54997

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 16, 2020

cf3d738 Android text input autofill (flutter/engine#17465)

b7380a7

bparrishMines mentioned this pull request Mar 29, 2023

[webview_flutter] Autofill doesn't work flutter/flutter#92165

Closed

Conversation

LongCatIsLooong commented Apr 2, 2020

Uh oh!

LongCatIsLooong commented Apr 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants