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

Organise annotations function definitions #2539

Closed
ljbade opened this issue Oct 6, 2015 · 6 comments
Closed

Organise annotations function definitions #2539

ljbade opened this issue Oct 6, 2015 · 6 comments
Assignees
Labels
Android Mapbox Maps SDK for Android refactor

Comments

@ljbade
Copy link
Contributor

ljbade commented Oct 6, 2015

While working on #2002 I noticed the MapView annotation functions need a bit of a tidy up as they do not follow a logical pattern.

E.g. addMarker is the only function that takes a Marker object directly not a ...Options object. Important for this API which is supposed to take a mutable object and return a immutable object (#2538 will need this).

I made a pass at fixing this and will push to a branch.

@ljbade ljbade added refactor Android Mapbox Maps SDK for Android labels Oct 6, 2015
ljbade pushed a commit that referenced this issue Oct 6, 2015
@ljbade ljbade self-assigned this Oct 6, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 6, 2015

@bleege @tobrun Can you take a look at fdcef9a

I think we will want this in 2.1.0 to avoid a breaking change to the API in 2.2.0

@ljbade ljbade added this to the android-v2.2.0 milestone Oct 6, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 6, 2015

Doing some testing and noticed we get crash due to unmodifiable lists. Will convert everything to use normal lists and make getAllAnnotations return a shallow copy to mAnnotations.

Will do this tomorrow as I am out of time.

@tobrun
Copy link
Member

tobrun commented Oct 6, 2015

Ah addMarkers should use a List of MarkerOptions, my bad!

Just one remark to the commit:
there is a legitimate reason why I was doing this

Marker m;
int count = markers.size();
for (int i = 0; i < count; i++) {
   m = markers.get(i);
   m.setId(ids[i]);
   m.setMapView(this);
   mAnnotations.add(m);

Instead of:

for (int i = 0; i < markers.size(); i++) {
    markers.get(i).setId(ids[i]);
    markers.get(i).setMapView(this);
    mAnnotations.add(markers.get(i));
}

The whole reason behind this is that this code needs to be scalable, eg iOS sample is adding 1000 markers. In second code snippet this will result in a lot of garbage objects while in first snippet this will not.

Jake Wharton has created a nice presentation on this subject.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 6, 2015

@tobrun Ah interesting. I'll copy your code to the other methods then.

I also optimised our ArrayList constructors to pre-allocate the correct amount of memory.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 8, 2015

@tobrun I changed to use the faster for loop for markers, and polylines.

I changed to use shallow List copies.

Finally I renamed getAnnotationsInBounds to getMarkersInBounds and changed the code that uses it to talk about "markers" instead of "annotation" just to make it clearer that we only consider markers when selecting.

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

ljbade commented Oct 8, 2015

Changed the milestone as I think this is something we should get in to v2.1.0 so that I can JavaDoc the tidier functions.

ljbade pushed a commit that referenced this issue Oct 8, 2015
ljbade pushed a commit that referenced this issue Oct 9, 2015
@ljbade ljbade removed the in progress label Oct 9, 2015
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this issue Nov 6, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android refactor
Projects
None yet
Development

No branches or pull requests

2 participants