Use preDraw for the Android embedding #27645

jiahaog · 2021-07-22T12:57:10Z

On Android, use the preDraw listener to report that the application is drawn only when Flutter has shown the first frame, when using the V2 FlutterActivity and FlutterFragmentActivity.

Related issue: flutter/flutter#85292

Changes

The preDraw listener is used by default for users of the FlutterActivity and FlutterFragmentActivity, unless:
- A custom splash screen is detected (through io.flutter.embedding.android.SplashScreenDrawable / provideSplashScreen). When this is detected, add log warnings to direct users to a migration guide flutter.dev/go/android-splash-migration (not written yet)
- RenderMode.surface is not used. This is because otherwise, FlutterTextureView will be used, but a deadlock will be caused because the underlying SurfaceTexture will never be available when preDraw returns false
Users who use FlutterFragment directly can enable this behavior through the shouldDelayFirstAndroidViewDraw builder method

Implementation Notes

Implement the check in FlutterActivityAndFragmentDelegate instead of FlutterView, to avoid the need to add yet another constructor to conditionally trigger this behavior. FlutterActivityAndFragmentDelegate also knows about the splash screen, and this implementation keeps the splash screen logic consolidated
Added a new parameter to FlutterActivityAndFragmentDelegate.onCreateView. I did not add a overload to preserve the old behavior since FlutterActivityAndFragmentDelegate is package private

Tested

Tested locally on a physical API 30 and API 31 device and it works as expected
Internal presubmit - minor changes needed to start activities asynchronously since some Integration Tests do not draw any frames

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jiahaog · 2021-07-22T13:44:02Z

@blasten @xster still have some WIP issues to iron out, but let me know if y'all have any feedback in the meantime

blasten · 2021-07-22T17:22:13Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityLaunchConfigs.java

@@ -18,6 +18,9 @@
      "io.flutter.embedding.android.NormalTheme";
  /* package */ static final String HANDLE_DEEPLINKING_META_DATA_KEY =
      "flutter_deeplinking_enabled";
+  /* package */ static final String USE_LEGACY_SPLASH_SCREEN_META_DATA_KEY =
+      "io.flutter.embedding.android.use-legacy-splash-screen";


I'm not sure if this meta data is necessary.

It might be worth looking at the case when the user didn't supply a custom drawable. In this case, there's no drawable, so the splash screen is unnecessary.

engine/shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

Line 496 in f05cb77

return null;

I was thinking that it could be the way to decide whether we install the predraw listener or not.

This will mean that the new behavior with preDraw will not be used by default, and is instead opt-in, which sounds a bit simpler, thanks 🙂 . I'll update the PR

do we still need io.flutter.embedding.android.use-legacy-splash-screen or can it be removed?

Whoops, removed it!

…tener after drawing

- Only enable it by default for users who use `FlutterActivity` and `FlutterActivityFragment` - Add checks and disable it by default when it is used with `RenderingMode.texture`

jiahaog · 2021-07-26T11:36:26Z

@blasten I updated the PR with what we discussed last Friday, and also to disable it for FlutterTextureView because that causes deadlocks, since onSurfaceTextureAvailable is never called when the preDraw listener is returning false. Let me know if there are any concerns regarding this.

xster · 2021-07-26T17:50:56Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+          new OnPreDrawListener() {
+            @Override
+            public boolean onPreDraw() {
+              if (isFlutterUiDisplayed) {


I suspect this might run again after you rotate the screen. We might want to run this only once?

I tested it locally and registering the callback only happens once. We also deregister the listener the moment isFlutterUiDisplayed returns true, so it should be safe.

xster · 2021-07-26T17:52:17Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

@@ -269,7 +282,8 @@ View onCreateView(
      LayoutInflater inflater,
      @Nullable ViewGroup container,
      @Nullable Bundle savedInstanceState,
-      int flutterViewId) {
+      int flutterViewId,
+      boolean shouldInterceptFirstDraw) {


maybe shouldDelayFirstAndroidViewDraw?

Much better, thanks!

blasten · 2021-07-26T23:00:11Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+          "A splash screen was provided to Flutter, but this is deprecated. See"
+              + " flutter.dev/go/android-splash-migration for migration steps.");
+      FlutterSplashView flutterSplashView = new FlutterSplashView(host.getContext());
+      flutterSplashView.setId(ViewUtils.generateViewId(486947586));


minor clean up. 486947586 could be a final private static value.

Good point, done

- Rename to shouldDelayFirstAndroidViewDraw - Static variable for Flutter splash view id - Remove change to FlutterActivityLaunchConfigs - Extract predraw to helper method

blasten

LGTM

@VisibleForTesting

- Throw IllegalArgumentException instead of RuntimeException when using TextureView and delaying the first draw - Expose handler with @VisibleForTesting to make it easier to test

jiahaog · 2021-07-27T07:39:10Z

Added some unit tests and some minor visibility tweaks to make testing easier, please take another look 🙂

I'll follow up with the deprecation in a separate PR.

xster · 2021-07-27T09:15:42Z

Still LGTM

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

blasten · 2021-07-28T23:11:12Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+      Log.w(
+          TAG,
+          "A splash screen was provided to Flutter, but this is deprecated. See"
+              + " flutter.dev/go/android-splash-migration for migration steps.");


nice! flutter.dev/go/android-splash-migration returns 404 though

I plan to write this separately along with the deprecation annotations, will follow up

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java

shell/platform/android/io/flutter/embedding/android/FlutterFragment.java

blasten

LGTM

This reverts commit ff770aa.

…)" This reverts commit 7538310.

…)" (#27811)

…er#27788) This reverts commit ff770aa.

flutter#27788)" (flutter#27811)

…er#27788) This reverts commit ff770aa.

flutter#27788)" (flutter#27811)

Use preDraw for Android

274092e

flutter-dashboard bot added the platform-android label Jul 22, 2021

google-cla bot added the cla: yes label Jul 22, 2021

blasten reviewed Jul 22, 2021

View reviewed changes

jiahaog added 4 commits July 23, 2021 10:16

Make preDraw opt-in when users don't pass a custom splash, remove lis…

6529677

…tener after drawing

Conditionally set the preDraw listener

f0737e3

- Only enable it by default for users who use `FlutterActivity` and `FlutterActivityFragment` - Add checks and disable it by default when it is used with `RenderingMode.texture`

Remove test change

add16d1

Update tests to new onCreateView

6a38868

Remove the preDraw listener in onDestroyView

ffa239e

xster reviewed Jul 26, 2021

View reviewed changes

blasten reviewed Jul 26, 2021

View reviewed changes

Update from comments

0f6ebdb

- Rename to shouldDelayFirstAndroidViewDraw - Static variable for Flutter splash view id - Remove change to FlutterActivityLaunchConfigs - Extract predraw to helper method

xster approved these changes Jul 27, 2021

View reviewed changes

blasten approved these changes Jul 27, 2021

View reviewed changes

Add tests

cdabfe9

- Throw IllegalArgumentException instead of RuntimeException when using TextureView and delaying the first draw - Expose handler with @VisibleForTesting to make it easier to test

jiahaog force-pushed the predraw branch from 721d622 to cdabfe9 Compare July 27, 2021 07:07

jiahaog marked this pull request as ready for review July 27, 2021 07:37

blasten reviewed Jul 28, 2021

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java Outdated Show resolved Hide resolved

blasten reviewed Jul 28, 2021

View reviewed changes

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java Outdated Show resolved Hide resolved

blasten reviewed Jul 28, 2021

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterFragment.java Outdated Show resolved Hide resolved

blasten approved these changes Jul 28, 2021

View reviewed changes

Minor updates from comments

50ae616

jiahaog added the waiting for tree to go green label Jul 29, 2021

fluttergithubbot merged commit ff770aa into flutter:master Jul 29, 2021

jiahaog deleted the predraw branch July 29, 2021 03:05

engine-flutter-autoroll mentioned this pull request Jul 29, 2021

Roll Engine from 74121a42bc7b to 40451453ab11 (2 revisions) flutter/flutter#87250

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2021

ff770aa Use preDraw for the Android embedding (flutter/engine#27645)

8bf2ffb

jiahaog added a commit that referenced this pull request Jul 29, 2021

Revert "Use preDraw for the Android embedding (#27645)"

1335965

This reverts commit ff770aa.

jiahaog mentioned this pull request Jul 29, 2021

Revert "Use preDraw for the Android embedding" #27788

Merged

engine-flutter-autoroll mentioned this pull request Jul 29, 2021

Roll Engine from 74121a42bc7b to 0ceee4d68386 (4 revisions) flutter/flutter#87276

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2021

ff770aa Use preDraw for the Android embedding (flutter/engine#27645)

938cd1c

This was referenced Jul 30, 2021

Let the Android embedding adopt OnPreDrawListener flutter/flutter#85292

Closed

Use FLUTTER_VIEW_ID to find the Flutter view flutter/flutter#87324

Merged

jiahaog added a commit that referenced this pull request Jul 30, 2021

Revert "Revert "Use preDraw for the Android embedding (#27645)" (#27788…

f9e2e02

…)" This reverts commit 7538310.

jiahaog mentioned this pull request Jul 30, 2021

Reland using preDraw for the Android embedding #27811

Merged

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Use preDraw for the Android embedding (flutter#27645)

feb1961

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Revert "Use preDraw for the Android embedding (flutter#27645)" (flutt…

053f7d4

…er#27788) This reverts commit ff770aa.

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Revert "Revert "Use preDraw for the Android embedding (flutter#27645)" (

acb36e8

flutter#27788)" (flutter#27811)

blasten mentioned this pull request Sep 11, 2021

java.lang.SecurityException: Permission Denial crash at launch flutter/flutter#88767

Closed

filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022

Use preDraw for the Android embedding (flutter#27645)

dbbd998

filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022

Revert "Use preDraw for the Android embedding (flutter#27645)" (flutt…

ebcdda5

…er#27788) This reverts commit ff770aa.

filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022

Revert "Revert "Use preDraw for the Android embedding (flutter#27645)" (

2197ecb

flutter#27788)" (flutter#27811)

Use preDraw for the Android embedding #27645

Use preDraw for the Android embedding #27645

Conversation

jiahaog commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Implementation Notes

Tested

Pre-launch Checklist

Uh oh!

jiahaog commented Jul 22, 2021

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!

jiahaog commented Jul 26, 2021

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!

blasten Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

jiahaog commented Jul 27, 2021

Uh oh!

xster commented Jul 27, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

jiahaog commented Jul 22, 2021 •

edited

Loading

blasten Jul 26, 2021 •

edited

Loading