maplibre / maplibre-native

MapLibre Native - Interactive vector tile maps for iOS, Android and other platforms.
https://maplibre.org
BSD 2-Clause "Simplified" License
1.09k stars 322 forks source link

Make Android `queryRenderedFeatures()` consistent with iOS and GL JS #2828

Open albertmoravec opened 2 months ago

albertmoravec commented 2 months ago

Is your feature request related to a problem? Please describe. I am currently trying to call queryRenderedFeatures() from flutter-maplibre-gl and there's a big difference between how each platform treats the layerIds parameter.

The core of the problem is that on iOS and for GL JS passing an empty array to layerIds parameter still applies the filter and returns an empty result every time, but on Android an empty array gets ignored as if null was passed.

Describe the solution you'd like I find the iOS and GL JS implementation sensible - null means I don't want to filter by layer ID, empty array means I want to filter by layer ID, but there's no match since the array is empty. I think Android implementation should use the same logic.

I think the following line should not have the array length check and should accept and pass down empty arrays as well. With that the behavior will be dictated by mbgl implementation and not by the platform glue code.

Unfortunately the solution is not as simple, because it would break current Java API. Since varargs are (IMO inappropriately) used here, passing no argument to layerIds results in empty layerIds array to be passed down to native and only then it is filtered out. Removing varargs from there would be painful but correct solution in my opinion so it is possible to pass either nothing (null) or an empty array.

Describe alternatives you've considered In my case I could also attempt to write some glue code directly into flutter-maplibre-gl to make it work consistently across all three platforms, but I believe it should be fixed here.

louwers commented 2 months ago

Would adding another API and deprecating the old one be a good option perhaps?

albertmoravec commented 2 months ago

Yes, I think deprecating the varargs API and replacing it with one which accepts nullable array makes sense. If the deprecated API then converts empty varargs to null layerIds parameter it should be functionally equivalent and not break anything.

sargunv commented 2 weeks ago

Something to watch out for is that Kotlin can't easily pass null to a nullable vararg. The iOS SDK accepts a Set, perhaps a nullable Set is a good approach for the Android side?