Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Add support for queryRenderedFeatures filter #8246

Merged
merged 3 commits into from
Mar 4, 2017
Merged

[core] Add support for queryRenderedFeatures filter #8246

merged 3 commits into from
Mar 4, 2017

Conversation

jfirebaugh
Copy link
Contributor

Fixes #7319
Fixes #8193

@mention-bot
Copy link

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

@jfirebaugh jfirebaugh mentioned this pull request Mar 1, 2017
Closed
@jfirebaugh jfirebaugh force-pushed the 7319-query-with-filter branch from 47bb98c to 1b3e80e Compare March 1, 2017 19:55
@ivovandongen ivovandongen force-pushed the 7319-query-with-filter branch 2 times, most recently from 95e2ca2 to f3912c1 Compare March 3, 2017 01:45
1ec5
1ec5 suggested changes Mar 3, 2017
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the comments below, please add a blurb to the iOS and macOS changelogs.

platform/ios/src/MGLMapView.mm Outdated
@@ -4605,7 +4606,7 @@ - (void)updateHeadingForDeviceOrientation
return [self visibleFeaturesAtPoint:point inStyleLayersWithIdentifiers:nil];
}

- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(CGPoint)point inStyleLayersWithIdentifiers:(NS_SET_OF(NSString *) *)styleLayerIdentifiers
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(CGPoint)point inStyleLayersWithIdentifiers:(NS_SET_OF(NSString *) *)styleLayerIdentifiers withPredicate:(NSPredicate *)predicate
Copy link
Contributor

@1ec5 1ec5 Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs to be reflected in the header. Otherwise, it won’t be possible to call this method anymore. Meanwhile, someone will call -visibleFeaturesAtPoint:inStyleLayersWithIdentifiers: and crash because dynamic method dispatch will fail.

platform/ios/src/MGLMapView.mm Outdated
@@ -4605,7 +4606,7 @@ - (void)updateHeadingForDeviceOrientation
return [self visibleFeaturesAtPoint:point inStyleLayersWithIdentifiers:nil];
}

- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(CGPoint)point inStyleLayersWithIdentifiers:(NS_SET_OF(NSString *) *)styleLayerIdentifiers
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(CGPoint)point inStyleLayersWithIdentifiers:(NS_SET_OF(NSString *) *)styleLayerIdentifiers withPredicate:(NSPredicate *)predicate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming -visibleFeaturesAtPoint:inStyleLayersWithIdentifiers: to -visibleFeaturesAtPoint:inStyleLayersWithIdentifiers:withPredicate: breaks backwards compatibility. Checking for nil below isn’t enough, because this signature now requires the developer to specify nil rather than omitting that part of the selector. (In Swift, this is the difference between an optional argument and an argument with a default value.) You’ll need to bring back -visibleFeaturesAtPoint:inStyleLayersWithIdentifiers:, which can call into this method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace withPredicate: with predicate:. While it can be helpful to make a selector read like an English sentence, at a certain point that falls apart and it’s better to name the rest of the parameters without prepositions.

@1ec5 1ec5 added this to the ios-v3.5.0 milestone Mar 3, 2017
@ivovandongen ivovandongen force-pushed the 7319-query-with-filter branch 2 times, most recently from 9f9849a to 2c38321 Compare March 3, 2017 22:13
@ivovandongen
Copy link
Contributor

@1ec5 Addressed your comments. Thanks.

1ec5
1ec5 reviewed Mar 3, 2017
View reviewed changes
platform/ios/CHANGELOG.md Outdated
@@ -52,6 +52,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
* Fixed an issue that sometimes caused crashes when the SDK interacted with the file system in the background. ([#8125](https://github.com/mapbox/mapbox-gl-native/pull/8125))
* Added a `MGLDistanceFormatter` class for formatting geographic distances. ([#7888](https://github.com/mapbox/mapbox-gl-native/pull/7888))
* Fixed an issue that was causing the system location indicator to stay on in background after telemetry was disabled. ([#7833](https://github.com/mapbox/mapbox-gl-native/pull/7833))
* Added support for filters in rendered feature querying [8256](https://github.com/mapbox/mapbox-gl-native/pull/8246)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s call them “predicates”, since that’s the term developers will know them by. (“Filter” is the verb, “predicate” is the noun, following the precedent set by -[NSArray filteredArrayUsingPredicte:].)

platform/ios/src/MGLMapView.h Outdated

This method may return all features from the specified layers. To filter
the returned features, use the
`-visibleFeaturesAtPoint:inStyleLayersWithIdentifiers:predicate` method. For more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s a missing : after “predicate” that will prevent jazzy from linking this reference to the method documentation. (×2)

platform/ios/src/MGLMapView.h Outdated
@return An array of objects conforming to the `MGLFeature` protocol that
represent features in the sources used by the current style.
*/
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(CGPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers NS_SWIFT_NAME(visibleFeatures(at:styleLayerIdentifiers:));
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(CGPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers predicate:(nullable NSPredicate*)predicate NS_SWIFT_NAME(visibleFeatures(at:styleLayerIdentifiers:predicate:));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space between NSPredicate and *. The predominent Objective-C coding style is Type *name and Type * const name. There’s less certainty about the proper style for Objective-C++ code, which is one reason we’ve yet to lint for that sort of stylistic detail.

platform/ios/src/MGLMapView.h Outdated
@param point A point expressed in the map view’s coordinate system.
@param styleLayerIdentifiers A set of strings that correspond to the names of
layers defined in the current style. Only the features contained in these
layers are included in the returned array.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: in Xcode, ⇧⌥⌘V pastes preserving indentation. (The same shortcut preserves formatting in any native macOS application.)

platform/macos/src/MGLMapView.h Outdated
*/
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(NSPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers NS_SWIFT_NAME(visibleFeatures(_:styleLayerIdentifiers:));
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(NSPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers predicate:(nullable NSPredicate*)predicate NS_SWIFT_NAME(visibleFeatures(at:styleLayerIdentifiers:predicate:));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, good catch for replacing _: with at: here!

platform/macos/src/MGLMapView.h Outdated
@return An array of objects conforming to the `MGLFeature` protocol that
represent features in the sources used by the current style.
*/
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesInRect:(NSRect)rect inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers NS_SWIFT_NAME(visibleFeatures(_:styleLayerIdentifiers:));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use in: instead of _: here, like you did above?

@ivovandongen ivovandongen force-pushed the 7319-query-with-filter branch from 2c38321 to b6618f7 Compare March 3, 2017 23:32
@ivovandongen ivovandongen force-pushed the 7319-query-with-filter branch from f6ff5fa to 829253c Compare March 4, 2017 22:29
1ec5
1ec5 approved these changes Mar 4, 2017
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple changes need to be brought over to macOS, but otherwise this change looks good to go.

platform/macos/src/MGLMapView.h Outdated
@@ -789,6 +789,33 @@ MGL_EXPORT IB_DESIGNABLE
Returns an array of rendered map features that intersect with a given point,
restricted to the given style layers.

This method may return all features from the specified layers. To filter
the returned features, use the
`-visibleFeaturesAtPoint:inStyleLayersWithIdentifiers:predicate` method. For more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s a missing : here too. (×2)

platform/macos/src/MGLMapView.h Outdated
*/
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(NSPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers NS_SWIFT_NAME(visibleFeatures(_:styleLayerIdentifiers:));
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(NSPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers predicate:(nullable NSPredicate*)predicate NS_SWIFT_NAME(visibleFeatures(at:styleLayerIdentifiers:predicate:));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NSPredicate * instead of NSPredicate*

platform/macos/src/MGLMapView.h Outdated
@return An array of objects conforming to the `MGLFeature` protocol that
represent features in the sources used by the current style.
*/
- (NS_ARRAY_OF(id <MGLFeature>) *)visibleFeaturesAtPoint:(NSPoint)point inStyleLayersWithIdentifiers:(nullable NS_SET_OF(NSString *) *)styleLayerIdentifiers NS_SWIFT_NAME(visibleFeatures(_:styleLayerIdentifiers:));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace _: with at: for consistency? Thanks!

@ivovandongen ivovandongen force-pushed the 7319-query-with-filter branch from 829253c to d5e989f Compare March 4, 2017 23:08
@ivovandongen ivovandongen self-assigned this Mar 4, 2017
@ivovandongen ivovandongen added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Mar 4, 2017
@ivovandongen ivovandongen merged commit b5d0a66 into master Mar 4, 2017
@ivovandongen ivovandongen deleted the 7319-query-with-filter branch March 4, 2017 23:42
@ivovandongen ivovandongen mentioned this pull request Mar 8, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@1ec5 1ec5 1ec5 approved these changes

Assignees

@ivovandongen ivovandongen

Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl feature iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Projects
None yet
Milestone
ios-v3.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jfirebaugh @mention-bot @ivovandongen @1ec5 @asheemmamoowala