Android Embedding Refactor PR36: Add splash screen support. by matthew-carroll · Pull Request #9525 · flutter-team-archive/engine

matthew-carroll · 2019-06-27T08:44:51Z

Android Embedding Refactor PR36: Add splash screen support.

xster · 2019-07-02T02:22:36Z


  @Override
  protected void onCreate(@Nullable Bundle savedInstanceState) {
+    switchLaunchThemeForNormalTheme();


What specific disconnect could happen here? If we were showing an activity in launch theme and no status bar etc, this onCreate happens at a point when we're potentially handing off the OS's launch drawable showing to our own 'splash view' showing right? If we switch the theme right here, would the status bar appear in the middle of an otherwise seamless transition?

Should we document that perhaps people might want to create custom hooks to delay this theme switching until they call transitionToFlutter?

I'm not sure about the specific visual artifacts that you're referring to. Can you provide some kind of example code or repro steps so I can see what you're concerned about?

You resolved this with new docs

xster · 2019-07-02T02:23:22Z

+   * operation.
+   * <p>
+   * This behavior is offered so that a "launch screen" can be displayed while the
+   * application initially loads. To utilize this behavior in an app, do the following:


while the flutter application initially loads right? Otherwise people will ask why I don't need to do this with my normal android app.

People do need to do this in their normal Android app.

xster · 2019-07-02T02:24:42Z

+        setTheme(normalThemeRID);
+      }
+    } catch (PackageManager.NameNotFoundException exception) {
+      Log.e(TAG, "Could not read meta-data for FlutterActivity.");


Could not read meta-data for FlutterActivity. Leaving the launch theme in place.

xster · 2019-07-02T02:26:03Z

+    flutterSplashView.setSplashScreen(provideSplashScreen());
+    flutterSplashView.setFlutterView(flutterView);
+
+    return flutterSplashView;


Ah I see. This is a neat way of keeping things modular.

xster · 2019-07-02T02:26:35Z

     */
    void configureFlutterEngine(@NonNull FlutterEngine flutterEngine);
  }
+


xster · 2019-07-02T02:31:51Z

+   * an existing {@link FlutterView} if {@code flutterView} is {@code null}.
+   */
+  public void setFlutterView(@Nullable FlutterView flutterView) {
+    if (this.flutterView != null) {


Since the default fragment always calls setSplashScreen and setFlutterView in quick succession, can we avoid some of this adding, removing and adding redundancy by also checking splashScreen != null && flutterView != null && !flutterView.hasRenderedFirstFrame() here or some such?

What's the redundancy that you're seeing? There are 2 details leading to timing complexity:

We can't enforce a SplashScreen in the constructor because of Android View constructor expectations (annoying).

FlutterSplashView should be capable of operating without a SplashScreen at all so that a FlutterSplashView can always be used in a compositional approach without needing to know ahead of time whether a splash will actually be shown.

Given those 2 details, I'm not aware of any redundancy. But feel free to correct me.

You resolved this with added docs from our last offline chat

xster · 2019-07-02T21:45:13Z

+   * The given {@link SplashScreen} is displayed as soon as this method is invoked, or
+   * upon attachment to the {@code Window}, whichever happens last.
+   * <p>
+   * If Flutter has already rendered its first frame, this method does not have any


discussed offline: also add that it doesn't do anything if no flutter view is set

xster · 2019-07-02T21:46:00Z

+
+  /**
+   * Displays the given {@link FlutterView} in this {@code FlutterSplashView}, or removes
+   * an existing {@link FlutterView} if {@code flutterView} is {@code null}.


If the FlutterView hasn't rendered its first frame yet, the splash view will be shown instead.

xster · 2019-07-02T22:29:57Z

LGTM

mklim

These lint errors look like real failures to me:

 [LintBaseline]
../../../flutter/shell/platform/android/io/flutter/embedding/android/SplashScreen.java:43: Error: Default method requires API level 24 (current min is 16) [NewApi]
  /**
  ^
../../../flutter/shell/platform/android/io/flutter/embedding/android/SplashScreen.java:74: Error: Default method requires API level 24 (current min is 16) [NewApi]
  /**
  ^
../../../flutter/shell/platform/android/io/flutter/embedding/android/FlutterSplashView.java:266: Error: Using the default class loader will not work if you are restoring your own classes. Consider using for example readBundle(getClass().getClassLoader()) instead. [ParcelClassLoader]
      splashScreenState = source.readBundle();
                                 ~~~~~~~~~~~~
3 errors, 1 warnings (15 errors filtered by baseline baseline.xml)

mklim · 2019-07-08T19:39:06Z

+ * {@link SplashScreen} that displays a given {@link Drawable}, which then cross-fades to
+ * Flutter's content.
+ */
+public class DrawableSplashScreen implements SplashScreen {


nit: Are we planning for this to be overridden? If not, safer to prohibit it and mark this final.

https://en.wikipedia.org/wiki/Fragile_base_class and Effective Java Item 19 talk about this in depth.

nit, but I think this still applies.

matthew-carroll · 2019-07-08T20:54:34Z

@mklim are you able to tell what the first 2 errors are in the linter? It's pointing to javadoc lines.

Also, do you know what the right resolution to the 3rd error is? Is it really to just pass a new class loader instance? I've never seen that issue before...

mklim · 2019-07-08T22:20:30Z

are you able to tell what the first 2 errors are in the linter? It's pointing to javadoc lines.

Pretty sure that it's trying to point to the methods underneath the Javadoc. The default method declaration is the problem. Looking into this more, default methods do come with Java 8 in all minSdkVersions. Is the engine OK requiring that it's always compiled with Java 8? If so I think it's probably OK to ignore these lines from the linter with a comment about it.

Also, do you know what the right resolution to the 3rd error is? Is it really to just pass a new class loader instance? I've never seen that issue before...

I've never seen it either. Found an issue with more detail about the lint. From what I can tell this looks like some issue where null usually ends up with Parcelable then going on to figure out what class to use automatically, but it can't always automatically figure it out with custom classes. And according to the lint this method ends up calling through to Parcelable and passing null in this case. So the lint is asking to pass in a class instance explicitly giving it something to work with instead so it doesn't hit that bug trying to automatically figure out what class to use.

matthew-carroll · 2019-07-09T10:47:25Z

@xster WRT to @mklim's comment above, we're good with Java 8 at this point, right?

mklim · 2019-07-09T20:30:47Z

we're good with Java 8 at this point, right?

Looked into this more. As far as I can tell we should be safe to use features with no minSdk requirement like this. We're also already using lambdas in PlatformViewsController.

matthew-carroll · 2019-07-09T22:19:04Z

@mklim do you know if the answer here is to add a suppression annotation to the code? or do we need to update the baseline for the CI linter?

mklim · 2019-07-09T22:27:27Z

do you know if the answer here is to add a suppression annotation to the code? or do we need to update the baseline for the CI linter?

Kind of subjective, but I'd prefer adding a @SuppressLint("NewApi") at the relevant line in the code with a comment explaining why the lint doesn't apply here for any future readers instead of updating the baseline. The baseline was our state of the world before adopting the linter, and long term we ideally want to drive it to 0. We don't want to add stuff to it that we actually need to preserve. I'd only rebaseline for stuff like moving old code from file A to file B, where the baseline doesn't apply anymore but it's still the same old failures.

matthew-carroll · 2019-07-16T23:20:42Z

@mklim does this PR look acceptable now?

@xster would you mind giving another LGTM? There have been a number of updates since your first one.

xster · 2019-07-17T18:45:40Z

+ * <p>
+ * {@code FlutterActivity} supports the display of an Android "launch screen" as well as a
+ * Flutter-specific "splash screen". The launch screen is displayed while the Android application
+ * loads. It is only applicable if {@code FlutterActivity} is the first {@code Activity} displayed


This sentence is a bit confusing. The previous sentence is "The launch screen is displayed while the Android application loads". Isn't that always true? This makes it sound like we're saying the launch screen only shows if FlutterActivity is the first activity whereas I'd imagine it's just always true.

A launch screen can be displayed in any Android app if developers do the relevant work. In the context of a full-Flutter app, Flutter offers a particular set of steps to display a launch screen. Those steps are similar, but not identical, to a standard Android app. Therefore, in all of these Javadocs, the language refers specifically to this Flutter process within a Flutter app.

ah right because we're applying the windowBackground styling to the flutter activity specifically. Makes sense

xster · 2019-07-17T19:55:22Z


  @Override
  protected void onCreate(@Nullable Bundle savedInstanceState) {
+    switchLaunchThemeForNormalTheme();


You resolved this with new docs

xster · 2019-07-17T20:05:36Z

+   * an existing {@link FlutterView} if {@code flutterView} is {@code null}.
+   */
+  public void setFlutterView(@Nullable FlutterView flutterView) {
+    if (this.flutterView != null) {


You resolved this with added docs from our last offline chat

xster · 2019-07-17T20:11:08Z

+    flutterRenderer.detachFromRenderSurface();
    flutterEngine = null;
-
-    // TODO(mattcarroll): clear the surface when JNI doesn't blow up


commented code

This comment is on removed code.

xster · 2019-07-17T22:49:51Z

+    // If the first frame has already been rendered, notify all first frame listeners.
+    // Do this after all other initialization so that listeners don't inadvertently interact
+    // with a FlutterView that is only partially attached to a FlutterEngine.
+    if (didRenderFirstFrame) {


just call onFirstFrameRendererListener.onFirstFrameRendered here?

I'm not sure what you're suggesting, can you elaborate?

<not super important, feel free to do what you think makes sense> I just meant could we avoid duplicating the logic here and just call onFirstFrameRenderListener.onFirstFrameRendered() which basically does the same thing here?

Ah, I see what you're saying now. Yeah I think we can make that change.

xster · 2019-07-17T22:54:15Z

  private final FlutterJNI flutterJNI;
  private final AtomicLong nextTextureId = new AtomicLong(0L);
  private RenderSurface renderSurface;
+  private boolean hasRenderedFirstFrame = false;


I get that you're saving this via bundles in the fluttersplashview. Why are you saving it again here?

I'm not sure what you mean by "saving"? Do you mean caching as a variable? It's returned by hasRenderedFirstFrame()...

never mind, the SavedState thing is 2 staged. You had to figure out both whether the current view instance rendered the first frame and whether another instance rendered the first frame before.

xster · 2019-07-18T20:22:25Z

+  }
+
  public void addOnFirstFrameRenderedListener(@NonNull OnFirstFrameRenderedListener listener) {
    flutterJNI.addOnFirstFrameRenderedListener(listener);


what does this do in this case? If a first frame already rendered with the renderer and another view calls this, it gets called back instantly and the listener gets registered through jni as a listener again? Does that listener get called again at some point?

I think your description is correct. The listener when added to a FlutterRenderer should not be invoked again because a Dart isolate can't be entered twice. But that's kind of an implementation detail, and this listener interface is also used at the FlutterView level, which could be called many times. So generally speaking wherever such a listener is involved, we accept adding and removing of listeners, and we invoke them whenever is appropriate for that place.

However, there is some cleanup I'd like to do WRT to these first frame callbacks.

xster · 2019-07-18T20:22:38Z

LGTM with tests

matthew-carroll · 2019-07-18T21:00:21Z

@xster you mentioned tests. Tests here and ready for review: flutter/flutter#35728

xster · 2019-07-20T02:34:28Z

also cc @gaaclarke

… path.

mklim

LGTM with a few nits. Thanks for adding tests in the other PR!

mklim · 2019-07-22T20:31:54Z

+ * {@link SplashScreen} that displays a given {@link Drawable}, which then cross-fades to
+ * Flutter's content.
+ */
+public class DrawableSplashScreen implements SplashScreen {


nit, but I think this still applies.

matthew-carroll · 2019-07-22T21:01:12Z

@xster @mklim I'm gonna go ahead and merge this in. The test PR hasn't been merged yet, but it also technically can't be merged until what it operates on is merged (this PR). On my machine all 3 splash projects run manually as expected, and the one project that is automated passed the last time I tried it.

…lutter-team-archive/engine#9525)

flutter-team-archive/engine@d6970bf...1f30131 git log d6970bf..1f30131 --no-merges --oneline 1f30131 Roll src/third_party/skia 6dc14ded91ea..00c680d2bb7c (10 commits) (flutter-team-archive/engine#10011) de6e82c CopyFiles is still used. (flutter-team-archive/engine#10012) 1c5e5b3 Roll fuchsia/sdk/core/linux-amd64 from tY_fod_tTCLft3FAUUxCP_HdLIahaJJK4WCXOA7nNGQC to M5an7VPM8DiCcNcKe6J0CkAtLk8X9oMeJUqGOrZATIsC (flutter-team-archive/engine#10008) 3aaeca3 Android Embedding Refactor PR36: Add splash screen support. (flutter-team-archive/engine#9525) 268533e [fuchsia] Use GatherArtifacts to create the requisite dir structure (flutter-team-archive/engine#10004) 1961417 Exit flutter_tester with an error code on an unhandled exception (flutter-team-archive/engine#9932) be8819e Declare a copy of the enable_bitcode flag within the Flutter build scripts for use in Fuchsia builds (flutter-team-archive/engine#10003) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

googlebot added cla: yes labels Jun 27, 2019

matthew-carroll requested review from mklim and xster July 1, 2019 20:57

matthew-carroll force-pushed the android_embedding_refactor_pr36_splash_screen_support branch 2 times, most recently from e74d980 to 135df20 Compare July 2, 2019 00:38

xster reviewed Jul 2, 2019

View reviewed changes

matthew-carroll mentioned this pull request Jul 8, 2019

Added demo projects for splash screen support on Android. flutter/flutter#35728

Merged

mklim reviewed Jul 8, 2019

View reviewed changes

matthew-carroll force-pushed the android_embedding_refactor_pr36_splash_screen_support branch 2 times, most recently from 5093386 to 334c8c2 Compare July 16, 2019 07:46

xster reviewed Jul 17, 2019

View reviewed changes

xster reviewed Jul 18, 2019

View reviewed changes

matthew-carroll added 4 commits July 22, 2019 00:41

Android Embedding Refactor PR36: Add splash screen support.

60bc67b

Removed unnecessary logs.

c7a1f41

Updated licenses.

1ef67bb

Use ViewCompat for isAttachedToWindow() on APIs < 19.

4699627

matthew-carroll added 11 commits July 22, 2019 00:41

Changed to normal View constructors for FlutterSplashView.

2fc3ea6

PR Updates.

8d6049e

Added support for old embedding's splash screen behavior.

2db91d3

Updated licenses.

17951f3

Reworked splash implementation to be robust against orientation changes.

7cf327a

PR Upates.

45ad41b

Suppress 'default' method linter warnings.

acebe7d

Changed meta-data splash key to take a drawable and use a new package…

dc95db7

… path.

PR Updates.

370bcd0

PR Updates.

a26a15d

PR Updates.

151b2a4

matthew-carroll force-pushed the android_embedding_refactor_pr36_splash_screen_support branch from 4cf94af to 151b2a4 Compare July 22, 2019 07:42

mklim approved these changes Jul 22, 2019

View reviewed changes

Made a couple classes final.

5d34753

matthew-carroll merged commit 3aaeca3 into flutter-team-archive:master Jul 22, 2019

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

3aaeca3 Android Embedding Refactor PR36: Add splash screen support. (f…

a67a12a

…lutter-team-archive/engine#9525)

engine-flutter-autoroll mentioned this pull request Jul 23, 2019

Roll engine d6970bf36a1e..1f3013163d48 (7 commits) flutter/flutter#36720

Merged

matthew-carroll mentioned this pull request Jul 24, 2019

Create a solution to show a native loading view while a Flutter view renders its first frame flutter/flutter#33494

Closed

Conversation

matthew-carroll commented Jun 27, 2019

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!

xster commented Jul 2, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

matthew-carroll commented Jul 8, 2019

Uh oh!

mklim commented Jul 8, 2019

Uh oh!

matthew-carroll commented Jul 9, 2019

Uh oh!

mklim commented Jul 9, 2019

Uh oh!

matthew-carroll commented Jul 9, 2019

Uh oh!

mklim commented Jul 9, 2019

Uh oh!

matthew-carroll commented Jul 16, 2019

Uh oh!

Uh oh!

Uh oh!

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!

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