Android Embedding PR22: Polish - FlutterActivity Intent factories, FlutterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready. #8317
Conversation
…utterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready.
| ensureFlutterFragmentCreated(); | ||
| } | ||
|
|
||
| private void configureStatusBarForFullscreenFlutterExperience() { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { |
There was a problem hiding this comment.
optional, subjective: Consider using guard clauses to reduce nesting.
https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html
On another note though is it OK that we're not executing anything on older SDKs? Is the UI going to still render correctly?
There was a problem hiding this comment.
I think older Android versions always have a black status bar.
| * @param renderMode render Flutter either as a {@link FlutterView.RenderMode#surface} or a | ||
| * {@link FlutterView.RenderMode#texture}. Use {@code texture} for situations | ||
| * where this {@code FlutterFragment} might have content on top of it, such as | ||
| * a drawer. |
There was a problem hiding this comment.
nit: It's hard to get into the details a ton in just a line or two here but as is this isn't letting people know that using texture most likely comes with a huge performance cost. I'd probably rephrase to something along the lines of "Use {@code texture} when you're willing and able to sacrifice performance to have this {@code FlutterFragment} interleave with other Android views. {@code surface} is a much more performant default, but won't composite with complicated View hierarchies." Exact wording doesn't matter much to me but I think a really rough summary of the pros/cons would be worth it here.
| */ | ||
| @NonNull | ||
| protected FlutterView.RenderMode getRenderMode() { | ||
| String renderModeName = getArguments().getString(ARG_FLUTTERVIEW_RENDER_MODE, null); |
There was a problem hiding this comment.
subjective, nit: If you set the default value here to FlutterView.RenderMode.surface.toString() you could have the next statement consistently return without needing a null check and ternary.
|
@jason-simmons @mklim @mjohnsullivan @tvolkert I just pushed a commit that converts |
…ries, FlutterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready. (flutter/engine#8317)
flutter/engine@3a3f707...7620056 git log 3a3f707..7620056 --no-merges --oneline 7620056 Android Embedding PR22: Polish - FlutterActivity Intent factories, FlutterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready. (flutter/engine#8317) 6bd697d Fix "PointerEvent" flow end event (flutter/engine#8319) 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 (bmparr@google.com), and stop the roller if necessary.
| * Builder to create an {@code Intent} that launches a {@code FlutterActivity} with the | ||
| * desired configuration. | ||
| */ | ||
| public static class IntentBuilder { |
There was a problem hiding this comment.
final
(Google's [internal] "Effective Java" guide, Item 19: "Design and document for inheritance or else prohibit it")
| * | ||
| * @return a new {@link FlutterFragment} | ||
| * To create a {@code FlutterFragment} with default {@code arguments}, invoke {@code build()} | ||
| * immeidately: |
| flutterShellArgs | ||
| ); | ||
| frag.setArguments(args); | ||
| public static class Builder { |
| * to {@link FlutterMain#findAppBundlePath(Context)} | ||
| */ | ||
| @NonNull | ||
| public Builder appBundlePath(@NonNull String appBundlePath) { |
There was a problem hiding this comment.
Any reason not to say this is @Nullable?
I don't feel strongly either way - the argument in favor of @NonNull is that since it defaults to null, there's no reason to call this with a null value. But otoh, it wouldn't hurt either...
| * Any special configuration arguments for the Flutter engine | ||
| */ | ||
| @NonNull | ||
| public Builder flutterShellArgs(@NonNull FlutterShellArgs shellArgs) { |
| * Any special configuration arguments for the Flutter engine | ||
| */ | ||
| @NonNull | ||
| public Builder flutterShellArgs(@NonNull FlutterShellArgs shellArgs) { |
There was a problem hiding this comment.
Side note: since the values we allow for shell args are well defined, I'd consider making the supported args in FlutterShellArgs an enum rather than Strings. Something like:
public final class FlutterShellArgs {
public static final enum Argument {
/**
* <documentation>
*/
TRACE_STARTUP("trace-startup"),
/**
* <documentation>
*/
START_PAUSED("start-paused"),
// More supported values here...
/**
* <documentation>
*/
VERBOSE_LOGGING("verbose-logging");
/**
* The intent key. (more documentation...)
*/
public final String key;
/**
* The flag that's passed to the Flutter shell.
*/
public final String value;
private Argument(String key) {
this.key = key;
value = "--" + key;
}
}
@NonNull
public static FlutterShelArgs fromIntent(@NonNull Intent intent) {
...
if (intent.getBooleanExtra(Argument.TRACE_STARTUP.key, false)) {
args.add(Argument.TRACE_STARTUP.value);
}
...
}
}Also, is there any reason for FlutterShellArgs to be mutable?
…utterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready. (flutter#8317)
…ries, FlutterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready. (flutter#8317)" This reverts commit 75a33f9.
…ries, FlutterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready. (flutter#8317)" This reverts commit 75a33f9.
Android Embedding PR22: Polish - FlutterActivity Intent factories, FlutterFragment control of render modes, FlutterSurfaceView transparent until rendering is ready.