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

Avoid a never-disappearing splash screen if the engine came from somewhere else on iOS #6834

Merged
merged 6 commits into from
Nov 14, 2018

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Nov 13, 2018

Fixes flutter/flutter#24260

Today, if a user does the following sequence, they'll end up with a blank screen:

FlutterViewController* flutterViewController =
      [[FlutterViewController alloc] initWithEngine:_engine
                                            nibName:nil
                                             bundle:nil];
  flutterViewController.splashScreenView = [[UIView alloc] init];
  [self presentViewController:flutterViewController
                     animated:false
                   completion:nil];

Where _engine is a prewarmed FlutterEngine from somewhere else.

This prevents that.

@dnfield dnfield changed the title Splash engine ios Avoid a never-disappearing splash screen if the engine came from somewhere else on iOS Nov 13, 2018
Copy link
Contributor

@jamesderlin jamesderlin left a comment

Choose a reason for hiding this comment

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

The first-frame callback (__flutterViewRenderedCallback) would also be broken for the case with an existing FlutterEngine.

To clarify: if FlutterViewController is initialized with an existing FlutterEngine, does the splash screen not disappear only if it's been explicitly set, or does the default splash screen loaded from a storyboard/etc. also not disappear? (Based on the code, I assume it's the latter, but the change description implies the former.)

Can you also clarify exactly why the splash screen state machine is broken in this scenario? Is -installSplashScreenCallback not triggered, or is the callback from SetNextFrameCallback not triggered, or do we get stuck somewhere else?

@@ -237,6 +237,11 @@ - (void)loadView {
#pragma mark - Managing launch views

- (void)installSplashScreenViewIfNecessary {
// If the engine wasn't created by this FlutterViewController, we should avoid installing
// a splash screen - otherwise we may never get rid of it.
if (_engineNeedsLaunch == NO) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should prefer if (booleanValue) or if (!booleanValue) instead of comparing against booleans.

@@ -237,6 +237,11 @@ - (void)loadView {
#pragma mark - Managing launch views

- (void)installSplashScreenViewIfNecessary {
// If the engine wasn't created by this FlutterViewController, we should avoid installing
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that ideally we should enable (or disable) the splash screen independently of how the FlutterViewController was initialized. That would be less of a cognitive burden for clients.

If we do make FlutterViewController ignore the splash screen if it's initialized with a FlutterEngine, then we should explicitly document that (and possibly make -setSplashScreen fail an assertion in that case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem seems to be that the the callback is never getting invoked in this case. It might be better to fix that than what I'm proposing here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The callback is getitng invoked after all... Still trying to figure out why I get stuck with a white screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a little progress: installSplashScreenViewIfNecessary gets called before the user gets a chance to actually set the splashscreen. The user then tries to set a splash screen, which is messing something up.

Copy link
Contributor

@jamesderlin jamesderlin left a comment

Choose a reason for hiding this comment

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

Okay. But I think it'd still be good to add a way to check if the engine is rendering, and then we can avoid installing the default splash screen and can fire the first-frame callback immediately when the view is loaded.

@dnfield dnfield merged commit 0d02877 into flutter:master Nov 14, 2018
@dnfield
Copy link
Contributor Author

dnfield commented Nov 14, 2018

/cc @matthew-carroll - we should probably coordinate on this to determine needs for embedding in general

@matthew-carroll
Copy link
Contributor

@dnfield I couldn't quite follow this thread. Can you frame the specific question(s) with regard to what the embedding should do?

@dnfield
Copy link
Contributor Author

dnfield commented Nov 14, 2018

Sorry about that @matthew-carroll , I should have been a bit more explicit

I was mainly trying to draw attention to the last part - it may be valuable for the embedding code to know whether the rasterizer has rendered at least one frame in a synchronous manner. For this particular issue, we could have shortcircuited logic that way. However, if we add that capability it would be good to discuss if/how it might be useful on the Android side as well.

@matthew-carroll
Copy link
Contributor

Sure. Feel free to schedule some time or draft a brief. On Android I believe we have a first frame callback. I thought there was one on iOS, too. We should be able to leverage that any way we'd like.

@dnfield dnfield deleted the splash_engine_ios branch November 14, 2018 05:15
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Nov 14, 2018
flutter/engine@4959b71...520746e

git log 4959b71..520746e --no-merges --oneline
520746e Roll src/third_party/skia 14b9f537c5ee..1e00aebd9a45 (5 commits) (flutter/engine#6848)
97b6293 Add missing pragma directive. (flutter/engine#6847)
0d02877 Avoid a never-disappearing splash screen if the engine came from somewhere else on iOS (flutter/engine#6834)
71ade82 Roll src/third_party/skia abde1adc5f0c..14b9f537c5ee (9 commits) (flutter/engine#6846)
4e89aa2 Roll src/third_party/skia 60b6bc3c2950..abde1adc5f0c (6 commits) (flutter/engine#6845)
6a132f8 Fix Windows Engine Bot (flutter/engine#6844)
e6d6f18 - Roll engine to version f9ebf2129732fd2b606286fdf58e500384b8a0bc (flutter/engine#6839)
889f41f Roll src/third_party/skia 9e3109c99ea5..60b6bc3c2950 (1 commits) (flutter/engine#6843)
a68e214 Roll src/third_party/skia 1e1ba0e0176f..9e3109c99ea5 (1 commits) (flutter/engine#6840)
1174193 Roll src/third_party/skia f04fb3cacbad..1e1ba0e0176f (2 commits) (flutter/engine#6838)
09ef73f Fix code smells reported by chrome's clang plugin (flutter/engine#6833)


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, who should
be CC'd on the roll, and stop the roller if necessary.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants