Add Android module libraries to Flutter project #2474

stevemessick · 2018-07-09T21:37:26Z

Analyzes the Android module and adds all libraries required to compile its code to the top-level Flutter project. This should fix problems related to not having indexed all the code in the Android module (i.e. completion, navigation, etc.)

It probably still needs work but I'm not sure what the issues are. It takes noticeably more time to open a project since a Gradle sync is required, but that is no worse than what Android Studio does for an Android project.

devoncarew · 2018-07-10T16:00:08Z

Awesome! Looking now...

devoncarew

Several questions :)

devoncarew · 2018-07-10T16:08:20Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryType.java

+import javax.swing.*;
+
+public class AndroidModuleLibraryType extends LibraryType<AndroidModuleLibraryProperties> {
+  public static final String LIBRARY_NAME = "Android Libraries";


How will this show up in the UI? As the category in the Project view? If so, perhaps something which indicates that it comes from Flutter (and/or is automanaged)? Flutter Plugin Libraries?

I see the latest is Android Libraries. It could still be useful to give a hint where these libraries come from (the Flutter plugin dependencies), but YMMV.

devoncarew · 2018-07-10T16:09:07Z

flutter-studio/src/io/flutter/project/FlutterProjectSystem.java

+    //return gradleProjectSystem.getAvailableDependency(coordinate, includePreview);
+    Method finders = ReflectionUtil.getMethod(gradleProjectSystem.getClass(), "getAvailableDependency");
+    if (finders == null) {
+      LOG.error("No method found: GradleProjectSystem.getAvailableDependency()");


Is this an error we want to show to the user, or something we just want to log (at a warn level)?

Oops! I forgot to change that. Done.

devoncarew · 2018-07-10T16:11:38Z

flutter-studio/src/io/flutter/project/FlutterProjectSystem.java

+      return (GradleCoordinate) finders.invoke(gradleProjectSystem, coordinate, includePreview);
+    }
+    catch (IllegalAccessException | InvocationTargetException e) {
+      LOG.error(e);


I assume this is a totally unexpected error? Does GradleProjectSystem.getAvailableDependency() itself declare any exceptions? If so, we should like declare them in this method, then instanceof check for them here, and if that's what the invoke threw, re-throw them here.

None of the exceptions will be thrown. The checking is all there to satisfy compilers and style guides. :) The method isn't used in 3.1 and exists in 3.2.

devoncarew · 2018-07-10T16:12:24Z

flutter-studio/src/io/flutter/project/FlutterProjectSystem.java

@@ -85,7 +85,20 @@ public ProjectSystemSyncManager getSyncManager() {
  @SuppressWarnings("override")
  @Nullable
  public GradleCoordinate getAvailableDependency(@NotNull GradleCoordinate coordinate, boolean includePreview) {
-    return null;
+    //return gradleProjectSystem.getAvailableDependency(coordinate, includePreview);


When will we be able to remove the reflection, and can you add a todo: comment for us to come back then and remove it?

We need that reflection as long as we build for 3.1. I added a TODO.

devoncarew · 2018-07-10T16:12:59Z

flutter-studio/src/io/flutter/FlutterStudioStartupActivity.java

@@ -40,5 +41,7 @@ public void runActivity(@NotNull Project project) {
    // Unset this flag for all projects, mainly to ease the upgrade from 3.0.1 to 3.1.
    // TODO(messick) Delete once 3.0.x has 0 7DA's.
    FlutterProjectCreator.disableUserConfig(project);
+    // Monitor Android dependencies.
+    AndroidModuleLibraryManager.startWatching();


Is this generally solid, or should we have an experimental flag to try it out?

I added a flag.

devoncarew · 2018-07-10T17:55:36Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+    doGradleSync(getProject(), (Project x) -> updateAndroidLibraryContent(x));
+  }
+
+  private Void updateAndroidLibraryContent(Project androidProject) {


Getting rid of type-checking warnings was a hassle. If there's a better way to do it I'd like to know. Using "void" will cause a compilation error now, though.

devoncarew · 2018-07-10T17:56:10Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+    doGradleSync(getProject(), (Project x) -> updateAndroidLibraryContent(x));
+  }
+
+  private Void updateAndroidLibraryContent(Project androidProject) {


Have androidProject be @Nonnullable?

Yep, missed that one. Done.

devoncarew · 2018-07-10T17:57:27Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+    LibraryTable androidProjectLibraryTable = LibraryTablesRegistrar.getInstance().getLibraryTable(androidProject);
+    Library[] androidProjectLibraries = androidProjectLibraryTable.getLibraries();
+    if (androidProjectLibraries.length == 0) {
+      LOG.error("Gradle sync was incomplete -- no Android libraries found");


If we do want this to be a user-facing error, we should use the toast notifications (from the FlutterMessages utility class). If not, we should log at a warn level.

devoncarew · 2018-07-10T17:59:50Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+    }
+  }
+
+  private static void fileChanged(@NotNull final Project project, @NotNull final VirtualFile file) {


Just for my own edification, it looks like we watch the build.gradle (in any subdirectory?), and schedule a gradle sync when it changes.

Yes, there are two of them (at least) in an Android module. We might want to watch settings.gradle too.

devoncarew · 2018-07-10T18:02:04Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+    return ServiceManager.getService(project, AndroidModuleLibraryManager.class);
+  }
+
+  public static void startWatching() {


It looks like this will only watch projects that are open at IntelliJ launch?

This runs each time a project is opened.

We're iterating over all open projects below, so we may want to instead just process the project that was opened (which, I think this information is available from the StartupActivity interface).

This is one of those "What was I thinking?" moments. I removed the loop from startWatching() and added the Project as a parameter. Thanks for catching that!

stevemessick · 2018-07-10T21:29:30Z

I made a few changes, PTAL.

stevemessick · 2018-07-11T14:29:49Z

The latest change simplifies the library name. PTAL

There's no need to append the project name. The convention is:

Dart * contains Dart code
Flutter * contains Flutter code
Android * contains Android code

devoncarew

Awesome! Three main comments (also in-line):

we could choose to expose as a user-facing preference (defaulting to off) instead of a system property, if we think we're ready
we should evaluate the workflow that we want with this, esp. wrt when we should drive flutter_tools, and when it should drive gradle
we're iterating over all the projects when a project is opened, and we should likely just examine the specific one that was opened

devoncarew · 2018-07-11T14:24:24Z

flutter-studio/src/io/flutter/FlutterStudioStartupActivity.java

 import io.flutter.project.FlutterProjectCreator;
 import io.flutter.utils.FlutterModuleUtils;
 import org.jetbrains.annotations.NotNull;

+import java.util.MissingResourceException;
+
 public class FlutterStudioStartupActivity implements StartupActivity {


I think if we implement void runActivity(@NotNull Project project); here we can get the reference to the project that is being opened.

devoncarew · 2018-07-11T14:24:53Z

flutter-studio/src/io/flutter/FlutterStudioStartupActivity.java

@@ -40,5 +44,10 @@ public void runActivity(@NotNull Project project) {
    // Unset this flag for all projects, mainly to ease the upgrade from 3.0.1 to 3.1.
    // TODO(messick) Delete once 3.0.x has 0 7DA's.
    FlutterProjectCreator.disableUserConfig(project);
+    // Monitor Android dependencies.
+    if (System.getProperty("flutter.android.library.sync", null) != null) {


Another option here is to add a user facing preference in the preferences (likely defaulting to off initially).

Could do; but later, I think.

devoncarew · 2018-07-11T14:27:35Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+  }
+
+  public void update() {
+    doGradleSync(getProject(), (Project x) -> updateAndroidLibraryContent(x));


OK, one thing to consider:

gradle is being run for these projects, just not explicitly by the user. flutter run will drive gradle as necessary to build the app. flutter packages get already does som set up in terms of maintaining flutter plugin metadata (including editing gradle files?). So, we have options for having some portions of this being driven from the CLI, and us triggering the flutter CLI when appropriate.

devoncarew · 2018-07-11T14:28:56Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+    return ServiceManager.getService(project, AndroidModuleLibraryManager.class);
+  }
+
+  public static void startWatching() {


We're iterating over all open projects below, so we may want to instead just process the project that was opened (which, I think this information is available from the StartupActivity interface).

devoncarew · 2018-07-11T15:16:39Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryType.java

+import javax.swing.*;
+
+public class AndroidModuleLibraryType extends LibraryType<AndroidModuleLibraryProperties> {
+  public static final String LIBRARY_NAME = "Android Libraries";


I see the latest is Android Libraries. It could still be useful to give a hint where these libraries come from (the Flutter plugin dependencies), but YMMV.

pq · 2018-07-11T16:50:22Z

flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

+
+      @Override
+      public void syncSucceeded(@NotNull Project project) {
+        callback.apply(androidProject);


Does it make sense to put the flag update in a finally? Something like:

try { callback.apply(androidProject); } finally { isUpdating.set(false); }

EDIT: I'm probably over-thinking.

No, that doesn't work too well because the callback runs async. Effectively removes the flag. Speaking from experience here :)

stevemessick · 2018-07-11T17:05:03Z

I made a slight modification to the code, in response to @devoncarew's comment about looping over projects. I checked it out and the smoke tests still work as expected so I'll submit without further review. Thanks for the comments!

stevemessick added 8 commits July 9, 2018 07:38

Index gradle deps

220e8c9

checkpoint

5e640dd

checkpoint

78b3629

checkpoint

3c1b786

checkpoint

fec70de

Delete unused experimental classes

b830772

Make Gradle sync run async

b9000e6

Fix compilation errors when compiling for 2017.3

2c9e924

googlebot added the cla: yes label Jul 9, 2018

devoncarew reviewed Jul 10, 2018

View reviewed changes

stevemessick added 2 commits July 10, 2018 14:22

Address review comments

3d105a0

Fix typo

79657d3

Simplify library name

60d6e3b

devoncarew approved these changes Jul 11, 2018

View reviewed changes

pq reviewed Jul 11, 2018

View reviewed changes

Stop looping over all projects and just watch the one being opened

3f52cda

stevemessick merged commit c2a36bc into master Jul 11, 2018

stevemessick deleted the gradle-index branch July 11, 2018 17:08

stevemessick mentioned this pull request Aug 3, 2018

A more convenient IDE solution for apps that do both native android kotlin and flutter flutter/flutter#17692

Closed

pq mentioned this pull request May 15, 2025

[perf] Fix incorrect parentDisposables #8107

Open

Add Android module libraries to Flutter project #2474

Add Android module libraries to Flutter project #2474

Uh oh!

Conversation

stevemessick commented Jul 9, 2018

Uh oh!

devoncarew commented Jul 10, 2018

Uh oh!

devoncarew 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!

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!

stevemessick commented Jul 10, 2018

Uh oh!

stevemessick commented Jul 11, 2018

Uh oh!

devoncarew 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!