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.

Sign up for GitHub

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

Jump to bottom

Add MapObjectManager for multi-layer support with KML/GeoJSON renderers #380

Merged
merged 25 commits into from
Jan 28, 2020
Merged

Add MapObjectManager for multi-layer support with KML/GeoJSON renderers #380

merged 25 commits into from
Jan 28, 2020

Conversation

jeffdgr8
Copy link
Contributor

@jeffdgr8 jeffdgr8 commented Mar 28, 2017 •
edited by barbeau
Loading

MarkerManager, PolygonManager, PolylineManager, GroundOverlayManager, and CircleManager used to create collections for map objects in KML and GeoJSON renderers. This allows them to set up listeners for these map objects while also allowing other collections of any of these map objects to be used external to the KML/GeoJSON objects on the map, which can implement their own listeners through their own collections.

MapObjectManager is an abstract base class based on MarkerManger, now used for MarkerManager and the newly implemented PolygonManager, PolylineManager, GroundOverlayManager, and CircleManager.

Fix #459, Fix #420, Fix #430, Fix #594, Fix #538

jeffdgr8 added 8 commits March 27, 2017 01:26
@jeffdgr8
Map object managers in KML/GeoJSON renderers
27a5971 
MarkerManager, PolygonManager, PolylineManager, and GroundOverlayManager
used to create collections for map objects in KML and GeoJSON renderers.
This allows them to set up listeners for these map objects while also
allowing other collections of any of these map objects to be used
external to the KML/GeoJSON objects on the map, which can implement
their own listeners through their own collections.

MapObjectManager is an abstract base class based on MarkerManger, now
used for MarkerManager and the newly implemented PolygonManager,
PolylineManager, and GroundOverlayManager.
@jeffdgr8
Add collections managers as listeners to map on instantiation
1e3b9d1 
- Added CircleManager
- Instantiate managers in Renderer constructor if null parameters
@jeffdgr8
Clarified comment for listener behavior
9494f03
@jeffdgr8
Set listeners on main thread
888eb4f
@jeffdgr8
Handle null map in Renderer for unit test cases
60b060b
@jeffdgr8
Remove map objects from collections instead of map directly
626cc2b
@jeffdgr8
Check for null ground overlay references
0559c8f
@jeffdgr8
Remove feature collections
9cb96ce
@jeffdgr8 jeffdgr8 mentioned this pull request Nov 8, 2017
Merged
@jeffdgr8
Merge branch 'master' into kml-marker-manager
da45080 
# Conflicts:
#	library/src/com/google/maps/android/collections/MarkerManager.java
#	library/src/com/google/maps/android/data/Renderer.java
@jeffdgr8
Copy link
Contributor Author

Merged latest changes from master and updated so it's mergeable again.

@jeffdgr8
Copy link
Contributor Author

jeffdgr8 commented Nov 2, 2018

These changes are important in order to be able to use the KML functionality alongside other usages of markers on the map, including clustering. The same applies for polygons, polylines, ground overlays, and circles. This allows each unique use case for each of these map objects to to be able to maintain separate listeners.

This was referenced Nov 21, 2018
Closed
Closed
Closed
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 20, 2019
@jpoehnelt jpoehnelt self-requested a review October 14, 2019 17:37
@jpoehnelt jpoehnelt added the triage me I really want to be triaged. label Oct 14, 2019
@barbeau barbeau added priority: p2 Moderately-important priority. Fix may not be included in next release. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed triage me I really want to be triaged. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Nov 8, 2019
@barbeau
Copy link
Collaborator

barbeau commented Nov 22, 2019

@jeffdgr8 Now that the retain bitmap cache (#381) PR was merged there are a few resulting conflicts here - would you mind resolving them?

@barbeau barbeau added the semver: major Hint for users that this is an API breaking change. label Jan 8, 2020
This was referenced Jan 8, 2020
Closed
Merged
@barbeau
Copy link
Collaborator

barbeau commented Jan 9, 2020

I've created a new multi-layer demo activity in PR #598 (which is based on this PR branch) that adds the following to a single map to test multiple layer interactions:

  • a cluster manager with markers
  • a GeoJSON polyline layer
  • 2 KML layers (polyline and polygon)
  • a normal marker (unclustered)

I've started testing this PR with the multi-layer demo, and here's what I see so far:

  • When all layers are enabled, the click listeners don't seem to fire on the GeoJSON or KML layers
  • If I comment out the KML polygon, then the KML polyline listener fires (and the cluster and normal marker respond to clicks) but not the GeoJSON layer
  • If I comment out both KML layers, then the remaining listeners work fine (GeoJSON, the cluster, and normal markers)

(Note that KML Polygon click listening is also broken in the master branch - see #558)

I haven't started digging into why the other items aren't functioning as intended (assuming I understand the intent of this PR correctly), but @jeffdgr8 let me know if you have any thoughts.

@jeffdgr8
Copy link
Contributor Author

jeffdgr8 commented Jan 9, 2020

@barbeau the object managers work by acting as a hub for the map event listeners, which delegate the specific events to the collections created by the managers in the different map layers. It's required to create these object managers and share them between the layers using the constructors that take them as parameters. I created a PR for your demo https://github.com/CUTR-at-USF/android-maps-utils/pull/1 that does this and confirmed all the click listeners are working, except for the polygon, which is apparently broken (#558).

I also hadn't added these shared object managers to the GeoJsonLayer yet, so I did this in this commit 609008d.

@barbeau
Copy link
Collaborator

barbeau commented Jan 9, 2020

@jeffdgr8 Thanks!! I'll take another look.

@barbeau
Copy link
Collaborator

barbeau commented Jan 23, 2020

@googlebot

barbeau
barbeau requested changes Jan 24, 2020
View reviewed changes
Copy link
Collaborator

@barbeau barbeau left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@jeffdgr8 Thanks for all your work on this feature to align the management of the various spatial data types to allow multiple simultaneous layers on the map! And for your patience waiting for this to be reviewed.

This looks good to me, and I've tested with PR #598 and everything seems to work as intended (modulo known issues in the master branch #558 and #538 EDIT Looks like this PR fixes #538).

One small item - I've added a few requests for clarification in the Javadocs to make sure the behavior is clear to others going forward. If you can review/update we should be able to get this merged! Thanks!

library/src/main/java/com/google/maps/android/data/kml/KmlLayer.java Show resolved Hide resolved
library/src/main/java/com/google/maps/android/data/kml/KmlLayer.java Show resolved Hide resolved
library/src/main/java/com/google/maps/android/data/geojson/GeoJsonLayer.java Show resolved Hide resolved
@barbeau barbeau mentioned this pull request Jan 24, 2020
Closed
@jeffdgr8
Copy link
Contributor Author

@barbeau I made those changes to the doc comments. I also removed the throws IOException, XmlPullParserException from addKMLToMap(), as these exceptions aren't actually thrown here, only in the KmlLayer constructors.

@jeffdgr8 jeffdgr8 requested a review from barbeau January 25, 2020 06:14
@barbeau barbeau changed the title Map object managers in KML/GeoJSON renderers Use new MapObjectManager with KML/GeoJSON renderers for multi-layer support Jan 28, 2020
@barbeau barbeau changed the title Use new MapObjectManager with KML/GeoJSON renderers for multi-layer support Use MapObjectManager with KML/GeoJSON renderers for multi-layer support Jan 28, 2020
@barbeau barbeau changed the title Use MapObjectManager with KML/GeoJSON renderers for multi-layer support Add MapObjectManager for multi-layer support with KML/GeoJSON renderers Jan 28, 2020
@barbeau barbeau merged commit a55bca0 into googlemaps:master Jan 28, 2020
@barbeau
Copy link
Collaborator

barbeau commented Jan 28, 2020

Thanks @jeffdgr8!!

@jeffdgr8 jeffdgr8 deleted the kml-marker-manager branch January 28, 2020 15:48
This was referenced Jan 29, 2020
Closed
Closed
@arriolac arriolac added this to the 1.0 milestone Feb 24, 2020
@bartekpacia
Copy link

That's great!
Will the official docs be updated with these changes?

@barbeau
Copy link
Collaborator

barbeau commented Mar 29, 2020

@bartekpacia you can find the multilayer docs at https://developers.google.com/maps/documentation/android-sdk/utility/multilayer

@arriolac arriolac mentioned this pull request Apr 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barbeau barbeau barbeau approved these changes

@arriolac arriolac Awaiting requested review from arriolac

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. semver: major Hint for users that this is an API breaking change.
Projects
None yet
Milestone
1.0
6 participants
@jeffdgr8 @barbeau @bartekpacia @arriolac @jpoehnelt @googlebot