Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KML/GeoJSON polygon onFeatureClick() not called #558

Closed
tenSunFree opened this issue Oct 5, 2019 · 7 comments · Fixed by #614
Closed

KML/GeoJSON polygon onFeatureClick() not called #558

tenSunFree opened this issue Oct 5, 2019 · 7 comments · Fixed by #614
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Milestone

Comments

@tenSunFree
Copy link

關鍵在於 Renderer 的 addedPolygon.setClickable(polygonOptions.isClickable());

polygonOptions.isClickable() 總是得到false

所以如果想讓 KmlDemoActivity的onFeatureClick 可以被調用

必須修正成 addedPolygon.setClickable(true);

@tenSunFree tenSunFree added triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 5, 2019
@jpoehnelt jpoehnelt added needs more info This issue needs more information from the customer to proceed. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 7, 2019
@tenSunFree
Copy link
Author

PolygonOptions在創建的時候 zzcu的值預設為false

也就是說 PolygonOptions.isClickable() 預設為false

也就導致 Renderer的 addedPolygon.setClickable(false)

進而導致 onFeatureClick 不會被調用

2019-10-08_020908

@jpoehnelt jpoehnelt removed the triage me I really want to be triaged. label Oct 8, 2019
@barbeau barbeau changed the title KmlDemoActivity onFeatureClick 不會被調用 KmlDemoActivity.onFeatureClick() not called Oct 30, 2019
@barbeau
Copy link
Collaborator

barbeau commented Oct 30, 2019

I looked at this with the current master branch at 80d9497 (0.6.2), and here's my understanding of what's happening (partially thanks to Google Translate). @tenSunFree Please let me know if I got any of this wrong :).

Summary

In KmlDemoActivity, if you load the KML file from a local resource and then tap on it, nothing happens (KmlLayer.OnFeatureClickListener's onFeatureClick() is never called).

Steps to repro

  1. In KmlDemoActivity.startDemo(), comment out retrieveFileFromUrl(); and uncomment retrieveFileFromResource(); to load a KML file as a map layer from bundled resources instead of a remote URL.
  2. Start the demo app and tap on "KML LAYER OVERLAY" button at the bottom
  3. Tap on one of the Google building map polygons (e.g., the blue polygon)

What I expect to see

The click listener should fire and I should see a Toast that says Feature clicked: #, where # is the feature ID.

What I actually see

Tapping on the blue polygon has no effect - the listener doesn't fire (see below screenshot).

image

I also noticed that the remote URL used for retrieveFileFromUrl(); is broken, but that's unrelated to this issue, so I'll open a separate issue for that.

@barbeau barbeau added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed needs more info This issue needs more information from the customer to proceed. labels Oct 30, 2019
@barbeau barbeau changed the title KmlDemoActivity.onFeatureClick() not called KmlDemoActivity onFeatureClick() not called Oct 30, 2019
@barbeau barbeau added the priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. label Oct 31, 2019
@barbeau
Copy link
Collaborator

barbeau commented Jan 8, 2020

I ended up digging further into this while reviewing PR #380 (PR #380 does not appear to fix this issue).

The root problem here seems to be that the Maps API PolygonOptions.clickable attribute defaults to false:
https://developers.google.com/android/reference/com/google/android/gms/maps/model/PolygonOptions.html#clickable(boolean)

...as does PolylineOptions.clickable:
https://developers.google.com/android/reference/com/google/android/gms/maps/model/PolylineOptions.html#clickable(boolean)

In Render.addLineStringToMap() there is a addedPolyline.setClickable(true), which enables the click listener to fire for polylines.

However, there isn't an equivalent in Renderer.addPolygonToMap() for polygons - instead it just copies the existing PolygonOptions clickable attribute with addedPolygon.setClickable(polygonOptions.isClickable()).

This affects GeoJSON polygons too, but the difference being that in app code you can set clickable to true for GeoJSON polygons by setting a GeoJSON polygon style:

GeoJsonPolygonStyle polygonStyle = new GeoJsonPolygonStyle();
polygonStyle.setClickable(true);

...which then gets propagated to Renderer.addPolygonToMap().

Unless I'm missing something, there doesn't seem to be an equivalent way to set clickable to true from app code via KmlLayer, KmlPlacemark, or KmlStyle, and hence there isn't a way to make the KML polygon clickable.

Possible solution seems to be adding a boolean clickable attribute to KmlLayer constructors and passing that through to the KmlStyle so it gets set in the PolygonOptions at some point.

@barbeau
Copy link
Collaborator

barbeau commented Jan 8, 2020

I'm also wondering if the hard-coding of addedPolyline.setClickable(true); in Render.addLineStringToMap() is a mistake - this overrides app code like:

// Create a new polyline style
GeoJsonLineStringStyle lineStyle = new GeoJsonLineStringStyle();
lineStyle.setClickable(false);

@barbeau barbeau changed the title KmlDemoActivity onFeatureClick() not called KML/GeoJSON polygon onFeatureClick() not called Jan 28, 2020
@barbeau
Copy link
Collaborator

barbeau commented Jan 29, 2020

I was curious to see if there were any interactions with the KML spec itself here - the closest attribute I can find is BalloonStyle <displayMode>, which says:

If is default, Google Earth uses the information supplied in to create a balloon . If is hide, Google Earth does not display the balloon. In Google Earth, clicking the List View icon for a Placemark whose balloon's is hide causes Google Earth to fly to the Placemark.

Regardless of how this should translate to this library, the documentation for this library clearly says that <displayMode> isn't supported, so we don't need to worry about supporting that in fixing this issue:
https://developers.google.com/maps/documentation/android-sdk/utility/kml#supported

@barbeau
Copy link
Collaborator

barbeau commented Jan 29, 2020

I wanted to see where exactly this issue regressed.

Looking back at old library versions:

  • v0.4.4 - KmlDemoActivity doesn't include the line kmlLayer.setOnFeatureClickListener(), and KmlLayer doesn't actually include that method.
  • v0.5.0 - First release to include the line kmlLayer.setOnFeatureClickListener() in KmlDemoActivity. But tapping on polygon is still broken.

Using Git blame, it looks like the KML click listening in KmlDemoActivity was added as part of the GeoJSON/KML integration in #351. #351 (comment) says:

The changes here refactors this so that the KML and GeoJSON implementations share a common base abstraction. This should make implementing features and fixes for both a lot more easier in the future. For example, last year I implemented click handling for GeoJSON, but not KML, given the previous design, but that's now been addressed as part of this new work.

I tested with the specific PR merge commit (a7a8fcc), but same result as v0.5.0 - tapping on the KML polygon has no effect.

So, it appears that KML polygon click listening never actually worked as intended, so we're free to come up with our own solution here.

@barbeau
Copy link
Collaborator

barbeau commented Jan 29, 2020

Actually, looking more carefully at the original KML/GeoJSON integration PR, it did include the line to hard-code clickable to true for polygons.

    /**
     * Adds a DataPolygon to the map as a Polygon
     *
     * @param polygonOptions
     * @param polygon      contains coordinates for the Polygon
     * @return Polygon object created from given DataPolygon
     */
    protected Polygon addPolygonToMap(PolygonOptions polygonOptions, DataPolygon polygon) {
        // First array of coordinates are the outline
        polygonOptions.addAll(polygon.getOuterBoundaryCoordinates());
        // Following arrays are holes
        List<List<LatLng>> innerBoundaries = polygon.getInnerBoundaryCoordinates();
        for (List<LatLng> innerBoundary : innerBoundaries) {
            polygonOptions.addHole(innerBoundary);
        }
        Polygon addedPolygon = mMap.addPolygon(polygonOptions);
        addedPolygon.setClickable(true);
        addedPolygon.setClickable(polygonOptions.isClickable());
        return addedPolygon;
    }

...but then this PR allowed setting the clickable attribute via the style, changing the above code to:

addedPolygon.setClickable(polygonOptions.isClickable());

...which broke it, because the PolygonOptions defaults clickable to false. So the issue was re-introduced prior to the 0.6.0 release.

So, in summary, we effectively have the same issue for both KML/GeoJSON polylines and polygons right now:

  • We don't currently support both a developer-defined clickable attribute as well as a default clickable value of true.

...but we're handling each case differently:

  • For lines - we're currently setting default clickable attribute to true and ignoring developer-defined clickable attribute
  • For polygons - we're currently supporting developer-defined clickable attributes, but that results in a default of not clickable. And for KmlLayer there currently isn't a way to override the default.

barbeau added a commit that referenced this issue Jan 31, 2020
As discussed in #558, the original intent when combining the KML and GeoJSON renderers was to have GeoJSON and KML lines and polygons clickable by default.

However, there was a regression in #454 that unintentionally disabled clicks for KML and GeoJSON polygons when attempting to allow developers to use GeoJSON custom styles to set clickable.

I investigated both KML and GeoJSON lines and polygons, and here is the current state of the library:

* For KML/GeoJSON polygons - we're currently supporting developer-defined clickable styles for GeoJSON (the change in #454), but that results in a default of not clickable for GeoJSON and KML. And there isn't a way to override the default in KmlLayer.
* For KML/GeoJSON lines - we're currently ignoring developer-defined clickable styles and hard-coding the default clickable style to true.

This PR solves both problems (default of "not clickable" and supporting developer-defined styles) as follows:

* Makes all styles for GeoJSON and KML lines and polygons clickable by default by setting clickable=true in style constructors rather than the renderer. This also allows the developer to set a custom style with clickable=false on GeoJSON lines and polygons (custom programmatic KML styles currently aren't supported by the library).
* Make clickable default to true in the Style superclass - this should hopefully avoid a similar issue in any new implementations that extend Style
* Normalize the GeoJsonLineStringStyle and GeoJsonPolygonStyle clickable implementations - GeoJsonPolygonStyle was missing code related to the clickable attribute
* Add missing <name> attribute to the KML polygon demo file used in the MultiLayerDemoActivity
* Add unit tests:
    * For GeoJSON - test both default and programmatic custom clickable styles for both the style and renderer classes
    * For KML - test the default style for both the style and renderer classes
    * Refactor KML and GeoJSON test utility methods used in more than one test class to new Kml/GeoJsonTestUtil classes
    * Fix GeoJSON polygonStyle.toPolygonOptions().isVisible() test (this previously didn't check PolygonOptions)

I've also added TODO statements in the unit tests where we could expand coverage further (i.e., after adding the layers to the map), but that requires additional test instrumentation to have a live GoogleMap object.

Fixes #558

Refs: 558
@arriolac arriolac added this to the 1.0 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants