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

BaseMarkerOptions.java breaks NativeScript compatibility #4761

Closed
EddyVerbruggen opened this issue Apr 20, 2016 · 9 comments
Closed

BaseMarkerOptions.java breaks NativeScript compatibility #4761

EddyVerbruggen opened this issue Apr 20, 2016 · 9 comments
Labels
Android Mapbox Maps SDK for Android

Comments

@EddyVerbruggen
Copy link

Hi folks,

Since Mapbox for Android 4.0.0 the annotation logic has been partially moved to a new BaseMarkerOptions superclass. This class has a title property as well as a title setter.

I understand why it was changed in the way it is, but it breaks using markers for the Mapbox NativeScript plugin as the NativeScript runtime can't distinguish the property from the setter.

Platform: Android (NativeScript)
Mapbox SDK version: 4.0.0

Steps to trigger behavior

Please check my description in the NativeScript issue tracker for a way to reproduce this in a NativeScript app.

Possible solution

We don't need the current setters like title(..) removed, but we'd be very much helped if regular Java setters like setTitle are added to BaseMarkerOptions.java. Until that's added I'm afraid we can't update the Android version of our plugin.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Apr 20, 2016
@bleege
Copy link
Contributor

bleege commented May 24, 2016

@EddyVerbruggen I think this is doable as our primary concern is to be API compatible at the MarkerOptions level with the Google Maps API. We can create convenience wrapper methods for position, snippet, title, and icon called setPosition(), setSnippet(), setTitle(), and setIcon().

Is there any other API breakages that you know of now or is BaseMarkerOptions the only one?

/cc @rdlauer @tobrun @cammace @zugaldia

@EddyVerbruggen
Copy link
Author

Hey @bleege thanks for taking note! AFAIK BaseMarkerOptions is the only problem for us.

@bleege
Copy link
Contributor

bleege commented Jun 15, 2016

@EddyVerbruggen I just created some wrappers for this and submitted PR #5367. Can you verify that this is what you'll need for the Cordova plugin? If so @zugaldia can work with you to get this into the next 4.1.0 build. Thanks!

@bleege bleege added this to the android-v4.1.0 milestone Jun 15, 2016
zugaldia pushed a commit that referenced this issue Jun 16, 2016
@zugaldia
Copy link
Member

@EddyVerbruggen we've now merged @bleege's PR into the release branch. You can expect a release candidate tomorrow or early next week that you can use to test the fix. Thanks again for your feedback.

@EddyVerbruggen
Copy link
Author

Looks good guys, I'll check the RC shortly, thanks!

@zugaldia
Copy link
Member

zugaldia commented Jun 21, 2016

@EddyVerbruggen We released beta3 yesterday, feel free to give it a test #5415

@bleege
Copy link
Contributor

bleege commented Jun 23, 2016

Awesome! Thanks for making this happen @EddyVerbruggen @zugaldia! I'm excited about getting the Cordova and NativeScript plugins back up to the latest code! 🚀

@EddyVerbruggen
Copy link
Author

@bleege @zugaldia I waited a bit for 4.1.0 and had a great time implementing some new features for the NativeScript plugin today. Your setters worked perfectly, thanks a LOT!

The next thing I'll be adding is offline maps support, and then give the Cordova plugin some love.

@bleege
Copy link
Contributor

bleege commented Jul 8, 2016

@EddyVerbruggen That's great to hear! Thanks so much for your work keeping the NativeScript and Cordova plugins up to date! 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

No branches or pull requests

4 participants