Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm #43690

mossmana · 2023-07-14T17:33:13Z

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.

hellohuanlin

looks clean

hellohuanlin · 2023-07-18T21:14:27Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

@@ -1871,7 +1876,6 @@ - (void)performOrientationUpdate:(UIInterfaceOrientationMask)new_preferences {
        if (!windowScene) {
          // When the view is not loaded, it does not make sense to access the interface
          // orientation, bail.
-          FML_LOG(WARNING) << "Trying to access the window scene before the view is loaded.";


is this intended change?

The log is a bit redundant, since the windowSceneIfViewLoaded function that is called in the line above already logs the message.

Although, I guess there could be a case where the windowScene could be null for other reasons. Maybe rather than delete the line, I should log out the comment instead?

yeah even if both are logged, one is for window scene, and the other is for screen, so not exactly duplicate.

cyanglaz

LGTM

cyanglaz · 2023-07-19T16:54:01Z

shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm

-                       viewFrame:(CGRect)viewFrame
-                  convertedFrame:(CGRect)convertedFrame {
-  OCMStub([viewControllerMock mainScreenIfViewLoaded]).andReturn(UIScreen.mainScreen);
+- (id)setupMockScreen {


nits: setUp
(We do have a lot of misuse of "setup" in our engine code. I think I can create a PR to clean them up)
cc @stuartmorgan

Ah yes, I wasn't even considering the appropriate verb vs. the noun usage when I modeled against existing functions. I'll go ahead and make the change for this test.

Or createMockScreen may be more accurate about what the function is doing

Historically, in the context of a test (not just for iOS), I've usually seen setUp more than create. They can be considered synonyms so, I think I'll go with setUp.

… actual behavior in prep for refactor

…reen.mainScreen

hellohuanlin · 2023-07-19T22:23:10Z

I went through the places you early return if view is not loaded - I think they are all safe to return, since they are all called after the view is loaded, e.g. scroll callbacks.

CC @cyanglaz to double check, especially for add-to-app scenario that I'm not familiar with

hellohuanlin · 2023-07-24T19:28:55Z

Circling back from the triage meeting with chris and stuart - the add-to-app scenario should be fine. i think we are good to land this one.

…roller.mm and FlutterViewControllerTest.mm (flutter/engine#43690)

…131221) flutter/engine@a489c74...ff02fa7 2023-07-24 233583+mossmana@users.noreply.github.com Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm (flutter/engine#43690) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#131221) flutter/engine@a489c74...ff02fa7 2023-07-24 233583+mossmana@users.noreply.github.com Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm (flutter/engine#43690) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the platform-ios label Jul 14, 2023

mossmana changed the title ~~renaming mainScreenIfViewLoaded to screenIfViewLoaded to better match…~~ Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm Jul 14, 2023

mossmana force-pushed the issue128260 branch 6 times, most recently from be2b56d to 4236cdd Compare July 18, 2023 20:27

mossmana marked this pull request as ready for review July 18, 2023 21:00

mossmana requested review from cyanglaz, hellohuanlin and stuartmorgan-g July 18, 2023 21:01

mossmana linked an issue Jul 18, 2023 that may be closed by this pull request

Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm flutter/flutter#128260

Closed

2 tasks

hellohuanlin approved these changes Jul 18, 2023

View reviewed changes

cyanglaz approved these changes Jul 19, 2023

View reviewed changes

mossmana force-pushed the issue128260 branch from 266d211 to 4e0c788 Compare July 19, 2023 18:35

mossmana added 6 commits July 19, 2023 14:19

renaming mainScreenIfViewLoaded to screenIfViewLoaded to better match…

5cd5ccb

… actual behavior in prep for refactor

calling screenIfViewLoaded instead of UIScreen mainScreen

1a8ef92

refactor to make it easier to switch to a test screen instead of UISc…

a9efcdc

…reen.mainScreen

mocking screen instead of using UIScreen main

2a55240

added back improved log message

1ebb401

corrected setup usage to setUp

b9dc437

mossmana force-pushed the issue128260 branch from ea8b25c to b9dc437 Compare July 19, 2023 21:19

Merge branch 'main' into issue128260

54560bb

mossmana merged commit ff02fa7 into flutter:main Jul 24, 2023

mossmana deleted the issue128260 branch July 24, 2023 20:29

engine-flutter-autoroll mentioned this pull request Jul 24, 2023

Roll Flutter Engine from a489c7496268 to ff02fa72acce (1 revision) flutter/flutter#131221

Merged

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

ff02fa72a Replace deprecated [UIScreen mainScreen] in FlutterViewCont…

5688641

…roller.mm and FlutterViewControllerTest.mm (flutter/engine#43690)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm #43690

Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm #43690

Uh oh!

mossmana commented Jul 14, 2023 •

edited

Loading

Uh oh!

hellohuanlin left a comment

Uh oh!

hellohuanlin Jul 18, 2023

Uh oh!

mossmana Jul 18, 2023

Uh oh!

mossmana Jul 18, 2023

Uh oh!

hellohuanlin Jul 18, 2023

Uh oh!

cyanglaz left a comment

Uh oh!

cyanglaz Jul 19, 2023

Uh oh!

mossmana Jul 19, 2023

Uh oh!

hellohuanlin Jul 19, 2023

Uh oh!

mossmana Jul 19, 2023

Uh oh!

hellohuanlin commented Jul 19, 2023

Uh oh!

hellohuanlin commented Jul 24, 2023

Uh oh!

Uh oh!

Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm #43690

Replace deprecated [UIScreen mainScreen] in FlutterViewController.mm and FlutterViewControllerTest.mm #43690

Uh oh!

Conversation

mossmana commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Jul 19, 2023

Uh oh!

hellohuanlin commented Jul 24, 2023

Uh oh!

Uh oh!

mossmana commented Jul 14, 2023 •

edited

Loading