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

AnnotationOptions.alpha Needs To Return SuperType #2631

Closed
bleege opened this issue Oct 15, 2015 · 8 comments
Closed

AnnotationOptions.alpha Needs To Return SuperType #2631

bleege opened this issue Oct 15, 2015 · 8 comments
Assignees
Labels
Android Mapbox Maps SDK for Android

Comments

@bleege
Copy link
Contributor

bleege commented Oct 15, 2015

Currently AnnotationOptions.alpha() returns an AnnotationsOptions object. While this is functional, it requires casting to be applied in common Builder patterns which isn't good. For example:

MapView map = ...;

// works
map.addPolygon(new PolygonOptions().add(latLngs));

// currently doesn't work due to typing, but is ideal
map.addPolygon(new PolygonOptions().add(latLngs).alpha(0.5f));

// casting needed to make work, but not something that should be used 
map.addPolygon((PolygonOptions) new PolygonOptions().add(latLngs).alpha(0.5f));

/cc @tobrun @ljbade

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

ljbade commented Oct 15, 2015

Interesting, the other options support this. Also core GL marker don't support alpha, so only needed for lines and polygons.

ljbade pushed a commit that referenced this issue 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 #2631
Fixes #2448
@ljbade ljbade self-assigned this Oct 16, 2015
@tobrun
Copy link
Member

tobrun commented Oct 16, 2015

@bleege didn't the generic builder pattern solve this?

@bleege bleege assigned bleege and unassigned ljbade Oct 16, 2015
@bleege
Copy link
Contributor Author

bleege commented Oct 16, 2015

@tobrun I didn't have time to finish yesterday afternoon so I just created this issue to make sure this was known. Hoping to get it to today.

@bleege
Copy link
Contributor Author

bleege commented Oct 16, 2015

Looking at Google's implementation they avoided this problem all together by keeping PolygonOptions, PolyLineOptions, and MarkerOptions all independent. In other words, they don't use DRY and each one of these Option classes extends from java.lang.Object and implements android.os.Parcelable.

@bleege
Copy link
Contributor Author

bleege commented Oct 16, 2015

Doing some R&D on this shows that the AnnotationsOptions abstract class came as part of clean up / refactoring in #1716. In other words this was introduced in Mapbox and not needed as part of the Google API (confirms previous statement on API).

@ljbade
Copy link
Contributor

ljbade commented Oct 16, 2015

@bleege Does the same apply to the Marker, Polyline, Polygon classes? If we split these up we will need to do a bit of re-factoring in MapView.

Also should point out this was an easy fix in my large PR, I just pushed .alpha to the concrete classes.

@bleege
Copy link
Contributor Author

bleege commented Oct 16, 2015

I decided to remove AnnotationsOptions and MultiPointOptions as they were for abstract classes that are not part of the Google Maps API. I moved their features to concrete implementations which mirrors Google and also solves this issue. Tests (see below) look good. I also added an explicit usage of .alpha() to the Polygon Annotation. Was able to confirm that the builder pattern worked code wise and more importantly the polygon actually was rendered using the proper alpha.

device-2015-10-16-174451
Test, Test, Test

device-2015-10-16-174638
Original Polygon, No Alpha

device-2015-10-16-174516
Original Polygon, 0.5 Alpha

bleege added a commit that referenced this issue Oct 16, 2015
@bleege
Copy link
Contributor Author

bleege commented Oct 16, 2015

Rebased and merged.

@bleege bleege closed this as completed Oct 16, 2015
bleege added a commit that referenced this issue Oct 20, 2015
bleege added a commit that referenced this issue Oct 22, 2015
ljbade pushed a commit that referenced this issue Oct 22, 2015
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
…ions in favor of concrete implementation
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this issue Nov 6, 2015
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this issue Nov 6, 2015
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.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

No branches or pull requests

4 participants