fixes #8782: properly show attribution in modal hierarchies #8837

incanus · 2017-04-27T18:07:52Z

Incorporates @cmoulton's fix from #8782, which we (@jmkiley and I) tested and debugged. We use three of the four code paths in the UIViewController category (all but tab view controller) in the process of finding the true top view controller from which to display the attribution sheet. Even with not using the tab bar controller path, we though it best to leave it in since it makes this method truly complete and generically useful.

Despite being one method, we though it best to start a UIViewController category here as a template for future use and a way to keep MGLMapView.mm trim, and this can indeed be a class method since it doesn't depend on any state.

Also did a little category import re-alphabetization while in there 😉

frederoni · 2017-04-27T20:31:04Z

platform/ios/src/MGLMapView.mm

    }
+    viewController = [UIViewController getTopViewControllerFrom:viewController];


Should this be conditional or always reassign line:1952?

I think it's always if it's indeed a navigation controller, since a navigation controller with out any view controllers doesn't make any sense. Or am I misunderstanding you @frederoni?

Oh nvm, I was unable to parse this. It looks good. 👍

I may be wrong but it seems like you can remove the check for topViewController so maybe this can now be:

UIViewController *viewController = self.window.rootViewController; [[UIViewController getTopViewControllerFrom:viewController] presentViewController:attributionController animated:YES completion:NULL];

The category seems like it'll work out the visibleViewController (modal or otherwise).

If that works and depending on what you decide to do about https://github.com/mapbox/mapbox-gl-native/pull/8837/files#r113819069, maybe this could end up like:

[[self.window.rootViewController topMostViewController] presentViewController:attributionController animated:YES completion:NULL];

Sorry, missed this in between some work on 4903db6 @boundsj. Basically, that's what I was already working on 😄.

frederoni · 2017-04-27T20:41:40Z

platform/ios/src/UIViewController+MGLAdditions.h

+
+@interface UIViewController (MGLAdditions)
+
+ (UIViewController *)getTopViewControllerFrom:(UIViewController *)viewController;


Nit: Prefixed method names in categories on framework classes.
~~Btw, do we need the viewController arg when we have access to self?~~

+[UIViewController(MGLAdditions) topViewController] would conflict with -[UINavigationViewController topViewController] but how about +[UIViewController(MGLAdditions) mgl_topMostViewController]?

-edited-
Just noticed 🙈

and this can indeed be a class method since it doesn't depend on any state.

However, get in method names are only used when returning objects or values indirectly.

Good point re: get. Will fix. This came from the user's suggestion. @jmkiley and I discussed this method name a bit and figured we'd get some more input first.

1ec5 · 2017-04-27T22:43:21Z

platform/ios/src/UIViewController+MGLAdditions.m

+
+@implementation UIViewController (MGLAdditions)
+
+ (UIViewController *)getTopViewControllerFrom:(UIViewController *)viewController


this can indeed be a class method since it doesn't depend on any state

That’s true as far as the current implementation goes. However, it would be possible to express this method in a different way, so that a UIViewController knows how to return its topmost containing view controller: -topmostViewController or perhaps -rootViewController.

incanus · 2017-04-28T00:46:15Z

In 4903db6 I get rid of the explicit inline check for topViewController and just rely on the category method's visibleViewController, which should capture this. Per the docs (emphasis mine):

The currently visible view can belong either to the view controller at the top of the navigation stack or to a view controller that was presented modally on top of the navigation controller itself.

1ec5 · 2017-04-28T04:17:32Z

platform/ios/src/UIViewController+MGLAdditions.h

+
+@interface UIViewController (MGLAdditions)
+
+ (UIViewController *)mgl_topMostViewControllerInHierarchyWith:(UIViewController *)viewController;


In Objective-C, a selector piece (mgl_topMostViewControllerInHierarchyWith:) should always end with a noun, not a preposition.

This should be an instance method or read-only property named mgl_topmostViewController. The return value is related to viewController, not the UIViewController class as a whole. One might expect this class method, as written, to be a factory method for creating UIViewControllers.

I would also prefer this to be an instance method to support being able to call it like in #8837 (comment). Otherwise I think this is 👍

boundsj · 2017-04-28T17:21:26Z

@incanus just noticed that this is against master but in the ios-v3.5.3 milestone. Please plan to cherry pick to the release-ios-v3.5.0-android-v5.0.0 or retarget this PR (in general, we push commits to the release branch and then merge it back to master rather than cherry picking but it does not really matter in this case)

1ec5 · 2017-05-01T23:54:31Z

platform/ios/src/UIViewController+MGLAdditions.m

+{
+    if ([viewController isKindOfClass:[UINavigationController class]])
+    {
+        return [self mgl_topMostViewControllerInHierarchyWith:[(UINavigationController *)viewController visibleViewController]];


Instead of passing in visibleViewController, call mgl_topMostViewController on visibleViewController. This method doesn't need to take any parameters.

jmkiley · 2017-05-02T19:56:33Z

I think I addressed @1ec5's feedback in 8e17f5a, but please let me know if I missed anything!

1ec5

Thanks for iterating – this looks great!

1ec5 · 2017-05-02T19:53:28Z

platform/ios/src/UIViewController+MGLAdditions.m

+    }
+    else if ([self isKindOfClass:[UITabBarController class]])
+    {
+        return [[((UITabBarController *)self) selectedViewController] mgl_topMostViewController];


Nit: extraneous parentheses around self.

Thanks! Just fixed.

fixes #8782: properly show attribution in modal hierarchies

8082ec7

incanus added this to the ios-v3.5.3 milestone Apr 27, 2017

incanus assigned incanus and jmkiley Apr 27, 2017

incanus requested review from boundsj, friedbunny and 1ec5 April 27, 2017 18:07

frederoni reviewed Apr 27, 2017

View reviewed changes

safer method name

fead41e

1ec5 reviewed Apr 27, 2017

View reviewed changes

refinements to nav controller inspection

4903db6

1ec5 reviewed Apr 28, 2017

View reviewed changes

tobrun added the iOS Mapbox Maps SDK for iOS label Apr 28, 2017

jmkiley force-pushed the 8782-modal-attribution branch from f718d4d to 4903db6 Compare May 1, 2017 19:13

[ios] changed to instance method

d417e84

1ec5 suggested changes May 1, 2017

View reviewed changes

[ios] combined into one method

8e17f5a

jmkiley force-pushed the 8782-modal-attribution branch from 824d269 to 8e17f5a Compare May 2, 2017 19:02

boundsj approved these changes May 2, 2017

View reviewed changes

1ec5 approved these changes May 2, 2017

View reviewed changes

jmkiley and others added 2 commits May 2, 2017 13:05

[ios] removed extra parenthesis

42403b1

Merge branch 'master' into 8782-modal-attribution

a051a58

jmkiley merged commit e5ae80c into master May 2, 2017

jmkiley deleted the 8782-modal-attribution branch May 2, 2017 21:08

jmkiley mentioned this pull request May 2, 2017

[ios] cherry-picked 8837 #8869

Merged

jmkiley mentioned this pull request May 17, 2017

[ios] Telemetry button in attribution UIAlertController doesn't present telemetry UIAlertController from modal view controller #8980

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #8782: properly show attribution in modal hierarchies #8837

fixes #8782: properly show attribution in modal hierarchies #8837

incanus commented Apr 27, 2017 •

edited

Loading

frederoni Apr 27, 2017

incanus Apr 27, 2017

frederoni Apr 27, 2017

boundsj Apr 28, 2017

incanus Apr 28, 2017

frederoni Apr 27, 2017 •

edited

Loading

incanus Apr 27, 2017

incanus Apr 28, 2017

1ec5 Apr 27, 2017

incanus commented Apr 28, 2017

1ec5 Apr 28, 2017

boundsj Apr 28, 2017

boundsj commented Apr 28, 2017

1ec5 May 1, 2017

jmkiley commented May 2, 2017

1ec5 left a comment

1ec5 May 2, 2017

jmkiley May 2, 2017

		}
		viewController = [UIViewController getTopViewControllerFrom:viewController];


		@interface UIViewController (MGLAdditions)

		+ (UIViewController )getTopViewControllerFrom:(UIViewController )viewController;


		@implementation UIViewController (MGLAdditions)

		+ (UIViewController )getTopViewControllerFrom:(UIViewController )viewController


		@interface UIViewController (MGLAdditions)

		+ (UIViewController )mgl_topMostViewControllerInHierarchyWith:(UIViewController )viewController;

fixes #8782: properly show attribution in modal hierarchies #8837

fixes #8782: properly show attribution in modal hierarchies #8837

Conversation

incanus commented Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj commented Apr 28, 2017

Choose a reason for hiding this comment

jmkiley commented May 2, 2017

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Apr 27, 2017 •

edited

Loading

frederoni Apr 27, 2017 •

edited

Loading