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

[android] #2588 - Make getTopOffsetPixelsForAnnotationSymbol private. #2592

Merged
merged 1 commit into from
Oct 13, 2015
Merged

[android] #2588 - Make getTopOffsetPixelsForAnnotationSymbol private. #2592

merged 1 commit into from
Oct 13, 2015

Conversation

ljbade
Copy link
Contributor

@ljbade ljbade commented Oct 12, 2015

Make getTopOffsetPixelsForAnnotationSymbol private

Also:

@ljbade ljbade added the Android Mapbox Maps SDK for Android label Oct 12, 2015
@ljbade ljbade added this to the android-v2.1.0 milestone Oct 12, 2015
@ljbade ljbade mentioned this pull request Oct 12, 2015
Closed
@ljbade ljbade force-pushed the 2588-topoffset-private branch from e490829 to df92362 Compare October 12, 2015 05:55
@ljbade
Copy link
Contributor Author

ljbade commented Oct 12, 2015

@bleege @tobrun Can you review.

bleege
bleege reviewed Oct 12, 2015
View reviewed changes
...id/java/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/MarkerOptions.java
@@ -84,7 +89,7 @@ public boolean isVisible() {
}

public MarkerOptions position(LatLng position) {
((Marker)annotation).position = position;
((Marker)annotation).setPosition(position);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause breakages with JNI as described in #2551.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 12, 2015

@bleege That's what I thought, but it didn't seem to break JNI? Also it's not that change that would break, it would be changing the inner fields of Marker, JNI doesn't use the functions.

I tested it both with the position and sprite fields as package private (current definition) vs private and it made no difference. I think JNI does not obey Java visibility rules and can grab whatever fields it wants to.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 12, 2015

Also noting that this fixed a bug in Marker.setPosition.

Going to do another test with full make clean and rebuild on my phone to double check JNI, then will merge.

@ljbade ljbade added the bug label Oct 12, 2015
@ljbade ljbade force-pushed the 2588-topoffset-private branch from df92362 to 67f0108 Compare October 13, 2015 01:19
@ljbade
Copy link
Contributor Author

ljbade commented Oct 13, 2015

Did more testing, JNI does not break. I can add and remove markers fine along with InfoWindows.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 13, 2015

Node is failing for some reason. As this is Android only change, merging.

@ljbade ljbade merged commit 67f0108 into master Oct 13, 2015
@ljbade ljbade deleted the 2588-topoffset-private branch October 13, 2015 01:50
@ljbade ljbade mentioned this pull request Oct 13, 2015
Closed
@bleege
Copy link
Contributor

bleege commented Oct 13, 2015

@ljbade Was the Node issue related to #2597?

@ljbade
Copy link
Contributor Author

ljbade commented Oct 13, 2015

@bleege Yeah looked like it, @mikemorris opened that after I came across it. I did try restarting the job several times keep hitting same node error.

@mikemorris
Copy link
Contributor

Node.js v4.2.1 landed today, should be all fixed now @ljbade

@ljbade
Copy link
Contributor Author

ljbade commented Oct 13, 2015

@mikemorris Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@ljbade ljbade

Labels
Android Mapbox Maps SDK for Android bug
Projects
None yet
Milestone
android-v2.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ljbade @bleege @mikemorris