New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Android embedding refactor pr3 add remaining systemchannels #7738

Merged

matthew-carroll merged 6 commits into flutter:master from matthew-carroll:android_embedding_refactor_pr3_add_remaining_systemchannels

Feb 15, 2019

Contributor

matthew-carroll commented Feb 8, 2019

This PR introduces implementations for an AccessibilityChannel, TextInputChannel, and PlatformChannel.

This PR should not alter any existing behavior, it just moves channel communication logic into Java APIs where it was previously written in the middle of other logic. Where possible, I introduced enums and data structures to codify message content that is received by Android from Flutter.

matthew-carroll requested review from goderbauer, GaryQian, mklim, jonahwilliams and jason-simmons

February 8, 2019 00:59

googlebot added the cla: yes label

jason-simmons reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/systemchannels/AccessibilityChannel.java Outdated Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java Outdated Show resolved Hide resolved

GaryQian reviewed

View reviewed changes

Contributor

GaryQian left a comment

The locale passing part LGTM, deferring additional review of other parts to others as I am not particularly familiar with the rest.

matthew-carroll mentioned this pull request

Recommended implementation of combining characters implementation. #7758

Merged

jason-simmons reviewed

View reviewed changes

Member

jason-simmons left a comment

LGTM

shell/platform/android/io/flutter/embedding/engine/systemchannels/AccessibilityChannel.java Outdated Show resolved Hide resolved

GaryQian reviewed

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java Outdated Show resolved Hide resolved

mklim reviewed

View reviewed changes

Contributor

mklim left a comment

This is just a partial review for up to shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java. I'll try to review the rest soon. In the future it would be really helpful if PRs like this were split into smaller chunks. 2000 net code lines changed is challenging to try and go through in one review.

shell/platform/android/io/flutter/embedding/engine/systemchannels/AccessibilityChannel.java Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/AccessibilityChannel.java Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java Outdated Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java Outdated Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java Outdated Show resolved Hide resolved

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

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

+                 * A text editing configuration.
+                 */
+                public static class Configuration {
+                  public static Configuration fromJson(@NonNull JSONObject json) throws JSONException {

Contributor

mklim Feb 9, 2019

Maybe document what the JSONObject should be formatted to look like?

Contributor Author

matthew-carroll Feb 11, 2019

I'm of two minds about this. On the one hand I agree with you that it would be really nice for devs to look at the javadoc and see the format. On the other hand I'm wondering how many days it will take for that comment to fall out of sync with the code.....what do you think?

Contributor

mklim Feb 14, 2019

That's the inherent problem with Javadoc and code comments, unfortunately. Right now this is basically taking an untyped Object object and then risking exploding at runtime if it doesn't happen to be the right things. It makes sense to me that the right structure should be documented somewhere. If not here than it makes sense to link to an example file or something similar where you can see the keys.

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java Outdated Show resolved Hide resolved

Contributor Author

matthew-carroll commented Feb 12, 2019

I'm going to force push a rebase to handle the conflicts.

matthew-carroll force-pushed the android_embedding_refactor_pr3_add_remaining_systemchannels branch from ad3351a to 03ca20c Compare

February 12, 2019 00:07

Matt Carroll added 6 commits

February 11, 2019 16:08


          Switched FlutterView over to use LocalizationChannel.

227ec13


          Added implementations for AccessibilityChannel, PlatformChannel, and …

c6ab42f

…TextInputChannel.


          Improved PlatformChannel comments.

3eac93f


          PR Updates.

9d43997


          PR Updates.

5ab5835


          PR Updates.

b7fcd5b

matthew-carroll force-pushed the android_embedding_refactor_pr3_add_remaining_systemchannels branch from 03ca20c to b7fcd5b Compare

February 12, 2019 00:09

Contributor Author

matthew-carroll commented Feb 12, 2019

@mklim please take another look when you get a chance. I responded to each of your comments. Only a couple were left open ended. Thanks.

mklim reviewed

View reviewed changes

Contributor

mklim left a comment

The classes I've looked at in detail LGTM in terms of Java style. Unfortunately I haven't reviewed the entire PR though.

Contributor Author

matthew-carroll commented Feb 15, 2019

Gonna merge this in with the understanding that @mklim is welcome to add more comments later if he gets time to review this any further, but I believe the reviews and modifications up to this point cover the important stuff.

matthew-carroll merged commit 256db4b into flutter:master

Member

cbracken commented Feb 15, 2019

This appears to have broken the build.

cbracken added a commit to cbracken/flutter_engine that referenced this pull request


          Revert "Android embedding refactor pr3 add remaining systemchannels (f…

1ee067b

…lutter#7738)"

From build log:
```
../../flutter/shell/platform/android/io/flutter/view/FlutterView.java:95: error: cannot find symbol
    private final LocalizationChannel localizationChannel;
                  ^
  symbol:   class LocalizationChannel
  location: class FlutterView
../../flutter/shell/platform/android/io/flutter/view/FlutterView.java:172: error: cannot find symbol
        localizationChannel = new LocalizationChannel(dartExecutor);
                                  ^
  symbol:   class LocalizationChannel
  location: class FlutterView
2 errors
```

This reverts commit 256db4b.

cbracken mentioned this pull request

Revert "Android embedding refactor pr3 add remaining systemchannels (… #7849

Merged

cbracken added a commit that referenced this pull request


          Revert "Android embedding refactor pr3 add remaining systemchannels (#…

163a2fd

…7738)" (#7849)

From build log:
```
../../flutter/shell/platform/android/io/flutter/view/FlutterView.java:95: error: cannot find symbol
    private final LocalizationChannel localizationChannel;
                  ^
  symbol:   class LocalizationChannel
  location: class FlutterView
../../flutter/shell/platform/android/io/flutter/view/FlutterView.java:172: error: cannot find symbol
        localizationChannel = new LocalizationChannel(dartExecutor);
                                  ^
  symbol:   class LocalizationChannel
  location: class FlutterView
2 errors
```

This reverts commit 256db4b.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          256db4b Android embedding refactor pr3 add remaining systemchannels (f…

7704b74

…lutter/engine#7738)

engine-flutter-autoroll mentioned this pull request

Roll engine 033f2072c315..163a2fd5e140 (26 commits) flutter/flutter#28035

Merged

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request


          Roll engine 033f207..163a2fd (26 commits) (#28035)

85c045c

flutter/engine@033f207...163a2fd

git log 033f207..163a2fd --no-merges --oneline
163a2fd Revert &#34;Android embedding refactor pr3 add remaining systemchannels (#7738)&#34; (flutter/engine#7849)
0d6ff16 Shut down and restart the Dart VM as needed. (flutter/engine#7832)
256db4b Android embedding refactor pr3 add remaining systemchannels (flutter/engine#7738)
02d7ca3 Don&#39;t call static method from instance variable (flutter/engine#7841)
10cee61 Delete GL textures when they are released from the texture registry. (flutter/engine#7836)
69e4606 Fix NullPointerException in SurfaceTextureRegistryEntry (flutter/engine#7837)
9a965bc Fix NullPointerException in ResourceCleaner (flutter/engine#7838)
2acd794 Add fml::FileExists implementation for Windows (flutter/engine#7845)
fa38a98 Update buildroot to 7f64ff4928e to unblock Mac builds. (flutter/engine#7846)
1ba3295 Add support for calling into other plugins from a background context on iOS (flutter/engine#7843)
df2fc97 Roll src/third_party/skia 154acd7a1374..be9aff25bddc (13 commits) (flutter/engine#7842)
0ca1d1f Remove the Dart JIT snapshot data from AOT builds of the embedder library (flutter/engine#7806)
c51ea41 Roll src/third_party/skia 7a74c7cb6da0..154acd7a1374 (1 commits) (flutter/engine#7840)
ecbaea1 Ensure to pass dill file after VM options for gen_snapshot (flutter/engine#7839)
5d3f714 Roll src/third_party/skia b00f7b34751b..7a74c7cb6da0 (1 commits) (flutter/engine#7835)
7337399 Roll src/third_party/skia 9a88bee122f1..b00f7b34751b (24 commits) (flutter/engine#7831)
6f318ef Move up ndk version that is being downloaded(old one no longer available) (flutter/engine#7830)
a8aa1ee Track flow id (flutter/engine#7826)
b1ce6b7 Roll src/third_party/skia a0dcd29f536b..9a88bee122f1 (4 commits) (flutter/engine#7825)
dbc1663 Roll src/third_party/skia 6152470dc69e..a0dcd29f536b (2 commits) (flutter/engine#7824)
9426776 Roll src/third_party/skia 4037f7f5d8b4..6152470dc69e (1 commits) (flutter/engine#7823)
e2394ad Add flow events connecting pointer events to frames (flutter/engine#7807)
0a5a7c5 Roll src/third_party/skia 09c01e9df260..4037f7f5d8b4 (3 commits) (flutter/engine#7821)
b867f48 Roll src/third_party/skia 186669c4128b..09c01e9df260 (5 commits) (flutter/engine#7820)
af39e22 Fix tests that were committed after cirrus ran (flutter/engine#7819)
1f2cbf7 Roll src/third_party/dart 0a7dcf17eb..c92d5ca288 (64 commits)

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 (liyuqian@google.com), and stop
the roller if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jason-simmons jason-simmons left review comments

mklim mklim left review comments

GaryQian GaryQian left review comments

goderbauer Awaiting requested review from goderbauer

jonahwilliams Awaiting requested review from jonahwilliams

Labels