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

Allow FlutterEngine to be used on back-to-back screens (#31264). #8808

Merged
matthew-carroll merged 2 commits into from
May 2, 2019
Merged

Allow FlutterEngine to be used on back-to-back screens (#31264). #8808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6697656
Allow FlutterEngine to be used on back-to-back screens (#31264).
May 2, 2019
7f3773c
Moved FlutterView/FlutterEngine detachment to FlutterFragment's onSto…
May 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions shell/platform/android/io/flutter/embedding/android/FlutterFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,25 +378,6 @@ protected FlutterEngine createFlutterEngine(@NonNull Context context) {
public View onCreateView(LayoutInflater inflater, @Nullable ViewGroup container, @Nullable Bundle savedInstanceState) {
flutterView = new FlutterView(getContext(), getRenderMode(), getTransparencyMode());
flutterView.addOnFirstFrameRenderedListener(onFirstFrameRenderedListener);

// We post() the code that attaches the FlutterEngine to our FlutterView because there is
// some kind of blocking logic on the native side when the surface is connected. That lag
// causes launching Activitys to wait a second or two before launching. By post()'ing this
// behavior we are able to move this blocking logic to after the Activity's launch.
// TODO(mattcarroll): figure out how to avoid blocking the MAIN thread when connecting a surface
new Handler().post(new Runnable() {
@Override
public void run() {
flutterView.attachToFlutterEngine(flutterEngine);

// TODO(mattcarroll): the following call should exist here, but the plugin system needs to be revamped.
// The existing attach() method does not know how to handle this kind of FlutterView.
//flutterEngine.getPluginRegistry().attach(this, getActivity());

doInitialFlutterViewRun();
}
});

return flutterView;
}

Expand Down Expand Up @@ -486,9 +467,34 @@ protected FlutterView.TransparencyMode getTransparencyMode() {
return FlutterView.TransparencyMode.valueOf(transparencyModeName);
}

@Override
public void onStart() {
Copy link
Contributor

@mklim mklim May 2, 2019

Choose a reason for hiding this comment

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

How often do we want this to repeat? onStart isn't executed every time the activity is brought into the foreground either unfortunately. onResume would be the right fit for that case.

On the flip side, is flutterView.detachFromFlutterEngine(); the mirror of this? If so it should probably be moved into onStop so it's called in equivalent situations, or onPause if this ultimately needs to be moved into onResume.

Copy link
Contributor Author

@matthew-carroll matthew-carroll May 2, 2019

Choose a reason for hiding this comment

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

Placing this in onResume() would imply that the UI could attach/detach while the Activity is still visible, but developers shouldn't do that. For example, when a dialog pops up, onPause() is invoked. I think the only reason to put this in onResume() would be if we expected to detach in onPause(), but in that case the Flutter UI would disappear out from under the dialog. So I do think onStart() is the correct lifecycle hook.

As for detaching, the reason that we need to attach in onStart() is in case the underlying engine was re-used in another Activity. So when returning to this Activity we re-attach in onStart(). Then there is detaching, and you asked if detaching should be symmetric to attaching. Technically we could detach in onStop() as you suggested, but it also occurred to me that in most cases that detachment is superfluous. In most cases an engine is probably not being re-used, but we'd go through detachment every time the FlutterActivity hits onStop().

I don't know if there is a meaningful performance risk to detaching like that, but I figured we can avoid it by not explicitly detaching until we know the FlutterFragment is detaching from the Activity or being destroyed.

Does that make sense? Do you still think detachment should happen in on onStop() to be symmetric?

Copy link
Contributor

@mklim mklim May 2, 2019

Choose a reason for hiding this comment

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

The onStart() vs onResume() change makes sense to me.

For detaching, I guess my remaining question is is there a consequence for the engine not being detached in line with another potential attach() call? Is that a potential issue or is there no larger issue there? If it doesn't matter then it's fine to be asymmetrical, but I was just assuming that there would be some kind of consequence for overlap there so you'd want to make sure it was torn up and down more predictably.

Copy link
Contributor Author

@matthew-carroll matthew-carroll May 2, 2019

Choose a reason for hiding this comment

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

Looking again at attachment, given that a number of things are setup there, and we may discover that they need to be cleaned up in detachment, I do think the safer thing is to be symmetric. So I'm going to move detachment to onStop().

super.onStart();
Log.d(TAG, "onStart()");

// We post() the code that attaches the FlutterEngine to our FlutterView because there is
// some kind of blocking logic on the native side when the surface is connected. That lag
// causes launching Activitys to wait a second or two before launching. By post()'ing this
// behavior we are able to move this blocking logic to after the Activity's launch.
// TODO(mattcarroll): figure out how to avoid blocking the MAIN thread when connecting a surface
new Handler().post(new Runnable() {
@Override
public void run() {
flutterView.attachToFlutterEngine(flutterEngine);

// TODO(mattcarroll): the following call should exist here, but the plugin system needs to be revamped.
// The existing attach() method does not know how to handle this kind of FlutterView.
//flutterEngine.getPluginRegistry().attach(this, getActivity());

doInitialFlutterViewRun();
}
});
}

@Override
public void onResume() {
super.onResume();
Log.d(TAG, "onResume()");
flutterEngine.getLifecycleChannel().appIsResumed();
}

Expand Down Expand Up @@ -517,14 +523,14 @@ public void onStop() {
super.onStop();
Log.d(TAG, "onStop()");
flutterEngine.getLifecycleChannel().appIsPaused();
flutterView.detachFromFlutterEngine();
}

@Override
public void onDestroyView() {
super.onDestroyView();
Log.d(TAG, "onDestroyView()");
flutterView.removeOnFirstFrameRenderedListener(onFirstFrameRenderedListener);
flutterView.detachFromFlutterEngine();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/io/flutter/embedding/android/FlutterView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ public void detachFromFlutterEngine() {
}

private boolean isAttachedToFlutterEngine() {
return flutterEngine != null;
return flutterEngine != null && flutterEngine.getRenderer().isAttachedTo(renderSurface);
}

/**
Expand Down
9 changes: 9 additions & 0 deletions shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import android.os.Build;
import android.os.Handler;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.view.Surface;

import java.nio.ByteBuffer;
Expand Down Expand Up @@ -43,6 +44,14 @@ public FlutterRenderer(@NonNull FlutterJNI flutterJNI) {
this.flutterJNI = flutterJNI;
}

/**
* Returns true if this {@code FlutterRenderer} is attached to the given {@link RenderSurface},
* false otherwise.
*/
public boolean isAttachedTo(@NonNull RenderSurface renderSurface) {
return this.renderSurface == renderSurface;
}

public void attachToRenderSurface(@NonNull RenderSurface renderSurface) {
// TODO(mattcarroll): determine desired behavior when attaching to an already attached renderer
if (this.renderSurface != null) {
Expand Down