SystemNavigator.pop can pop w/o UINavigationController #6341

dnfield · 2018-09-26T05:07:34Z

Today, iOS apps in Flutter ignore System.pop unless they find that the rootViewController is a UINavigationViewController.

For Add2App, it's entirely possible someone will show the view using something like showDetailViewController from UIViewController. This patch updates the logic to handle that scenario, but to do so the FlutterPlatformPlugin needs to get access to the FlutterViewController, and to do that I added functionality there for FlutterViewController to create WeakPtrs.

I'd be very happy if it turns out there's a way to stack allocate the WeakPtrFactory (or equivalent functionality) in Objective C++.

/cc @matthew-carroll - It might be worth investigating how SystemNavigator.pop is working in Android's Add2App use cases

Fixes part of flutter/flutter#22054

dnfield · 2018-09-26T22:14:27Z

Also would be happy if there's a more idiomatic Objective C++ way to pass a weak reference to another object

chinmaygarde · 2018-09-27T00:03:48Z

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

 @interface FlutterPlatformPlugin : NSObject

+- (instancetype)initWithViewController:(fml::WeakPtr<UIViewController>)vc;


Is this a designated initializer? If so, please decorated this with NS_DESIGNATED_INITIALIZER.

chinmaygarde · 2018-09-27T00:05:12Z

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

@@ -31,7 +32,20 @@

 using namespace shell;

-@implementation FlutterPlatformPlugin
+@implementation FlutterPlatformPlugin {
+  fml::WeakPtr<UIViewController> _vc;


Nit: We prefer not to abbreviate instance variable names in code. _viewController works better IMO.

chinmaygarde · 2018-09-27T00:07:43Z

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

  UIViewController* viewController = [UIApplication sharedApplication].keyWindow.rootViewController;
  if ([viewController isKindOfClass:[UINavigationController class]]) {
    [((UINavigationController*)viewController) popViewControllerAnimated:NO];
+  } else if (_vc && viewController != _vc.get()) {


The second check is redundant.

chinmaygarde · 2018-09-27T00:10:11Z

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

@@ -34,6 +35,7 @@ @implementation FlutterViewController {
  fml::scoped_nsobject<FlutterDartProject> _dartProject;
  shell::ThreadHost _threadHost;
  std::unique_ptr<shell::Shell> _shell;
+  fml::WeakPtrFactory<FlutterViewController>* _weakFactory;


We really try to avoid raw pointers. Please consider using a unique pointer. I realize the ObjC++ interop makes this a bit awkward.

chinmaygarde

Minor nit but looks good otherwise. Thanks!

chinmaygarde · 2018-09-27T04:34:49Z

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

@@ -65,6 +67,8 @@ - (instancetype)initWithProject:(FlutterDartProject*)projectOrNil
                         bundle:(NSBundle*)nibBundleOrNil {
  self = [super initWithNibName:nibNameOrNil bundle:nibBundleOrNil];
  if (self) {
+    _weakFactory = std::unique_ptr<fml::WeakPtrFactory<FlutterViewController>>(


Nit: std::make_unique<fml::WeakPtrFactory<FlutterViewController>>(self) is used in the engine for construction of unique pointer.

chinmaygarde · 2018-09-27T04:38:51Z

The CI bot failure is because of clang-format. I recommend wiring up your editor to format on save.

dnfield · 2018-09-27T04:39:51Z

I was having issues with depot_tools clang-format not working, and I thought I fixed it but now it doesn't seem to get the right clang format rules... I'll fix that though. Thanks!

flutter/engine@38a646e...3052dbd git log 38a646e..3052dbd --no-merges --oneline 3052dbd SystemNavigator.pop can pop w/o UINavigationController (flutter/engine#6341) 0c096f7 Roll src/third_party/skia b3e48afc936d..227d4e10276c (1 commits) (flutter/engine#6359) b8c2a17 Roll src/third_party/skia cfe1264d7465..b3e48afc936d (3 commits) (flutter/engine#6356) c589b31 Expose push/popRoute on FlutterViewController (flutter/engine#6347) 075b3fc Roll src/third_party/skia 5ea41fc89b26..cfe1264d7465 (1 commits) (flutter/engine#6355) 2dd9b99 Roll Dart to version 808ed6238b9262660e31ea826f7aea6cfa3a3493 (flutter/engine#6354) 5b79938 Dont make any binaries specify an X11 dependency. (flutter/engine#6353) 309ac4e V0.8.2 fix compile problem with xcode10 (flutter/engine#6339) 26fdd1e Roll src/third_party/skia 5767fc042834..5ea41fc89b26 (3 commits) (flutter/engine#6351) cc44ca5 Perform persistent cache stores on the IO thread outside the frame workload. (flutter/engine#6350) f2a3df9 Wire up the Skia persistent GPU related artifacts cache. (flutter/engine#6278) 9d4b80a Roll src/third_party/skia 17282da3aa94..5767fc042834 (8 commits) (flutter/engine#6348) ef98dcb Add support for counter timeline traces from the engine. (flutter/engine#6315) a961e96 Roll buildroot to pick up updated vs toolchain. (flutter/engine#6346) d6bb599 Fixed IsolateNameServer documentation (flutter/engine#6344) 0c854ac Roll src/third_party/skia bdf1431686c2..17282da3aa94 (4 commits) (flutter/engine#6345) e394ea1 Roll src/third_party/skia c05302bc84da..bdf1431686c2 (1 commits) (flutter/engine#6343) bcdfc23 Roll src/third_party/skia 5457141b011e..c05302bc84da (1 commits) (flutter/engine#6342) a861f37 Pass on the new unsafe-package-serialization option (flutter/engine#6331) 5e77a1e Roll src/third_party/skia bd03b54cb98d..5457141b011e (1 commits) (flutter/engine#6340) 6332db8 Roll src/third_party/skia f18c297cfb74..bd03b54cb98d (1 commits) (flutter/engine#6338) a08bc52 Roll src/third_party/skia 7ffa40cedbc4..f18c297cfb74 (6 commits) (flutter/engine#6336) 8247ce2 Implement restore functions on Android and iOS (flutter/engine#6322) 2ead5c0 Roll src/third_party/skia 4ef464cd3c2e..7ffa40cedbc4 (10 commits) (flutter/engine#6334) 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.

flutter/engine@9d4b80a...5ae4708 git log 9d4b80a..5ae4708 --no-merges --oneline 5ae4708 Roll src/third_party/skia 227d4e10276c..ab3144c3abb9 (11 commits) (flutter/engine#6360) 763627f Do not export libdart symbols (flutter/engine#6337) 3052dbd SystemNavigator.pop can pop w/o UINavigationController (flutter/engine#6341) 0c096f7 Roll src/third_party/skia b3e48afc936d..227d4e10276c (1 commits) (flutter/engine#6359) b8c2a17 Roll src/third_party/skia cfe1264d7465..b3e48afc936d (3 commits) (flutter/engine#6356) c589b31 Expose push/popRoute on FlutterViewController (flutter/engine#6347) 075b3fc Roll src/third_party/skia 5ea41fc89b26..cfe1264d7465 (1 commits) (flutter/engine#6355) 2dd9b99 Roll Dart to version 808ed6238b9262660e31ea826f7aea6cfa3a3493 (flutter/engine#6354) 5b79938 Dont make any binaries specify an X11 dependency. (flutter/engine#6353) 309ac4e V0.8.2 fix compile problem with xcode10 (flutter/engine#6339) 26fdd1e Roll src/third_party/skia 5767fc042834..5ea41fc89b26 (3 commits) (flutter/engine#6351) cc44ca5 Perform persistent cache stores on the IO thread outside the frame workload. (flutter/engine#6350) f2a3df9 Wire up the Skia persistent GPU related artifacts cache. (flutter/engine#6278) 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.

flutter/engine@9d4b80a...3a01f39 git log 9d4b80a..3a01f39 --no-merges --oneline 3a01f39 Change log level from ERROR to WARNING (flutter/engine#6361) 5ae4708 Roll src/third_party/skia 227d4e10276c..ab3144c3abb9 (11 commits) (flutter/engine#6360) 763627f Do not export libdart symbols (flutter/engine#6337) 3052dbd SystemNavigator.pop can pop w/o UINavigationController (flutter/engine#6341) 0c096f7 Roll src/third_party/skia b3e48afc936d..227d4e10276c (1 commits) (flutter/engine#6359) b8c2a17 Roll src/third_party/skia cfe1264d7465..b3e48afc936d (3 commits) (flutter/engine#6356) c589b31 Expose push/popRoute on FlutterViewController (flutter/engine#6347) 075b3fc Roll src/third_party/skia 5ea41fc89b26..cfe1264d7465 (1 commits) (flutter/engine#6355) 2dd9b99 Roll Dart to version 808ed6238b9262660e31ea826f7aea6cfa3a3493 (flutter/engine#6354) 5b79938 Dont make any binaries specify an X11 dependency. (flutter/engine#6353) 309ac4e V0.8.2 fix compile problem with xcode10 (flutter/engine#6339) 26fdd1e Roll src/third_party/skia 5767fc042834..5ea41fc89b26 (3 commits) (flutter/engine#6351) cc44ca5 Perform persistent cache stores on the IO thread outside the frame workload. (flutter/engine#6350) f2a3df9 Wire up the Skia persistent GPU related artifacts cache. (flutter/engine#6278) 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.

flutter/engine@9d4b80a...edf6249 git log 9d4b80a..edf6249 --no-merges --oneline edf6249 Add pushOffset to SceneBuilder (flutter/engine#6349) 3a01f39 Change log level from ERROR to WARNING (flutter/engine#6361) 5ae4708 Roll src/third_party/skia 227d4e10276c..ab3144c3abb9 (11 commits) (flutter/engine#6360) 763627f Do not export libdart symbols (flutter/engine#6337) 3052dbd SystemNavigator.pop can pop w/o UINavigationController (flutter/engine#6341) 0c096f7 Roll src/third_party/skia b3e48afc936d..227d4e10276c (1 commits) (flutter/engine#6359) b8c2a17 Roll src/third_party/skia cfe1264d7465..b3e48afc936d (3 commits) (flutter/engine#6356) c589b31 Expose push/popRoute on FlutterViewController (flutter/engine#6347) 075b3fc Roll src/third_party/skia 5ea41fc89b26..cfe1264d7465 (1 commits) (flutter/engine#6355) 2dd9b99 Roll Dart to version 808ed6238b9262660e31ea826f7aea6cfa3a3493 (flutter/engine#6354) 5b79938 Dont make any binaries specify an X11 dependency. (flutter/engine#6353) 309ac4e V0.8.2 fix compile problem with xcode10 (flutter/engine#6339) 26fdd1e Roll src/third_party/skia 5767fc042834..5ea41fc89b26 (3 commits) (flutter/engine#6351) cc44ca5 Perform persistent cache stores on the IO thread outside the frame workload. (flutter/engine#6350) f2a3df9 Wire up the Skia persistent GPU related artifacts cache. (flutter/engine#6278) 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.

flutter/engine@9d4b80a...d80c1de git log 9d4b80a..d80c1de --no-merges --oneline d80c1de Roll src/third_party/skia ab3144c3abb9..656cefe65d62 (11 commits) (flutter/engine#6362) edf6249 Add pushOffset to SceneBuilder (flutter/engine#6349) 3a01f39 Change log level from ERROR to WARNING (flutter/engine#6361) 5ae4708 Roll src/third_party/skia 227d4e10276c..ab3144c3abb9 (11 commits) (flutter/engine#6360) 763627f Do not export libdart symbols (flutter/engine#6337) 3052dbd SystemNavigator.pop can pop w/o UINavigationController (flutter/engine#6341) 0c096f7 Roll src/third_party/skia b3e48afc936d..227d4e10276c (1 commits) (flutter/engine#6359) b8c2a17 Roll src/third_party/skia cfe1264d7465..b3e48afc936d (3 commits) (flutter/engine#6356) c589b31 Expose push/popRoute on FlutterViewController (flutter/engine#6347) 075b3fc Roll src/third_party/skia 5ea41fc89b26..cfe1264d7465 (1 commits) (flutter/engine#6355) 2dd9b99 Roll Dart to version 808ed6238b9262660e31ea826f7aea6cfa3a3493 (flutter/engine#6354) 5b79938 Dont make any binaries specify an X11 dependency. (flutter/engine#6353) 309ac4e V0.8.2 fix compile problem with xcode10 (flutter/engine#6339) 26fdd1e Roll src/third_party/skia 5767fc042834..5ea41fc89b26 (3 commits) (flutter/engine#6351) cc44ca5 Perform persistent cache stores on the IO thread outside the frame workload. (flutter/engine#6350) f2a3df9 Wire up the Skia persistent GPU related artifacts cache. (flutter/engine#6278) 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.

* SystemNavigator.pop can pop w/o UINavigationController

SystemNavigator.pop can pop w/o UINavigationController

717593a

dnfield requested review from chinmaygarde and cbracken September 26, 2018 05:07

googlebot added the cla: yes label Sep 26, 2018

format

2778644

This was referenced Sep 26, 2018

FlutterViewController needs to be able to communicate about navigation flutter/flutter#22054

Closed

Expose push/popRoute on FlutterViewController #6347

Merged

chinmaygarde reviewed Sep 27, 2018

View reviewed changes

Use unique_ptr, NS_DESIGNATED_INITIALIZER, nits

4446b32

chinmaygarde approved these changes Sep 27, 2018

View reviewed changes

make_unique, format

3514706

format

a3523b0

dnfield merged commit 3052dbd into flutter:master Sep 27, 2018

engine-flutter-autoroll mentioned this pull request Sep 27, 2018

Roll engine 38a646e14cc2..3052dbd79bdc (24 commits) flutter/flutter#22357

Closed

This was referenced Sep 27, 2018

Roll engine 9d4b80a60cb5..5ae47084524a (13 commits) flutter/flutter#22364

Closed

Roll engine 9d4b80a60cb5..3a01f395685a (14 commits) flutter/flutter#22365

Closed

engine-flutter-autoroll mentioned this pull request Sep 27, 2018

Roll engine 9d4b80a60cb5..edf6249e07bb (15 commits) flutter/flutter#22367

Closed

engine-flutter-autoroll mentioned this pull request Sep 27, 2018

Roll engine 9d4b80a60cb5..d80c1de7b53a (16 commits) flutter/flutter#22374

Closed

GaryQian pushed a commit to GaryQian/engine that referenced this pull request Oct 2, 2018

SystemNavigator.pop can pop w/o UINavigationController (flutter#6341)

1e4d1ba

* SystemNavigator.pop can pop w/o UINavigationController

dnfield deleted the system_pop_ios branch November 14, 2018 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SystemNavigator.pop can pop w/o UINavigationController #6341

SystemNavigator.pop can pop w/o UINavigationController #6341

Uh oh!

dnfield commented Sep 26, 2018 •

edited

Loading

Uh oh!

dnfield commented Sep 26, 2018

Uh oh!

chinmaygarde Sep 27, 2018

Uh oh!

chinmaygarde Sep 27, 2018

Uh oh!

chinmaygarde Sep 27, 2018

Uh oh!

chinmaygarde Sep 27, 2018

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Sep 27, 2018

Uh oh!

chinmaygarde commented Sep 27, 2018

Uh oh!

dnfield commented Sep 27, 2018

Uh oh!

Uh oh!

		@interface FlutterPlatformPlugin : NSObject

		- (instancetype)initWithViewController:(fml::WeakPtr<UIViewController>)vc;

SystemNavigator.pop can pop w/o UINavigationController #6341

SystemNavigator.pop can pop w/o UINavigationController #6341

Uh oh!

Conversation

dnfield commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Sep 26, 2018

Uh oh!

chinmaygarde Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Sep 27, 2018

Uh oh!

dnfield commented Sep 27, 2018

Uh oh!

Uh oh!

dnfield commented Sep 26, 2018 •

edited

Loading