Add enabled property to MGLShape #12352

jmkiley · 2018-07-10T00:12:50Z

To Do:

Add enabled property to MGLShape.
~~- [x] Account for MGLShapeCollection, which does not inherit from MGLMultiPoint.~~ This is not necessary.
Document that the enabled property only applies to annotations, and not style layers.
Add tests.

jmkiley · 2018-07-13T18:07:52Z

I am still working on the tests. The challenge I'm currently running into is with simulating a tap, since the annotations can still be selected programmatically.

For the implementation, I just used the same approach we currently take with MGLAnnotationView and MGLAnnotationImage.

cc @fabian-guerra @julianrex @1ec5

jmkiley · 2018-07-13T19:26:32Z

In order to test tapping on an annotation, I will likely need to add UI Testing to either the Integration Test Harness or to iosapp.

cc @friedbunny

jmkiley · 2018-07-13T19:46:06Z

Upon reading the discussion in #12313, I'm going to remove the attempts at UI tests in this PR and add a test for this PR to ios-sdk-examples.

fabian-guerra

With this change we will have the following methods that can be used to "enable" annotations callbacks:

Post tap processing- Shape and View based annotations:

[MGLMapViewDelegate mapView:didSelectAnnotation:].
-[MGLMapViewDelegate mapView:annotationCanShowCallout:] .

Tap processing:

-[MGLAnnotationImage isEnabled].
-[MGLShape isEnabled] Shape and PointAnnotations.

Should we document the appropriate usage for each? Or should we reduce the options? Can we change the level of granularity to enable all or non shape/view based annotations?

fabian-guerra · 2018-07-13T21:50:15Z

platform/ios/src/MGLMapView.mm

@@ -4091,6 +4091,10 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
            {
                if ([annotation isKindOfClass:[MGLMultiPoint class]])
                {
+                    if ( ! ((MGLShape *)annotation).enabled) {


MGLPointAnnotation is a form of MGLShape annotations if we set the enable property to NO on a point annotation I would expect that is going to be disabled. This line is never reached in that case.

Thanks for pointing that out!

jmkiley · 2018-07-13T23:48:18Z

I'm going to reevaluate where I'm doing the check. There may be issues with cycling through MGLAnnotationImage objects with the current approach.

Also, I should allow the MGLPointAnnotation's enabled status to pass on to the subsequent view/image.

julianrex · 2018-07-14T00:02:30Z

platform/darwin/src/MGLMultiPoint.mm

@@ -20,6 +20,7 @@ - (instancetype)initWithCoordinates:(const CLLocationCoordinate2D *)coords count
            [NSException raise:NSInvalidArgumentException
                        format:@"A multipoint must have at least one vertex."];
        }
+        self.enabled = YES;


Since you're in an init, this would be better as _enabled = YES;

julianrex · 2018-07-14T00:13:24Z

platform/darwin/src/MGLShape.h

+ `NO`, the `MGLShape` object ignores touch events and cannot be selected.
+ */
+@property (nonatomic, getter=isEnabled) BOOL enabled;
+


Discussion point for later: I wonder if this property could be part of a protocol that MGLShape, MGLAnnotationImage, MGLAnnotationView should conform to?

That could be interesting. MGLAnnotation covers two of the three (but not MGLAnnotationImage as far as I know).

It’s important to distinguish between data and presentation: MGLShape, MGLAnnotation, etc. are all about data storage, whereas MGLAnnotationView and MGLAnnotationImage are ways to render that data. The same distinction can be made between MGLSource and MGLStyleLayer.

MGLAnnotationView and MGLAnnotationImage both have a existing enabled properties. How will the property being added in this PR interact with the existing ones? What if an annotation’s enabled property disagrees with its view or image’s enabled property at the time the user taps it?

In #10539 (comment), I suggested that MGLAnnotationImage.enabled property be deprecated, but to me a property on MGLShape is worse because an aspect of the data’s presentation is defined on the data itself. The problem with MGLAnnotationImage.enabled is not that it’s on the wrong kind of class but that there’s no way to change the annotation image’s interactivity after it appears on the map for the first time. MGLAnnotationView.enabled doesn’t suffer from this problem, because you can call -[MGLMapView viewForAnnotation:] at any time.

julianrex · 2018-07-14T00:18:31Z

platform/ios/src/MGLMapView.mm

@@ -4089,8 +4089,12 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
            }
            else
            {
-                if ([annotation isKindOfClass:[MGLMultiPoint class]])
+                if ([annotation conformsToProtocol:[MGLShape self]])


MGLShape isn't a protocol (unless there's also a protocol with the same name?) - so this check should be a isKindOfClass test.

However, this relates to my question above - perhaps it is worth having an "interactable annotation" protocol after all?

Ack, I was mixing up MGLShape and MGLAnnotation, thank you

julianrex · 2018-07-16T14:46:35Z

This PR needs to target master (and then be cherry-picked if we want it in the 4.2.0 release)

1ec5

Below are some concerns about the direction these changes take the SDK. It seems to me that there must be other avenues to explore. If it would be burdensome for the developer to track the selected annotation themselves in order to achieve custom behavior, then perhaps we could make MGLMapView’s selection functionality more customizable via MGLMapViewDelegate methods. But before we go down that route, I think we should confirm whether -[MGLMapViewDelegate mapView:annotationCanShowCallout:, as described in #10539 (comment), addresses the present need without adding to the public API.

1ec5 · 2018-07-18T06:08:00Z

platform/darwin/src/MGLShape.h

+ The default value of this property is `YES`. If the value of this property is
+ `NO`, the `MGLShape` object ignores touch events and cannot be selected.
+ */
+@property (nonatomic, getter=isEnabled) BOOL enabled;


Implementing this property on MGLShape prevents the developer from opting custom MGLAnnotation classes out of selection and interactivity. At the same time, it means features that are part of a shape source or feature querying result have a meaningless enabled property.

1ec5 · 2018-07-18T06:09:13Z

platform/darwin/src/MGLShape.h

+ `NO`, the `MGLShape` object ignores touch events and cannot be selected.
+ */
+@property (nonatomic, getter=isEnabled) BOOL enabled;
+


It’s important to distinguish between data and presentation: MGLShape, MGLAnnotation, etc. are all about data storage, whereas MGLAnnotationView and MGLAnnotationImage are ways to render that data. The same distinction can be made between MGLSource and MGLStyleLayer.

MGLAnnotationView and MGLAnnotationImage both have a existing enabled properties. How will the property being added in this PR interact with the existing ones? What if an annotation’s enabled property disagrees with its view or image’s enabled property at the time the user taps it?

In #10539 (comment), I suggested that MGLAnnotationImage.enabled property be deprecated, but to me a property on MGLShape is worse because an aspect of the data’s presentation is defined on the data itself. The problem with MGLAnnotationImage.enabled is not that it’s on the wrong kind of class but that there’s no way to change the annotation image’s interactivity after it appears on the map for the first time. MGLAnnotationView.enabled doesn’t suffer from this problem, because you can call -[MGLMapView viewForAnnotation:] at any time.

1ec5 · 2018-07-18T06:21:48Z

platform/darwin/src/MGLShape.mm

@@ -72,6 +73,7 @@ - (void)encodeWithCoder:(NSCoder *)coder
 {
    [coder encodeObject:_title forKey:@"title"];
    [coder encodeObject:_subtitle forKey:@"subtitle"];
+    [coder encodeBool:_enabled forKey:@"enabled"];


If two MGLShape objects are identical in every way except that one is enabled and one isn’t, the two are considered object-equal according to this PR. Is that correct, or should object equality also consider enabled state? Philosophically, a shape’s coordinates – its form – is intrinsic to the shape. Could the same be said of its interactivity? If so, what about the feature’s cursor (which is currently implemented on macOS as MGLAnnotationImage.cursor)?

fabian-guerra · 2018-08-15T18:35:40Z

Changed the previous implementation to what is required in this comment: #10539 (comment)

1ec5 · 2018-08-15T20:23:35Z

platform/ios/CHANGELOG.md

@@ -8,6 +8,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
 * When a symbol in an `MGLSymbolStyleLayer` has both an icon and text, both are shown or hidden together based on available space. ([#12521](https://github.com/mapbox/mapbox-gl-native/pull/12521))
 * The `-[MGLMapView visibleFeaturesAtPoint:]` method can now return features near tile boundaries at high zoom levels. ([#12570](https://github.com/mapbox/mapbox-gl-native/pull/12570))
 * Fixed inconsistencies in exception naming. ([#12583](https://github.com/mapbox/mapbox-gl-native/issues/12583))
+* `-[MGLMapViewDelegate mapView:canSelectAnnotation:]` can now specify if an annotation is selectable. ([#12352](https://github.com/mapbox/mapbox-gl-native/pull/12352))


Added an -[MGLMapViewDelegate mapView:canSelectAnnotation:] method to specify whether an annotation is selectable.

1ec5 · 2018-08-15T20:28:11Z

platform/ios/src/MGLMapViewDelegate.h

+ Returns a Boolean value indicating whether the annotation can be selected.
+
+ If the return value is `YES`, or if this method is absent from the delegate,
+ the annotation will be selected.


I don’t think it’s the case that, if this method is unimplemented, the annotation automatically gets selected when it’s added to the map view. It also doesn’t seem to affect whether the developer can select it programmatically via -selectAnnotation:. Rather, it seems to only affect whether an annotation can be selected by a user gesture:

If the return value is YES, the user can select the annotation by tapping [clicking] on it. If the delegate does not implement this method, the default value is YES.

There’s demand in #10539 for an option to prevent tap-to-select, but no requests, as far as I can tell, for an option to prevent programmatic selection.

1ec5 · 2018-08-15T20:30:07Z

platform/ios/src/MGLMapView.mm

@@ -4125,6 +4125,11 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
                return true;
            }

+            if ([self.delegate respondsToSelector:@selector(mapView:canSelectAnnotation:)] &&
+                ![self.delegate mapView:self canSelectAnnotation:annotation]) {


Should this delegate method apply to all annotations or only shape annotations? Point annotations already have a MGLAnnotationImage.enabled and MGLAnnotationView.enabled, so a redundant option without a defined order of precedence could be confusing. The enabled properties can be more efficient because they’re declarative: application code doesn’t run when MGLMapView needs to check whether a point annotation is interactive.

Consider renaming the method -mapView:shapeAnnotationIsEnabled: and mentioning the enabled properties in its documentation. There’s precedent in MGLMapViewDelegate for shape annotation–specific options.

1ec5

Mostly documentation nits, but otherwise good to go.

1ec5 · 2018-08-17T21:32:57Z

platform/ios/CHANGELOG.md

@@ -8,7 +8,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
 * When a symbol in an `MGLSymbolStyleLayer` has both an icon and text, both are shown or hidden together based on available space. ([#12521](https://github.com/mapbox/mapbox-gl-native/pull/12521))
 * The `-[MGLMapView visibleFeaturesAtPoint:]` method can now return features near tile boundaries at high zoom levels. ([#12570](https://github.com/mapbox/mapbox-gl-native/pull/12570))
 * Fixed inconsistencies in exception naming. ([#12583](https://github.com/mapbox/mapbox-gl-native/issues/12583))
-* `-[MGLMapViewDelegate mapView:canSelectAnnotation:]` can now specify if an annotation is selectable. ([#12352](https://github.com/mapbox/mapbox-gl-native/pull/12352))
+* Added an -[MGLMapViewDelegate mapView:shapeAnnotationIsEnabled:] method to specify whether an annotation is selectable. ([#12352](https://github.com/mapbox/mapbox-gl-native/pull/12352))


Nit: use backticks around -[MGLMapViewDelegate mapView:shapeAnnotationIsEnabled:]. (×2)

1ec5 · 2018-08-17T21:34:54Z

platform/ios/src/MGLMapView.mm

@@ -4156,7 +4151,12 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
            {
                if ([annotation isKindOfClass:[MGLMultiPoint class]])
                {
-                    return false;
+                    if ([self.delegate respondsToSelector:@selector(mapView:shapeAnnotationIsEnabled:)] &&


Not sure if it matters for performance reasons, but for better readability, you could factor this call out of the std::remove_if() call. (×2)

1ec5 · 2018-08-17T21:36:11Z

platform/ios/src/MGLMapView.mm

+                        ![self.delegate mapView:self shapeAnnotationIsEnabled:(MGLMultiPoint *)annotation]) {
+                        return true;
+                    } else {
+                        return false;


This is fine, but FYI you can also wrap the conditional in !! or compare to YES. (×2)

1ec5 · 2018-08-17T21:36:55Z

platform/ios/src/MGLMapViewDelegate.h


- If the return value is `YES`, or if this method is absent from the delegate,
- the annotation will be selected.
+ If the return value is YES, the user can select the annotation by tapping [clicking]


The “clicking” was meant for the macOS implementation of MGLMapView. iOS documentation should never refer to clicking, while macOS documentation should never refer to tapping. (×2)

1ec5 · 2018-08-17T21:37:39Z

platform/ios/src/MGLMapViewDelegate.h

- If the return value is `YES`, or if this method is absent from the delegate,
- the annotation will be selected.
+ If the return value is YES, the user can select the annotation by tapping [clicking]
+ on it. If the delegate does not implement this method, the default value is YES.


Nit: surround YES in backticks. (×4)

julianrex · 2018-08-17T21:39:14Z

platform/ios/src/MGLMapView.mm

@@ -4151,7 +4151,12 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
            {
                if ([annotation isKindOfClass:[MGLMultiPoint class]])
                {
-                    return false;
+                    if ([self.delegate respondsToSelector:@selector(mapView:shapeAnnotationIsEnabled:)] &&


Minor: You could replace the if with return (...) here and below.

…ethod.

…ode readability.

julianrex · 2018-08-21T00:36:48Z

Thanks @fabian-guerra!

jmkiley added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold annotations Annotations on iOS and macOS or markers on Android in progress labels Jul 10, 2018

jmkiley self-assigned this Jul 10, 2018

jmkiley changed the base branch from master to release-drink July 12, 2018 00:37

jmkiley force-pushed the jmkiley-annotation-enabled branch from c297443 to a0822d9 Compare July 13, 2018 19:53

jmkiley added ✓ ready for review and removed in progress ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jul 13, 2018

jmkiley requested review from julianrex and friedbunny July 13, 2018 19:54

jmkiley changed the title ~~[WIP] Add enabled property to MGLShape~~ Add enabled property to MGLShape Jul 13, 2018

jmkiley requested a review from fabian-guerra July 13, 2018 21:42

fabian-guerra reviewed Jul 13, 2018

View reviewed changes

jmkiley added in progress and removed ✓ ready for review labels Jul 13, 2018

julianrex reviewed Jul 14, 2018

View reviewed changes

1ec5 reviewed Jul 18, 2018

View reviewed changes

1ec5 mentioned this pull request Jul 25, 2018

Prevent specific MGLAnnotation objects from being selected #10539

Closed

julianrex assigned fabian-guerra Aug 13, 2018

fabian-guerra force-pushed the jmkiley-annotation-enabled branch 2 times, most recently from 19524ec to 5ce307f Compare August 14, 2018 23:49

fabian-guerra changed the base branch from release-drink to master August 14, 2018 23:49

fabian-guerra force-pushed the jmkiley-annotation-enabled branch from 54fd60c to a5c9e0a Compare August 15, 2018 18:24

1ec5 added the macOS Mapbox Maps SDK for macOS label Aug 15, 2018

1ec5 reviewed Aug 15, 2018

View reviewed changes

1ec5 approved these changes Aug 17, 2018

View reviewed changes

julianrex reviewed Aug 17, 2018

View reviewed changes

fabian-guerra force-pushed the jmkiley-annotation-enabled branch 2 times, most recently from 8617ca8 to 09a377a Compare August 20, 2018 20:32

fabian-guerra added 6 commits August 20, 2018 16:01

[ios, macos] Add -[MGLMapViewDelegate mapView:canSelectAnnotation:] m…

9aaaef3

…ethod.

[ios, macos] Update changelogs.

363ebcc

[ios, macos] Add mapView:canSelect: integration tests.

fc952c3

[ios, macos] Change semantics to shape annotations.

0272216

[ios, macos] Update changelogs.

f9472b5

[ios, macos] Update shapeAnnotationIsEnabled documentation, improve c…

995f0b9

…ode readability.

fabian-guerra force-pushed the jmkiley-annotation-enabled branch from 09a377a to 995f0b9 Compare August 20, 2018 23:03

fabian-guerra merged commit 7fc872b into master Aug 20, 2018

fabian-guerra deleted the jmkiley-annotation-enabled branch August 20, 2018 23:30

1ec5 mentioned this pull request Sep 26, 2018

Moving gesture selected annotations onscreen should be configurable #12064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enabled property to MGLShape #12352

Add enabled property to MGLShape #12352

jmkiley commented Jul 10, 2018 •

edited

Loading

jmkiley commented Jul 13, 2018

jmkiley commented Jul 13, 2018

jmkiley commented Jul 13, 2018 •

edited

Loading

fabian-guerra left a comment

fabian-guerra Jul 13, 2018

jmkiley Jul 13, 2018

jmkiley commented Jul 13, 2018 •

edited

Loading

julianrex Jul 14, 2018

julianrex Jul 14, 2018

jmkiley Jul 14, 2018

1ec5 Jul 18, 2018

julianrex Jul 14, 2018

jmkiley Jul 14, 2018

julianrex commented Jul 16, 2018

1ec5 left a comment

1ec5 Jul 18, 2018

1ec5 Jul 18, 2018

1ec5 Jul 18, 2018

fabian-guerra commented Aug 15, 2018

1ec5 Aug 15, 2018

1ec5 Aug 15, 2018 •

edited

Loading

1ec5 Aug 15, 2018

1ec5 left a comment

1ec5 Aug 17, 2018

1ec5 Aug 17, 2018

1ec5 Aug 17, 2018

1ec5 Aug 17, 2018

1ec5 Aug 17, 2018

julianrex Aug 17, 2018

julianrex commented Aug 21, 2018

Add enabled property to MGLShape #12352

Add enabled property to MGLShape #12352

Conversation

jmkiley commented Jul 10, 2018 • edited Loading

jmkiley commented Jul 13, 2018

jmkiley commented Jul 13, 2018

jmkiley commented Jul 13, 2018 • edited Loading

fabian-guerra left a comment

Choose a reason for hiding this comment

Post tap processing- Shape and View based annotations:

Tap processing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmkiley commented Jul 13, 2018 • 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

julianrex commented Jul 16, 2018

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

Choose a reason for hiding this comment

fabian-guerra commented Aug 15, 2018

Choose a reason for hiding this comment

1ec5 Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

julianrex commented Aug 21, 2018

jmkiley commented Jul 10, 2018 •

edited

Loading

jmkiley commented Jul 13, 2018 •

edited

Loading

jmkiley commented Jul 13, 2018 •

edited

Loading

1ec5 Aug 15, 2018 •

edited

Loading