[ios] Disable/enable related gesture recognizer when disabling/enabling user interactivity. #8304

davidchiles · 2017-03-07T22:17:50Z

Added an example behavior of MGLMapView as a subview of UIScrollView in
iOS app.

mention-bot · 2017-03-07T22:17:57Z

@davidchiles, thanks for your PR! By analyzing this pull request, we identified @incanus, @1ec5 and @boundsj to be potential reviewers.

boundsj · 2017-03-07T23:46:29Z

Thanks! I think this is a good implementation of the suggestion in #7217 (comment) to fully disable gesture recognizers instead of short circuiting their handlers. When they are disabled, then containing view recognizers (like a UIScrollView) work as expected (as illustrated by the new demo in the iosapp in this PR).

The full list is of gesture recognizers affected with this proposal are:

UIPanGestureRecognizer pan handled by handlePanGesture and guarded by isScrollEnabled
UIPanGestureRecognizer twoFingerDrag handled by handleTwoFingerDragGesture and guarded by isZoomEnabled
UIPinchGestureRecognizer pinch handled by handlePinchGesture and guarded by isZoomEnabled
UITapGestureRecognizer doubleTap handled by handleDoubleTapGesture and guarded by isZoomEnabled
UILongPressGestureRecognizer quickZoom handled by handleQuickZoomGesture and guarded by isZoomEnabled
UITapGestureRecognizer twoFingerTap handled by handleTwoFingerTapGesture and guarded by isPitchEnabled
UIRotationGestureRecognizer rotate handled by handleRotateGesture and guarded by isRotateEnabled

All of the handlers noted in this list have guards like if ( ! self.isScrollEnabled) return;. I think we can probably remove those now since the handler methods will never get called since the gesture recognizers are fully disabled.

cc @incanus @friedbunny @1ec5

incanus

Great addition and fixes! Just some critiques and typos I've called out, many of which are minor nitpicks but contribute to overall code quality.

incanus · 2017-03-07T23:53:28Z

platform/ios/app/MBXViewController.m

    MBXSettingsMiscellaneousPrintLogFile,
-    MBXSettingsMiscellaneousDeleteLogFile,
+    MBXSettingsMiscellaneousDeleteLogFile
+


Nix this space.

incanus · 2017-03-07T23:54:06Z

platform/ios/app/MBXViewController.m

    MBXSettingsMiscellaneousPrintLogFile,
-    MBXSettingsMiscellaneousDeleteLogFile,
+    MBXSettingsMiscellaneousDeleteLogFile


Our internal style is to retain commas in these situations to make it easier to rearrange and add new entries.

incanus · 2017-03-07T23:54:13Z

platform/ios/app/MBXViewController.m

@@ -350,6 +353,7 @@ - (void)dismissSettings:(__unused id)sender
                @"Start World Tour",
                [NSString stringWithFormat:@"%@ Custom User Dot", (_customUserLocationAnnnotationEnabled ? @"Disable" : @"Enable")],
                [NSString stringWithFormat:@"%@ Zoom Level", (_showZoomLevelEnabled ? @"Hide" :@"Show")],
+                @"Embeded Map View"


Another trailing comma to add.

incanus · 2017-03-07T23:54:33Z

platform/ios/app/MBXViewController.m

+                    UIStoryboard *storyboard = [UIStoryboard storyboardWithName:@"Main" bundle:nil];
+                    MBXEmbededMapViewController *embeddedMapViewController = (MBXEmbededMapViewController *)[storyboard instantiateViewControllerWithIdentifier:@"MBXEmbededMapViewController"];
+                    [self.navigationController pushViewController:embeddedMapViewController animated:YES];
+


Remove blank line.

incanus · 2017-03-07T23:55:18Z

platform/ios/src/MGLMapView.mm

 @property (nonatomic) UIPanGestureRecognizer *pan;
 @property (nonatomic) UIPinchGestureRecognizer *pinch;
 @property (nonatomic) UIRotationGestureRecognizer *rotate;
+@property (nonatomic) UITapGestureRecognizer *doubleTap;


Keep the taps all together?

incanus · 2017-03-07T23:57:47Z

platform/ios/app/MBXEmbededMapViewController.m

+    self.mapView.transform = CGAffineTransformRotate(rotationGesture.view.transform, rotationGesture.rotation);
+}
+
+- (void)switchContorl:(MBXEmbeddedControl) control {


incanus · 2017-03-07T23:58:02Z

platform/ios/app/MBXEmbededMapViewController.m

+    [self.view addSubview:stackView];
+}
+
+- (void)didSwitch:(UISwitch *)sw {


We always try to write out variable names.

incanus · 2017-03-07T23:58:29Z

platform/ios/app/MBXEmbededMapViewController.m

+
+    //Create list of all the possible control titles
+    NSMutableArray <UIView*>*items = [[NSMutableArray alloc] init];
+    for(NSInteger index =0; index < 4; index++) {


Consistent spacing.

incanus · 2017-03-07T23:59:03Z

platform/ios/app/MBXEmbededMapViewController.m

+};
+
+
+@interface MBXEmbededMapViewController () <UIScrollViewDelegate>


Class name is misspelled: Embeded.

incanus · 2017-03-07T23:59:47Z

platform/ios/app/MBXEmbededMapViewController.h

+//
+//  Created by David Chiles on 3/7/17.
+//  Copyright © 2017 Mapbox. All rights reserved.
+//


Remove comment header & personal identification.

1ec5 · 2017-03-08T10:13:49Z

platform/ios/app/MBXEmbeddedMapViewController.m

+        switchControl.tag = index;
+        [switchControl addTarget:self action:@selector(didSwitch:) forControlEvents:UIControlEventValueChanged];
+        switchControl.translatesAutoresizingMaskIntoConstraints = NO;
+        switchControl.on = [self statusForControl:index];


The body of this for loop seems like a good candidate to break out into a separate method. Considering that there are only four switch controls in all, perhaps it makes sense to set each switch’s title explicitly, track them with separate properties, and hook them up to separate action methods. Someone theoretically adding a “Roll Enabled” switch (!) would have fewer methods to update.

1ec5 · 2017-03-08T10:19:00Z

platform/ios/app/MBXEmbeddedMapViewController.m

+    [scrollView addGestureRecognizer:gestureRecognizer];
+
+    CGRect mapRect = CGRectMake(0, 0, CGRectGetWidth(scrollView.frame) * 2, CGRectGetHeight(scrollView.frame) * 2);
+    self.mapView = [[MGLMapView alloc] initWithFrame:mapRect];


Since the view controller itself is already defined in the storyboard, I wonder if it would make the demo clearer if we moved some of this setup to the storyboard as well. Then most of this class’s code would relate to the switches’ behavior instead of the layout code.

Aye, this is a bit awkward — let’s go one way or the other.

I'm not very fluent in storyboard so I can just remove the view controller from there.

friedbunny · 2017-03-08T18:33:33Z

platform/ios/app/MBXEmbeddedMapViewController.m

+    [scrollView addGestureRecognizer:gestureRecognizer];
+
+    CGRect mapRect = CGRectMake(0, 0, CGRectGetWidth(scrollView.frame) * 2, CGRectGetHeight(scrollView.frame) * 2);
+    self.mapView = [[MGLMapView alloc] initWithFrame:mapRect];


Aye, this is a bit awkward — let’s go one way or the other.

friedbunny · 2017-03-08T18:37:19Z

platform/ios/app/MBXEmbeddedMapViewController.m

+    [self.view addSubview:scrollView];
+    [scrollView addGestureRecognizer:gestureRecognizer];
+
+    CGRect mapRect = CGRectMake(0, 0, CGRectGetWidth(scrollView.frame) * 2, CGRectGetHeight(scrollView.frame) * 2);


I’m not following the purpose of a gigantic map within a scrollview — is this a common use case that we’re demoing? It seems like multiple smaller maps within a regular viewport frame (with only a vertical scroll axis) is a potential use case that would demonstrate these conditionally disabled gestures in a more straightforward way.

It's true that #7217 demonstrated the need for only vertical (and perhaps horizontal) gesture overrides. However, the solution implemented in this PR technically allows for all possible UIScrollView gestures to function instead of the built in map view gestures. This demo may not be realistic but it is comprehensive and I don't think it would make sense to throw away the pinch and rotate parts in iosapp.

That said, a more realistic example in mapbox/ios-sdk-examples would be helpful, I think.

incanus · 2017-03-08T20:58:25Z

platform/ios/app/MBXEmbeddedMapViewController.m

+
+@interface MBXEmbeddedMapViewController () <UIScrollViewDelegate>
+
+@property (nonatomic, strong) MGLMapView *mapView;


Again, super minor, but strong is the default so can be left out.

incanus · 2017-03-08T20:58:59Z

platform/ios/app/MBXEmbeddedMapViewController.m

+{
+    switch (control) {
+        case MBXEmbeddedControlZoom:
+            return @"Zoom Enabled";


Missing break?

A break isn’t needed after a return.

incanus · 2017-03-08T20:59:14Z

platform/ios/app/MBXViewController.m

@@ -350,6 +352,7 @@ - (void)dismissSettings:(__unused id)sender
                @"Start World Tour",
                [NSString stringWithFormat:@"%@ Custom User Dot", (_customUserLocationAnnnotationEnabled ? @"Disable" : @"Enable")],
                [NSString stringWithFormat:@"%@ Zoom Level", (_showZoomLevelEnabled ? @"Hide" :@"Show")],
+                @"Embeded Map View",


incanus · 2017-03-08T20:59:54Z

platform/ios/src/MGLMapView.mm

+    _pitchEnabled = pitchEnabled;
+    self.twoFingerDrag.enabled = pitchEnabled;
+}
+


Love these routines BTW. Great way to see what the setters impact across the view.

🙆‍♂️

boundsj · 2017-03-14T17:29:18Z

Adding this to the 3.6.0 milestone.

boundsj · 2017-04-07T06:41:13Z

2cefd09 makes the changes suggested above. This includes moving all view setup boilerplate to the storyboard so the salient bits of the example are clearer.

cc @incanus @1ec5

1ec5

Thanks!

1ec5 · 2017-04-08T00:01:13Z

platform/ios/app/MBXEmbeddedMapViewController.m

+
+    self.scrollView.delegate = self;
+    self.scrollView.contentSize = CGSizeMake(CGRectGetWidth(self.view.bounds),
+                                             CGRectGetHeight(self.view.bounds));


self.view.bounds.size would have the same effect.

Comments have been addressed. Thanks!

1ec5 added the iOS Mapbox Maps SDK for iOS label Mar 7, 2017

boundsj requested review from incanus, friedbunny and 1ec5 March 7, 2017 23:44

incanus suggested changes Mar 8, 2017

View reviewed changes

davidchiles added a commit to davidchiles/mapbox-gl-native that referenced this pull request Mar 8, 2017

Fixes for mapbox#8304 feedback

7f616c8

friedbunny assigned davidchiles Mar 8, 2017

1ec5 reviewed Mar 8, 2017

View reviewed changes

friedbunny previously requested changes Mar 8, 2017

View reviewed changes

incanus previously requested changes Mar 8, 2017

View reviewed changes

boundsj added this to the ios-v3.6.0 milestone Mar 14, 2017

boundsj assigned boundsj and unassigned davidchiles Mar 21, 2017

boundsj added the bug label Mar 28, 2017

boundsj pushed a commit that referenced this pull request Apr 7, 2017

Fixes for #8304 feedback

a0ee6b5

boundsj force-pushed the 7217-disable-gesture-handlers branch from 96c81e9 to 2cefd09 Compare April 7, 2017 06:39

boundsj requested review from incanus and 1ec5 April 7, 2017 06:42

1ec5 approved these changes Apr 8, 2017

View reviewed changes

boundsj force-pushed the 7217-disable-gesture-handlers branch from 6c7efef to 4999fc0 Compare April 10, 2017 20:08

[ios] Match gesture recognizer with map view user interaction state

96d96d9

boundsj force-pushed the 7217-disable-gesture-handlers branch from 4999fc0 to 96d96d9 Compare April 10, 2017 22:07

boundsj merged commit 10c6f1f into mapbox:master Apr 11, 2017

boundsj mentioned this pull request May 3, 2017

Disabled gestures preempt gesture recognizers in superview #5020

Closed

1ec5 mentioned this pull request Aug 22, 2018

[ios] Camera animation stops when all the map interaction are disabled. #11415

Closed

ahk mentioned this pull request Jun 4, 2019

Line rendering on VideoCore 4 GPUs #7583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Disable/enable related gesture recognizer when disabling/enabling user interactivity. #8304

[ios] Disable/enable related gesture recognizer when disabling/enabling user interactivity. #8304

davidchiles commented Mar 7, 2017

mention-bot commented Mar 7, 2017

boundsj commented Mar 7, 2017

incanus left a comment

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

incanus Mar 7, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

friedbunny Mar 8, 2017

davidchiles Mar 8, 2017

friedbunny Mar 8, 2017

friedbunny Mar 8, 2017 •

edited

Loading

boundsj Mar 8, 2017

incanus Mar 8, 2017

incanus Mar 8, 2017

1ec5 Mar 9, 2017

incanus Mar 8, 2017

incanus Mar 8, 2017

boundsj commented Mar 14, 2017

boundsj commented Apr 7, 2017

1ec5 left a comment

1ec5 Apr 8, 2017

		};


		@interface MBXEmbededMapViewController () <UIScrollViewDelegate>


		@interface MBXEmbeddedMapViewController () <UIScrollViewDelegate>

		@property (nonatomic, strong) MGLMapView *mapView;

[ios] Disable/enable related gesture recognizer when disabling/enabling user interactivity. #8304

[ios] Disable/enable related gesture recognizer when disabling/enabling user interactivity. #8304

Conversation

davidchiles commented Mar 7, 2017

mention-bot commented Mar 7, 2017

boundsj commented Mar 7, 2017

incanus left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny Mar 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj commented Mar 14, 2017

boundsj commented Apr 7, 2017

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny Mar 8, 2017 •

edited

Loading