Get rid of FlutterNativeView, introduce FlutterEngine and FlutterRenderer #6195
Conversation
matthew-carroll
requested review from
amirh,
chinmaygarde,
jason-simmons,
Hixie,
xster and
cbracken
|
Are we going to lose git history for the files that you are copying to legacy? |
| return false; | ||
| } | ||
|
|
||
| // TODO(mattcarroll): this assumes the use of the new API is based on what we're compiling against but that's not true. Need to support both simultaneously. |
There was a problem hiding this comment.
Apps should link against a flutter.jar that contains a matching set of the io.flutter.* Java classes and the libflutter.so binary.
Is there a situation where you're seeing the Java and native sides get out of sync?
There was a problem hiding this comment.
Does this mean we need to update build files to generate different flutter.jar's?
At the moment this .cc file contains support for the old way of doing things, and the new one. My TODO comment was pointing out that the compile-time existence of the new Java class doesn't imply that the developer is necessarily using the new Java class. So the TODO says we need explicit instruction from the Java side as to which universe we're supporting.
I'm not sure off the top of my head how to support customized JARs because developers pull a single build of the engine with the framework, right? How would a developer choose one JAR or the other in that scenario?
Or am I totally misunderstanding your question?
There was a problem hiding this comment.
Does the flutter.jar built on your branch include both the old and new Java classes? Or does it only include the new classes?
If the former, then the JNI code should register implementations for both the old and new method signatures. If the latter, then you can remove the registration code for the old classes.
|
Actually seems like we're going to lose history for e.g FlutterView as well, is it possible to I think we should try harder to maintain git history + it should make the review easier to see what was moved as-is and what are the deltas (hopefully if Github's codreview UI does that for git mv) |
|
Ohh we are on a staging branch 😄, anyway can we do something to matntain git history, and show a reasonable diff for review? (5k lines to read is quite a lot for one PR) |
|
@amirh Fair point about git history. I don't know that we can preserve it for FlutterActivity or FlutterFragment, I'm also not sure we want to, but I'm trying to figure out how to at least pull in the history for everything in /legacy. |
…ge change. Grouped related methods within FlutterView. Deleted a few unused methods.
…d native calls from FlutterView to FlutterEngine.
…ul behavior yet. Need strategy for resolving FlutterNativeView vs FlutterEngine first.
…iminated FlutterEngine references from within FlutterView. Centralized all JNI calls within a FlutterJNI and updated the C side accordingly.
matthew-carroll
force-pushed
the
break-down-flutterview
branch
from
67c2122 to
89f7726
Compare
|
Alright, after committing all copied source files in its own PR and then rebasing, the new code count went from 5,000 lines to just under 2,000. And now there is almost 2,000 lines removed, too. CC @amirh |
matthew-carroll
force-pushed
the
break-down-flutterview
branch
from
89f7726 to
79c5bff
Compare
| } | ||
| //------ END TextureRegistry IMPLEMENTATION ---- | ||
|
|
||
| // TODO(mattcarroll): what exactly is this method intended to do? |
There was a problem hiding this comment.
TMK these are methods called by Android system when the texture lifecycle changes (occluded/needs redrawing etc).
There was a problem hiding this comment.
@mehmetf any idea if there are docs about that lifecycle? I'd like to comment here with the technical specifics of what it means for a surface to be created, changed, or destroyed...
shell/platform/android/io/flutter/embedding/legacy/PlatformViewsController.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/legacy/PlatformViewsController.java
Show resolved
Hide resolved
|
@bparrishMines may also wish to review this |
…o break-down-flutterview
|
CLAs look good, thanks! |
|
CLAs look good, thanks! |
matthew-carroll
merged commit 124d2ff
into
flutter:21008_rewrite-android-embedding-to-reduce-coupling
…erer (flutter#6195) * Copied over FlutterView. Copied a number of dependencies due to package change. Grouped related methods within FlutterView. Deleted a few unused methods. * Moved BinaryMessenger out of FlutterView and into FlutterEngine, moved native calls from FlutterView to FlutterEngine. * Introduced concept of a FlutterRenderer. It doesn't have any meaningful behavior yet. Need strategy for resolving FlutterNativeView vs FlutterEngine first. * Removed discovery behavior from FlutterView to match recent PR: flutter#6157 * Replaced FlutterNativeView with FlutterEngine and FlutterRenderer. Eliminated FlutterEngine references from within FlutterView. Centralized all JNI calls within a FlutterJNI and updated the C side accordingly. * Added comments to FlutterEngine, FlutterRender, FlutterFragment, and FlutterView. * Removed some extraneous code. Adjusted first frame notification slightly. * Fixed formatting in platform_view_android_jni.cc * Update licenses file. * Exposed FlutterEngine from plugin system. Updated some access modifiers as needed. * PR Updates.
…erer (flutter#6195) * Copied over FlutterView. Copied a number of dependencies due to package change. Grouped related methods within FlutterView. Deleted a few unused methods. * Moved BinaryMessenger out of FlutterView and into FlutterEngine, moved native calls from FlutterView to FlutterEngine. * Introduced concept of a FlutterRenderer. It doesn't have any meaningful behavior yet. Need strategy for resolving FlutterNativeView vs FlutterEngine first. * Removed discovery behavior from FlutterView to match recent PR: flutter#6157 * Replaced FlutterNativeView with FlutterEngine and FlutterRenderer. Eliminated FlutterEngine references from within FlutterView. Centralized all JNI calls within a FlutterJNI and updated the C side accordingly. * Added comments to FlutterEngine, FlutterRender, FlutterFragment, and FlutterView. * Removed some extraneous code. Adjusted first frame notification slightly. * Fixed formatting in platform_view_android_jni.cc * Update licenses file. * Exposed FlutterEngine from plugin system. Updated some access modifiers as needed. * PR Updates.
Note: Please ignore the /legacy package in terms of architecture or code quality concerns. The creation of that package was a required step for compilation. I will move classes from /legacy into appropriate packages as I consider them truly "updated" for this next version of the API.
Overarching ideas of this PR
Android is Android, and Flutter is Flutter
We have had a strong entanglement of core Android framework artifacts with core Flutter artifacts. This is especially clear in FlutterView. This PR takes a major step forward in the process of clearly delineating the Android framework classes that happen to render Flutter stuff, vs Flutter classes that can be rendered by the Android framework.
Breakdown Flutter classes by responsibility
Previously, the notion of a "flutter engine" and a "flutter view" were completely fused in FlutterNativeView. This PR introduces 2 important encapsulation boundaries.
First, this PR sets up a composition relationship whereby a FlutterEngine contains within it a reference to a FlutterRenderer that may, or may not be attached to a RenderSurface. This relationship makes explicit the notion of a FlutterEngine running without any UI, or in some manner existing between UIs.
Second, this PR introduces a FlutterJNI class which contains all JNI method declarations. It's unclear if I'll keep them all in one place by the end of the refactor, but with FlutterJNI we are now in a position to introduce an Interface boundary around the JNI calls which will then allow for easy mocking for tests or any other purpose.
PR Wins
No more FlutterNativeView
FlutterNativeView was a Class with a confusing name and an amalgamation of responsibilities. That class has now been replaced by a combination of FlutterEngine, FlutterRenderer, and FlutterJNI.
No more native calls in FlutterView
FlutterView is an Android View class. As such, we don't want to create any conflict for lifecycle or execution control. Therefore, the presence of JNI methods in FlutterView was a problem. Those methods meant that FlutterView needed to be passed around to other Flutter related Objects which then entangled Android lifecycle concerns with Flutter lifecycle concerns.
This PR has extracted all JNI methods from FlutterView (and FlutterNativeView) and placed them in FlutterJNI.
FlutterView no longer shares a lifespan with FlutterNativeView/FlutterEngine
Even though we say that Flutter can technically be attached to, and detached from a FlutterView, the FlutterView constructor actually required the existence of a FlutterNativeView AKA FlutterEngine. Thus, no execution path existed to add, remove, add, remove, etc. a given FlutterNativeView AKA FlutterEngine to/from a given FlutterView.
This PR separates FlutterView construction from the idea of attaching to, and detaching from a FlutterEngine. Now, for the first time, a FlutterView can exist without any reference to a FlutterEngine. It is up to the developer when to attach FlutterView to a given FlutterEngine. This looser coupling allows a view hierarchy to contain a FlutterView while the developer does other work, and then when the other work is done, the developer can simply attach a FlutterEngine to the existing FlutterView. This is generally the paradigm for Android Views. Consider a WebView that sits idle until the developer wants to load a web page. Consider a ListView that sits empty until the developer wants to attach a ListAdapter. The examples go on, but in general an Android View should be able to exist without issue even when it is not connected to any "content". This is now true of FlutterView.
Removed all lifecycle methods from FlutterView
FlutterView previously replicated every Activity/Fragment lifecycle method so that those calls could be forwarded to the plugin system that was partially stuck within FlutterView. Now, instead of FlutterFragment forwarding those calls to the FlutterView, which would then forward those calls to FlutterNativeView, the FlutterFragment holds onto a FlutterEngine and the FlutterFragment reports these lifecycle events directly.
Added JavaDocs
I drafted initial class JavaDocs for FlutterEngine, FlutterRenderer, RenderSurface, and FlutterView.
Issues Introduced by this PR
Lots of logging
This PR added lots of log statements. These logs help ensure that I don't break basic execution paths. They will eventually be removed as the refactor stabilizes, and as tests are added.
Uglier platform_view_android_jni.cc
I'm new to JNI so I made a bit of a mess of the C binding. I'll clean this up once I reach a final Java breakdown of JNI calls.
New /legacy package
Due to a high level of entanglement of Android embedding classes, in conjunction with copious use of package private modifiers, I had no choice in this PR but to copy over a number of source files. Furthermore, as I made changes to FlutterView, FlutterNativeView, FlutterEngine, and FlutterRenderer, a number of those files needed to have reference types updated. For example, FlutterNativeView was deleted, so anything that expected to receive a FlutterNativeView needed to be updated.
Movement of lifecycle calls and some plugins from FlutterView to FlutterFragment
This actually isn't a negative, but it might initially seem that way. It might seem bad to move responsibilities up the dependency graph instead of down. But here is what I'm thinking....
Up to this point we have failed to distinguish between Activity/Fragment lifecycles and View lifecycles. The Flutter embedding has setup many explicit and implicit dependencies on these lifecycles as if all such lifecycles are universally available. This isn't the case. Maybe an Activity is available, maybe it isn't. Maybe a View is available, maybe it isn't. In this next API iteration it is important that we find a way to place responsibility where it belongs.
Therefore, I made FlutterFragment responsible for invoking any Flutter methods that are related to the Activity/Fragment lifecycle. When I extracted that logic from FlutterView, it required that I also move a few plugins from FlutterView to FlutterFragment. I think this is a code smell that suggests that our current treatment of plugins within FlutterView is probably incorrect.
In a followup PR I intend to look at how we setup all default plugins within FlutterFragment and FlutterView and see if it deserves a different treatment in general. But for now the current shift to FlutterFragment is a step in the right direction.