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

fixes #5238: Android marker view z-ordering #5862

Closed
wants to merge 4 commits into from

Conversation

incanus
Copy link
Contributor

@incanus incanus commented Aug 3, 2016

First phase of #5238, allowing addition of marker views meant to always be above another, named marker view.

I'll squash etc. once this looks good to go.

@tobrun @zugaldia

@incanus incanus added feature Android Mapbox Maps SDK for Android labels Aug 3, 2016
@incanus incanus changed the title fixes #5238 Android marker view z-ordering fixes #5238: Android marker view z-ordering Aug 3, 2016
@incanus
Copy link
Contributor Author

incanus commented Aug 4, 2016

Something that occurred to me here is my relative inexperience with memory management in Java, so I would welcome thoughts on any sort of issues with aboveMarkerView references as a result of this.

@tobrun
Copy link
Member

tobrun commented Aug 4, 2016

@incanus this is great! just one remark is that the builder creating the MarkerView must comply to the Parceable interface. The Parceable interface is used to restore the builder and its variables after a configuration change (eg. orientation change). At this point the MarkerView itself does not implement the Parceable interface and we won't be able to recreate this interaction correctly (the set MarkerView will be null).

There are a couple of ways to workaround this:

  • expose the functionality only on MarkerView (it's the user responsibility to restore correctly)
  • expose a method on MarkerViewManager that takes in 2 MarkerViews
  • only save the id of the MarkerView and lookup the concrete MarkerView when needed through MarkerViewManager

@incanus
Copy link
Contributor Author

incanus commented Aug 4, 2016

only save the id of the MarkerView and lookup the concrete MarkerView when needed through MarkerViewManager

I'm not used to objects having this, what sounds like a global id, that can be used. I'll look into this.

@tobrun
Copy link
Member

tobrun commented Aug 4, 2016

only save the id of the MarkerView and lookup the concrete MarkerView when needed through MarkerViewManager

Now thinking of this, this won't be an option since the id will only be available after we have added the marker to the map (this id would be equal to the core gl marker id).

update: I now took an actual look in the code and it would be feasible to do this as a part of addMarker.

@incanus
Copy link
Contributor Author

incanus commented Aug 4, 2016

Backing this up a moment @tobrun, I am seeing things ordered ok after device orientation change. Is this truly a problem?

@incanus
Copy link
Contributor Author

incanus commented Aug 11, 2016

@tobrun and I voiced on this at length yesterday and worked through some consideration of both an API design like I've proposed as well as a callback-based sort comparator. The short is that it probably makes sense to skip right to the comparator as fetching it from a callback is a lesser consideration on performance than the actual sorting. Additionally, we could add Parcelable to Marker in order to make the proposed solution work, but the API is still limiting in referring to only one other marker, when marker add order needs to remain flexible.

I will start on this soon, but it's not as high priority right now.

@incanus incanus self-assigned this Aug 11, 2016
@tobrun tobrun added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Aug 13, 2016
@incanus
Copy link
Contributor Author

incanus commented Aug 15, 2016

Closing this PR per #5862 (comment) since we're go the comparator route instead.

@incanus incanus closed this Aug 15, 2016
@incanus incanus deleted the 5238-android-marker-view-z-ordering branch August 15, 2016 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants