-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Android Embedding Refactor PR36: Add splash screen support. #9525
Android Embedding Refactor PR36: Add splash screen support. #9525
Conversation
e74d980
to
135df20
Compare
@@ -187,14 +182,56 @@ public Intent build(@NonNull Context context) { | |||
|
|||
@Override | |||
protected void onCreate(@Nullable Bundle savedInstanceState) { | |||
switchLaunchThemeForNormalTheme(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved this with new docs
* 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while the flutter application initially loads right? Otherwise people will ask why I don't need to do this with my normal android app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People do need to do this in their normal Android app.
setTheme(normalThemeRID); | ||
} | ||
} catch (PackageManager.NameNotFoundException exception) { | ||
Log.e(TAG, "Could not read meta-data for FlutterActivity."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not read meta-data for FlutterActivity. Leaving the launch theme in place.
flutterSplashView.setSplashScreen(provideSplashScreen()); | ||
flutterSplashView.setFlutterView(flutterView); | ||
|
||
return flutterSplashView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. This is a neat way of keeping things modular.
@@ -853,4 +872,9 @@ protected void onFirstFrameRendered() {} | |||
*/ | |||
void configureFlutterEngine(@NonNull FlutterEngine flutterEngine); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs?
* an existing {@link FlutterView} if {@code flutterView} is {@code null}. | ||
*/ | ||
public void setFlutterView(@Nullable FlutterView flutterView) { | ||
if (this.flutterView != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 AndroidView
constructor expectations (annoying). FlutterSplashView
should be capable of operating without aSplashScreen
at all so that aFlutterSplashView
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved this with added docs from our last offline chat
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline: also add that it doesn't do anything if no flutter view is set
|
||
/** | ||
* Displays the given {@link FlutterView} in this {@code FlutterSplashView}, or removes | ||
* an existing {@link FlutterView} if {@code flutterView} is {@code null}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the FlutterView hasn't rendered its first frame yet, the splash view will be shown instead.
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
* {@link SplashScreen} that displays a given {@link Drawable}, which then cross-fades to | ||
* Flutter's content. | ||
*/ | ||
public class DrawableSplashScreen implements SplashScreen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but I think this still applies.
shell/platform/android/io/flutter/embedding/android/FlutterFragment.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/android/FlutterSplashView.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/android/SplashScreen.java
Outdated
Show resolved
Hide resolved
@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... |
Pretty sure that it's trying to point to the methods underneath the Javadoc. The
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 |
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 |
@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? |
Kind of subjective, but I'd prefer adding a |
5093386
to
334c8c2
Compare
shell/platform/android/io/flutter/embedding/android/DrawableSplashScreen.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/android/DrawableSplashScreen.java
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/android/DrawableSplashScreen.java
Outdated
Show resolved
Hide resolved
* <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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right because we're applying the windowBackground styling to the flutter activity specifically. Makes sense
@@ -187,14 +182,56 @@ public Intent build(@NonNull Context context) { | |||
|
|||
@Override | |||
protected void onCreate(@Nullable Bundle savedInstanceState) { | |||
switchLaunchThemeForNormalTheme(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved this with new docs
shell/platform/android/io/flutter/embedding/android/FlutterActivity.java
Show resolved
Hide resolved
* an existing {@link FlutterView} if {@code flutterView} is {@code null}. | ||
*/ | ||
public void setFlutterView(@Nullable FlutterView flutterView) { | ||
if (this.flutterView != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved this with added docs from our last offline chat
flutterEngine = null; | ||
|
||
// TODO(mattcarroll): clear the surface when JNI doesn't blow up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is on removed code.
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just call onFirstFrameRendererListener.onFirstFrameRendered here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're suggesting, can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see what you're saying now. Yeah I think we can make that change.
@@ -41,9 +41,18 @@ | |||
private final FlutterJNI flutterJNI; | |||
private final AtomicLong nextTextureId = new AtomicLong(0L); | |||
private RenderSurface renderSurface; | |||
private boolean hasRenderedFirstFrame = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that you're saving this via bundles in the fluttersplashview. Why are you saving it again here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "saving"? Do you mean caching as a variable? It's returned by hasRenderedFirstFrame()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public boolean hasRenderedFirstFrame() { | ||
return hasRenderedFirstFrame; | ||
} | ||
|
||
public void addOnFirstFrameRenderedListener(@NonNull OnFirstFrameRenderedListener listener) { | ||
flutterJNI.addOnFirstFrameRenderedListener(listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
LGTM with tests |
@xster you mentioned tests. Tests here and ready for review: flutter/flutter#35728 |
also cc @gaaclarke |
4cf94af
to
151b2a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nits. Thanks for adding tests in the other PR!
* {@link SplashScreen} that displays a given {@link Drawable}, which then cross-fades to | ||
* Flutter's content. | ||
*/ | ||
public class DrawableSplashScreen implements SplashScreen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but I think this still applies.
shell/platform/android/io/flutter/embedding/android/FlutterSplashView.java
Outdated
Show resolved
Hide resolved
@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. |
flutter/engine@d6970bf...1f30131 git log d6970bf..1f30131 --no-merges --oneline 1f30131 Roll src/third_party/skia 6dc14ded91ea..00c680d2bb7c (10 commits) (flutter/engine#10011) de6e82c CopyFiles is still used. (flutter/engine#10012) 1c5e5b3 Roll fuchsia/sdk/core/linux-amd64 from tY_fod_tTCLft3FAUUxCP_HdLIahaJJK4WCXOA7nNGQC to M5an7VPM8DiCcNcKe6J0CkAtLk8X9oMeJUqGOrZATIsC (flutter/engine#10008) 3aaeca3 Android Embedding Refactor PR36: Add splash screen support. (flutter/engine#9525) 268533e [fuchsia] Use GatherArtifacts to create the requisite dir structure (flutter/engine#10004) 1961417 Exit flutter_tester with an error code on an unhandled exception (flutter/engine#9932) be8819e Declare a copy of the enable_bitcode flag within the Flutter build scripts for use in Fuchsia builds (flutter/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 (aaclarke@google.com), and stop the roller if necessary.
flutter/engine@d6970bf...1f30131 git log d6970bf..1f30131 --no-merges --oneline 1f30131 Roll src/third_party/skia 6dc14ded91ea..00c680d2bb7c (10 commits) (flutter/engine#10011) de6e82c CopyFiles is still used. (flutter/engine#10012) 1c5e5b3 Roll fuchsia/sdk/core/linux-amd64 from tY_fod_tTCLft3FAUUxCP_HdLIahaJJK4WCXOA7nNGQC to M5an7VPM8DiCcNcKe6J0CkAtLk8X9oMeJUqGOrZATIsC (flutter/engine#10008) 3aaeca3 Android Embedding Refactor PR36: Add splash screen support. (flutter/engine#9525) 268533e [fuchsia] Use GatherArtifacts to create the requisite dir structure (flutter/engine#10004) 1961417 Exit flutter_tester with an error code on an unhandled exception (flutter/engine#9932) be8819e Declare a copy of the enable_bitcode flag within the Flutter build scripts for use in Fuchsia builds (flutter/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 (aaclarke@google.com), and stop the roller if necessary.
Android Embedding Refactor PR36: Add splash screen support.