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

[android] Refactor annotations API #2637

Merged
merged 1 commit into from
Oct 22, 2015
Merged

[android] Refactor annotations API #2637

merged 1 commit into from
Oct 22, 2015

Conversation

ljbade
Copy link
Contributor

@ljbade ljbade commented Oct 16, 2015

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 #2448

@ljbade ljbade added the Android Mapbox Maps SDK for Android label Oct 16, 2015
@ljbade ljbade added this to the android-v2.1.0 milestone Oct 16, 2015
@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

There is a lot going on in this commit, but the result is an annotations API that only exposes what we have implemented. It also improves parts of the code, and fixes a bug that had not been ticketed before.

I will aim to get this merged tonight, ready for Sirius build tomorrow.

I have carefully tested everything with the TestApp and there should be no new bugs introduced.

/cc @bleege @tobrun

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

The only Travis failure was the known hang in the iOS tests. Restarting that job, if it still fails we can merge as this is an Android only change.

@tobrun
Copy link
Member

tobrun commented Oct 16, 2015

@ljbade
When doing a couple of test runs I was able to produce a crash.
I'm not able to reproduce it anymore, I believe I was too fast to press a marker before the infowindow was ready to handle the click event. This could well be a crash related to my code an not this PR. If you haven't picked this up tonight I will later this day.

java.lang.NullPointerException: Attempt to invoke virtual method 'void com.mapbox.mapboxsdk.annotations.Marker.showInfoWindow()' on a null object reference
at com.mapbox.mapboxsdk.views.MapView.adjustTopOffsetPixels(MapView.java:1978)
at com.mapbox.mapboxsdk.views.MapView.access$500(MapView.java:98)
at com.mapbox.mapboxsdk.views.MapView$1.onMapChanged(MapView.java:828)
at com.mapbox.mapboxsdk.views.MapView$3.run(MapView.java:2823)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5417)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

@tobrun That is a strange crash, I can't see how it gets a null since it checks for that specifically:

if (mSelectedMarker != null) {
            if (mSelectedMarker.isInfoWindowShown()) {
                mSelectedMarker.hideInfoWindow();
                mSelectedMarker.showInfoWindow();
            }
        }

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

Ah might be race condition between the UI thread and the main thread. Map change event comes from main thread, other changes such as touch input come from UI thread.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

I think this crash is from changes in #2627

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

No doesn't look like map change problem since we already forward them to the UI thread.

Something must be touching MapView from a non-UI thread when it shouldn't.

@tobrun If you can't reproduce again, I think we can chalk it up to your device playing weird. And merge.

@tobrun
Copy link
Member

tobrun commented Oct 16, 2015

@tobrun If you can't reproduce again,
I think we can chalk it up to your device playing weird. And merge.

@ljbade 👍

@bleege
Copy link
Contributor

bleege commented Oct 16, 2015

@ljbade This is a LARGE change so close to launch. I'm not comfortable merging this into master right now. Let's bump this to android-v2.2.0 to give us more time.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

Sure, should we quickly add some do not use or not implemented javadoc to the annotation functions that don't do anything for 2.1.0 pending there removal?

@bleege
Copy link
Contributor

bleege commented Oct 16, 2015

should we quickly add some do not use or not implemented javadoc to the annotation functions that don't do anything for 2.1.0 pending there removal?

I was under the impression that these were going to be made private access? If they haven't already then let's do that.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

@bleege OK I'll make a version of this PR that includes just those changes (make non-functional methods private). I can't make all of the internal only methods private though (that was a big part of this refactor - to make it possible to make some of them private).

I'll include the .alpha fix too.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 16, 2015

@bleege The other thing I've been puzzling about is how Google keep the annotation classes in the map.model package which is separate from the GoogleMap object in map. They manage to do this without exposing any internal APIs.

Do they perhaps strip internal functions from the SDK before the publish? This is how they do it in the Android framework.

@ljbade ljbade force-pushed the 2546-fix-annotations branch 2 times, most recently from 863d68c to 16cf809 Compare October 22, 2015 08:46
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
@ljbade
Copy link
Contributor Author

ljbade commented Oct 22, 2015

Finally got this branch up to date with v2.1.0 merge.

Waiting for Travis.

@ljbade
Copy link
Contributor Author

ljbade commented Oct 22, 2015

Only failing on iOS due to hang in location tests. Merging.

@ljbade ljbade merged commit 927b4c6 into master Oct 22, 2015
@ljbade ljbade deleted the 2546-fix-annotations branch October 22, 2015 09:26
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

Successfully merging this pull request may close these issues.

Make Annotation objects immutable Add setOnInfoWindowClickListener()
3 participants