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

Layer::getFilter() returns filter with $type compared to unsigned integer #7552

Closed
1ec5 opened this issue Dec 27, 2016 · 8 comments
Closed
Labels
bug Core The cross-platform C++ core, aka mbgl runtime styling
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 27, 2016

A filter in style JSON that compares $type to a string such as Polygon gets parsed as an mbgl::style::EqualsFilter with an integer value such as 3. That seems like a reasonable in-memory optimization, but developers using the runtime styling API would expect a string representation such as Polygon.

For example, on iOS and macOS, the following Objective-C code:

mapView.styleURL = [MGLStyle streetsStyleURLWithVersion:10];
…
MGLFillStyleLayer *fillLayer = (MGLFillStyleLayer *)[mapView.style layerWithIdentifier:@"barrier_line-land-polygon"];
NSLog("%@", fillLayer.predicate);

produces this predicate:

$type == 3 AND class == "land"

whereas it should produce:

$type == "Polygon" AND class == "land"

at least until #7298 is fixed.

@jfirebaugh, should the SDK be responsible for converting these integers back into strings, or should mbgl handle that?

/cc @boundsj

@1ec5 1ec5 added bug Core The cross-platform C++ core, aka mbgl runtime styling labels Dec 27, 2016
@jfirebaugh
Copy link
Contributor

We should probably have enumerated constants for the feature types, and explicit filter types TypeEqualsFilter, TypeNotEqualsFilter, TypeInFilter, TypeNotInFilter.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 2, 2017

The way the style specification is written, something like [">=", "$type", "LineString"] would be allowed. We might want to redefine that property as being an enumeration and specify which operators are allowed for enumerations.

@jfirebaugh
Copy link
Contributor

@ericrwolfe
Copy link
Contributor

Looks like as it stands in 3.4.0, setting a layer's predicate to "$type == 'LineString'" etc will not work as described in the documentation.

The only workaround right now is to use the integer values for type e.g. "$type == 2" (1 for Point, 2 for LineString, 3 is for Polygon).

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 25, 2017

Here’s the iOS/macOS documentation. (The Android SDK is also affected, but it relies on the style specification to document filters.) Since this is a bug, I suppose we should hold off on updating the documentation, misleading as it may be. Or we could remove the part about $type.

@jfirebaugh
Copy link
Contributor

#7971 is the core change I have in mind. Is it feasible to adjust the SDK bindings to accommodate the new types (TypeEqualsFilter, IdentifierEqualsFilter, etc.) without breaking backward compatibility?

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 8, 2017

#7971 (comment) would adjust the iOS and macOS SDKs without breaking backwards compatibility.

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 9, 2017

Fixed in #7971.

@1ec5 1ec5 closed this as completed Feb 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl runtime styling
Projects
None yet
Development

No branches or pull requests

3 participants