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

Prevent long flash when switching to Flutter app. (#47903) #16527

Merged
matthew-carroll merged 2 commits into from
Feb 11, 2020
Merged

Prevent long flash when switching to Flutter app. (#47903) #16527

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37de638
Prevent long flash when switching to Flutter app. (#47903)
matthew-carroll Feb 8, 2020
f5d9026
Formatting fixes.
matthew-carroll Feb 10, 2020
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
44 changes: 17 additions & 27 deletions shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import android.content.Intent;
import android.os.Build;
import android.os.Bundle;
import android.os.Handler;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.VisibleForTesting;
Expand Down Expand Up @@ -235,11 +234,16 @@ void onAttach(@NonNull Context context) {
/**
* Invoke this method from {@code Activity#onCreate(Bundle)} to create the content {@code View},
* or from {@code Fragment#onCreateView(LayoutInflater, ViewGroup, Bundle)}.
*
* <p>{@code inflater} and {@code container} may be null when invoked from an {@code Activity}.
*
* <p>This method creates a new {@link FlutterView}, adds a {@link FlutterUiDisplayListener} to
* it, and then returns it.
* <p>
Copy link
Contributor

@mklim mklim Feb 10, 2020

Choose a reason for hiding this comment

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

nit: I expect the formatter is going to complain about this, ./ci/format.sh | patch -p0 should work to fix it. If you install google-java-format in your IDE of choice you should be able to get the changes applied within IntelliJ too. https://github.com/google/google-java-format#intellij-android-studio-and-other-jetbrains-ides

Copy link
Contributor Author

@matthew-carroll matthew-carroll Feb 10, 2020

Choose a reason for hiding this comment

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

Thanks. Formatter installed and run.

* {@code inflater} and {@code container} may be null when invoked from an {@code Activity}.
* <p>
* This method:
* <ol>
* <li>creates a new {@link FlutterView} in a {@code View} hierarchy</li>
* <li>adds a {@link FlutterUiDisplayListener} to it</li>
* <li>attaches a {@link FlutterEngine} to the new {@link FlutterView}</li>
* <li>returns the new {@code View} hierarchy</li>
* </ol>
*/
@NonNull
View onCreateView(
Expand All @@ -261,6 +265,9 @@ View onCreateView(
}
flutterSplashView.displayFlutterViewWithSplash(flutterView, host.provideSplashScreen());

Log.v(TAG, "Attaching FlutterEngine to FlutterView.");
flutterView.attachToFlutterEngine(flutterEngine);
Copy link
Contributor

@mklim mklim Feb 10, 2020

Choose a reason for hiding this comment

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

The main potential negative implication of this I can think of is that this is going to be blocking rendering from the Android side. So if for some reason attachToFlutterEngine takes a long time the user is going to see a black screen and Android may want to kill the Flutter app.

As I understand it this is a very cheap method and shouldn't block for any noticeable amount of time though, so I'm not worried about it.

Copy link
Contributor Author

@matthew-carroll matthew-carroll Feb 10, 2020

Choose a reason for hiding this comment

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

Yeah, that was the tradeoff made here. We'll accept whatever this blocking call is in exchange for not having the very noticeable black screen at each app switch. I think this blocking call was the original impetus to post it on the main handler.


return flutterSplashView;
}

Expand All @@ -281,31 +288,13 @@ void onActivityCreated(@Nullable Bundle bundle) {
* <p>
*
* <ol>
* <li>Attaches the {@link FlutterEngine} owned by this delegate to the {@link FlutterView}
* owned by this delegate.
* <li>Begins executing Dart code, if it is not already executing.
* <li>Begins executing Dart code, if it is not already executing.</li>
* </ol>
*/
void onStart() {
Log.v(TAG, "onStart()");
ensureAlive();

// 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() {
Log.v(TAG, "Attaching FlutterEngine to FlutterView.");
flutterView.attachToFlutterEngine(flutterEngine);

doInitialFlutterViewRun();
}
});
doInitialFlutterViewRun();
}

/**
Expand Down Expand Up @@ -418,7 +407,6 @@ void onStop() {
Log.v(TAG, "onStop()");
ensureAlive();
flutterEngine.getLifecycleChannel().appIsPaused();
flutterView.detachFromFlutterEngine();
}

/**
Expand All @@ -429,6 +417,8 @@ void onStop() {
void onDestroyView() {
Log.v(TAG, "onDestroyView()");
ensureAlive();

flutterView.detachFromFlutterEngine();
Copy link
Contributor

@mklim mklim Feb 10, 2020

Choose a reason for hiding this comment

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

The big potential drawback of the move to here is that this method isn't always actually called before the activity is killed, so there's a chance that FlutterView is never going to see this call in some cases now.

I'm not sure how much of a concern this is because I'm not sure how often this happens on Android, just that it does happen and it's not recommended to actually do critical work on onDestroy for that reason. As I understand it this typically happens if Android is about to kill the app for going OOM. The typical "fix" for this is to move actual critical resource releasing into onPause instead. I don't think that's really necessary here, but it may be something to revisit if we start seeing a lot of bugs around plugins not getting detached callbacks.

Copy link
Contributor Author

@matthew-carroll matthew-carroll Feb 10, 2020

Choose a reason for hiding this comment

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

Agreed. The reason this was initially done elsewhere is because of the non-guarantee that you mention. But my hope/expectation is that if onDestroy() is skipped it's because the app is being evicted, at which point nothing will survive.

flutterView.removeOnFirstFrameRenderedListener(flutterUiDisplayListener);
}

Expand Down