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

Make Annotation objects immutable #2546

Closed
ljbade opened this issue Oct 6, 2015 · 15 comments · Fixed by #2637
Closed

Make Annotation objects immutable #2546

ljbade opened this issue Oct 6, 2015 · 15 comments · Fixed by #2637
Assignees
@ljbade
Labels
Android Mapbox Maps SDK for Android refactor
Milestone
android-v2.2.0

Comments

@ljbade
Copy link
Contributor

ljbade commented Oct 6, 2015

In the various annotation objects (Marker etc) we have a number of set... functions. These need to be removed so that they are immutable for the moment.

Main reason is the expected behaviour from developers is that for example Marker.setColor() will change the existing marker's colour. Instead nothing happens.

Full support for mutable annotations is not in the core layer at this stage. Even on iOS you can't update existing markers.

@bleege I think this is good candidate for 2.1.0
@ljbade ljbade added Android Mapbox Maps SDK for Android refactor labels Oct 6, 2015
@bleege
Copy link
Contributor

bleege commented Oct 7, 2015

@ljbade Sounds good. How about just commenting them out?

@bleege bleege added this to the android-v2.1.0 milestone Oct 7, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

@bleege I'm not a huge fan of commenting code out. core/iOS seem to avoid commented code but we are getting it scattered in Android.

I think correct fix here is to make them package private so the MarkerOptions class can still use set.

@ljbade ljbade mentioned this issue Oct 8, 2015
Closed
@bleege
Copy link
Contributor

bleege commented Oct 8, 2015

@ljbade Can you help me understand how making them package private so that MarkerOptions can still use them solves this issue? I'm just confused as the original recommendation was "These need to be removed so that they are immutable for the moment."

@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

@bleege I mean immutable in the "public API". in #2551 you say we should prefer to use set methods over field access. Package private means MarkerOptions can use them to create the Marker object, but once we return the Marker object to the end developer, they can't make any further changes to the object.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

@bleege I also noticed that some of the annotation object expose properties from Google Maps API we don't support in core. These should be made private.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

Fixing up a few other things I noticed:

  • mark Marker, Polygon Polyline as final.
  • mark Annotation, AnnotationOptions, MultiPoint and MultiPointOptions constructors protected.
  • change Circle and CircleOptions to package private as they are currently unimplemented in native layer
  • make Marker, Polygon Polyline constructors package private - only way to create these should be via MapView.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

Hmm looks like MapView needs to be able to set the ID field on the objects. We might have to implement #2538

@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

Current WIP is in https://github.com/mapbox/mapbox-gl-native/tree/2546-fix-annotations

@ljbade ljbade modified the milestones: android-v2.2.0, android-v2.1.0 Oct 16, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

Moving this to 2.2.0 as I don't think we have enough time to make all these changes. It is looking like the next version will include a number of refactors anyhow.

@bleege Do you agree?

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

On second thought now that we have #2631 I might as well roll a fix for that into this branch.

@ljbade ljbade modified the milestones: android-v2.1.0, android-v2.2.0 Oct 16, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

Pending #2538 I have marked all the public methods that should only be used internally by the SDK with JavaDoc that says "Do not use this X. Used internally by the SDK."

Only way to prevent this is to move to a single package SDK.

/cc @bleege

@ljbade ljbade mentioned this issue Oct 16, 2015
Closed
ljbade pushed a commit that referenced this issue Oct 16, 2015
[android] Refactor annotations API
93c0953 
Remove unimplemented properties.
Make annotations immutable.
Correct defintions of equals() and hasCode().
Change anchor U/V to int.
Move .alpha to MultiPoint and anchor() to PolylineOptions and PolygonOptions.
Make InfoWindow classes package private.
Add setOnInfoWindowClickListener and remove old method from Marker.
JavaDoc internal methods with "Do not use."
Refactor showInfoWindow() to remove need for exposing internal method.
Make select/deselectMarker public. Add getSelectedMarker.
Fix bug where you couldn't reselect a closed info window.
Fixes #2546
Fixes #2631
Fixes #2448
@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

This branch ended up being quite a bit of work.

Full details in the PR.

@ljbade ljbade mentioned this issue Oct 16, 2015
Merged
@bleege
Copy link
Contributor

bleege commented Oct 16, 2015

@ljbade This is a LARGE change and rushing it doesn't make sense. Let's put this back into android-v2.2.0 so that we have time to test it properly. See #2637 (comment).

@bleege bleege modified the milestones: android-v2.2.0, android-v2.1.0 Oct 16, 2015
@ljbade ljbade mentioned this issue Oct 17, 2015
Merged
ljbade pushed a commit that referenced this issue Oct 17, 2015
[android] Tidy up annotations API
1c5bab1 
Make unimplemented properties package private.
Make annotations object properties immutable.
Make InfoWindow and Circle classes package private.
Remove no longer needed casts from Options classes.
Added a few missing get/add methods to annotations classes for consistency.
Mininal version of #2546 suitable for v2.1.0 release.
@ljbade
Copy link
Contributor Author

ljbade commented Oct 17, 2015

Further work could be to make setId/getId, set/getMapView etc private by directly accessing them in JNI where needed.

To do this nativeAddMarker could be refactored to take a MarkerOptions and return a Marker.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 17, 2015

Going to need to remove anchor per #2652 in this 2.2.0 PR.

ljbade pushed a commit that referenced this issue Oct 20, 2015
[android] Tidy up annotations API
c0e1b54 
Make unimplemented properties package private.
Make annotations object properties immutable.
Make InfoWindow and Circle classes package private.
Remove no longer needed casts from Options classes.
Added a few missing get/add methods to annotations classes for consistency.
Mininal version of #2546 suitable for v2.1.0 release.
ljbade pushed a commit that referenced this issue Oct 22, 2015
[android] Tidy up annotations API
ec95dfe 
Make unimplemented properties package private.
Make annotations object properties immutable.
Make InfoWindow and Circle classes package private.
Remove no longer needed casts from Options classes.
Added a few missing get/add methods to annotations classes for consistency.
Mininal version of #2546 suitable for v2.1.0 release.
ljbade pushed a commit that referenced this issue Oct 22, 2015
[android] Refactor annotations API
927b4c6 
Remove unimplemented properties.
Correct defintions of equals() and hasCode().
Add setOnInfoWindowClickListener and remove old method from Marker.
Refactor showInfoWindow() to remove need for exposing internal method.
Make select/deselectMarker public. Add getSelectedMarker.
Fix bug where you couldn't reselect a closed info window.
Add empty constructor to LatLng and LatLngZoom.
Fixes #2546
Fixes #2631
Fixes #2448
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this issue Nov 6, 2015
[android] Tidy up annotations API
afccf5f 
Make unimplemented properties package private.
Make annotations object properties immutable.
Make InfoWindow and Circle classes package private.
Remove no longer needed casts from Options classes.
Added a few missing get/add methods to annotations classes for consistency.
Mininal version of mapbox#2546 suitable for v2.1.0 release.
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this issue Nov 6, 2015
[android] Refactor annotations API
7a2bb60 
Remove unimplemented properties.
Correct defintions of equals() and hasCode().
Add setOnInfoWindowClickListener and remove old method from Marker.
Refactor showInfoWindow() to remove need for exposing internal method.
Make select/deselectMarker public. Add getSelectedMarker.
Fix bug where you couldn't reselect a closed info window.
Add empty constructor to LatLng and LatLngZoom.
Fixes mapbox#2546
Fixes mapbox#2631
Fixes mapbox#2448
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ljbade ljbade

Labels
Android Mapbox Maps SDK for Android refactor
Projects
None yet
Milestone
android-v2.2.0
Development

Successfully merging a pull request may close this issue.

[android] Refactor annotations API mapbox/mapbox-gl-native
2 participants
@bleege @ljbade