[iOS] Fixes system navigator pop not work when App's root vc is TabBarController #30939

zhongwuzw · 2022-01-20T09:54:56Z

In add-to-app scene, if App's root view controller is TabBarViewController, system navigator pop is not work because window's root view controller is not UINavigationController. We can use engineViewController's navigationController instead.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…rController

flutter-dashboard · 2022-01-20T09:54:58Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dnfield

This LGTM but I would appreciate a review from @jmagman or @cyanglaz to make sure I'm not missing something here. I'm a bit rusty on the iOS world right now :)

jmagman · 2022-01-21T19:04:50Z

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

-    [((UINavigationController*)viewController) popViewControllerAnimated:isAnimated];
+
+  auto engineViewController = static_cast<UIViewController*>([_engine.get() viewController]);
+  UINavigationController* navigationController = [engineViewController navigationController];


Should this fall back on UIApplication.sharedApplication.keyWindow.rootViewController if it's nil to match the old behavior?

Isn't that what it's doing on line 250?

That code path isn't calling popViewControllerAnimated.

Should this fall back on UIApplication.sharedApplication.keyWindow.rootViewController if it's nil to match the old behavior?

Em, I think we can't match the old behavior, because if UIApplication.sharedApplication.keyWindow.rootViewController is UINavigationController but FlutterViewController's navigationController is nil, like FlutterViewController is presented, it would pop the top native view controller but not presented FlutterViewController

Ah, I see, thanks for the explanation.

jmagman

LGTM

jmagman · 2022-01-22T03:23:23Z

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

-    [((UINavigationController*)viewController) popViewControllerAnimated:isAnimated];
+
+  auto engineViewController = static_cast<UIViewController*>([_engine.get() viewController]);
+  UINavigationController* navigationController = [engineViewController navigationController];


Ah, I see, thanks for the explanation.

jmagman · 2022-01-22T03:25:10Z

@gaaclarke do you have any opinions?

google-cla · 2022-01-24T01:56:43Z

☹️ Sorry, but only Googlers may change the label cla: yes.

gaaclarke

Yea, I think this sounds alright. There is just a bit of cleanup needed.

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

…is TabBarController (flutter/engine#30939)

rakeyang · 2022-02-14T09:41:16Z

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

+  UIViewController* engineViewController = [_engine.get() viewController];
+  UINavigationController* navigationController = [engineViewController navigationController];
+  if (navigationController) {
+    [navigationController popViewControllerAnimated:isAnimated];


如果是present出来的UINavigationController而且rootViewController是FlutterViewController，这里是没办法关闭的吧？

Translation:

If it is the presented UINavigationController and the rootViewController is the FlutterViewController, there is no way to close it, right?

@rakeyang If you're seeing a specific problem related to this, please file an issue and we'll investigate. Closed PRs aren't usually monitored.

[iOS] Fixes system navigator pop not work when App's root vc is TabBa…

d2d8b27

…rController

flutter-dashboard bot added platform-ios needs tests labels Jan 20, 2022

Add test

ea828af

zhongwuzw removed the needs tests label Jan 20, 2022

zhongwuzw requested a review from dnfield January 21, 2022 02:14

dnfield reviewed Jan 21, 2022

View reviewed changes

jmagman reviewed Jan 21, 2022

View reviewed changes

jmagman approved these changes Jan 22, 2022

View reviewed changes

zhongwuzw requested a review from gaaclarke January 22, 2022 03:51

zhongwuzw added the cla: yes label Jan 24, 2022

google-cla bot removed the cla: yes label Jan 24, 2022

gaaclarke reviewed Jan 24, 2022

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm Outdated Show resolved Hide resolved

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm Outdated Show resolved Hide resolved

Update review feedback

d19f269

gaaclarke merged commit 54e1f3f into flutter:main Jan 25, 2022

engine-flutter-autoroll mentioned this pull request Jan 25, 2022

Roll Engine from b5162c93f513 to ce06bbe714ab (28 revisions) flutter/flutter#97219

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 25, 2022

54e1f3f [iOS] Fixes system navigator pop not work when App's root vc …

6cebcbd

…is TabBarController (flutter/engine#30939)

engine-flutter-autoroll mentioned this pull request Jan 25, 2022

Roll Engine from b5162c93f513 to cc5852a286bb (29 revisions) flutter/flutter#97229

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 25, 2022

54e1f3f [iOS] Fixes system navigator pop not work when App's root vc …

bfdb2e0

…is TabBarController (flutter/engine#30939)

engine-flutter-autoroll mentioned this pull request Jan 25, 2022

Roll Engine from b5162c93f513 to f48857e14864 (32 revisions) flutter/flutter#97239

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 25, 2022

54e1f3f [iOS] Fixes system navigator pop not work when App's root vc …

3b9fe1d

…is TabBarController (flutter/engine#30939)

engine-flutter-autoroll mentioned this pull request Jan 25, 2022

Roll Engine from b5162c93f513 to 54e1f3f4bb36 (22 revisions) flutter/flutter#97248

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 25, 2022

54e1f3f [iOS] Fixes system navigator pop not work when App's root vc …

4fa46ee

…is TabBarController (flutter/engine#30939)

rakeyang reviewed Feb 14, 2022

View reviewed changes

jmagman mentioned this pull request Apr 7, 2023

Build iOS with -fapplication-extension flag #40972

Closed

8 tasks

[iOS] Fixes system navigator pop not work when App's root vc is TabBarController #30939

[iOS] Fixes system navigator pop not work when App's root vc is TabBarController #30939

Uh oh!

Conversation

zhongwuzw commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 20, 2022

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Jan 22, 2022

Uh oh!

google-cla bot commented Jan 24, 2022

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhongwuzw commented Jan 20, 2022 •

edited

Loading