Keep object order in unmodifiable collection returned by MapObjectMan…

[solcott]

Additional fix for #772 as it wasn't completely fixed by #972

[google-cla[bot]]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[solcott]

@googlebot I signed it!

[solcott]

I'm having trouble reproducing this in a test but I include some sample code that reproduces it on device for me.

Here is my use case:

I am working on an app that among other things allows users to manipulate property boundaries via dragging Markers. The app loads the property's boundary as a list of LatLng's which we add to the map using MarkerManager.

Here is an example of what I am doing that reproduces this issue:

val markerManager = MarkerManager(googleMap)
    val collection = markerManager.newCollection("polygon-points")
    val tempLatLngs = listOf<LatLng>(
      LatLng(-15.40554390831084,28.363610624581153), LatLng (-15.405650087831479,28.363583034345403), LatLng(-15.405614028392167,28.363463023931395), LatLng(-15.405504142821151,28.363480626265034), LatLng(-15.405495484176438,28.36349293560783), LatLng(-15.405525359257068,28.363604854657208)
    )
    Log.e("TestMarkerManager","Before: $tempLatLngs")
    tempLatLngs.forEach { latLng ->
      collection
        .addMarker(createMarkerOptions(latLng, polygonPinBitmap))
        .apply { tag = POLYGON_POINT_TAG }
    }
   Log.e("TestMarkerManager","After:  ${collection.markers.map { it.position }}")

After running this the following is logged to logcat:

Before: [lat/lng: (-15.40554390831084,28.363610624581153), lat/lng: (-15.405650087831479,28.363583034345403), lat/lng: (-15.405614028392167,28.363463023931395), lat/lng: (-15.405504142821151,28.363480626265034), lat/lng: (-15.405495484176438,28.36349293560783), lat/lng: (-15.405525359257068,28.363604854657208)]
After:  [lat/lng: (-15.405650087831479,28.363583034345403), lat/lng: (-15.405495484176438,28.36349293560783), lat/lng: (-15.405504142821151,28.363480626265034), lat/lng: (-15.405614028392167,28.363463023931395), lat/lng: (-15.405525359257068,28.363604854657208), lat/lng: (-15.40554390831084,28.363610624581153)]

Notice that the latlngs are not in same order as they were inserted.

Now if I change getObjects() to return Collections.unmodifiableSet(this.mObjects) and run it again the LatLngs are in the same order.

Before: [lat/lng: (-15.40554390831084,28.363610624581153), lat/lng: (-15.405650087831479,28.363583034345403), lat/lng: (-15.405614028392167,28.363463023931395), lat/lng: (-15.405504142821151,28.363480626265034), lat/lng: (-15.405495484176438,28.36349293560783), lat/lng: (-15.405525359257068,28.363604854657208)]
After:  [lat/lng: (-15.40554390831084,28.363610624581153), lat/lng: (-15.405650087831479,28.363583034345403), lat/lng: (-15.405614028392167,28.363463023931395), lat/lng: (-15.405504142821151,28.363480626265034), lat/lng: (-15.405495484176438,28.36349293560783), lat/lng: (-15.405525359257068,28.363604854657208)]

I'm also baffled at why this is the case after reading the javadoc for Collections.unmodifiableSet and Collections.unmodifiableCollection.

[jeffdgr8]

I took your sample code and tested it and found indeed there is something weird going on here, but it's actually a problem with the v2.2.6 build itself.

I was able to reproduce what you were seeing with v2.2.6, but not if I copied the MapObjectManager and MarkerManager source code directly to my app, unmodified. I then tested v2.3.0 and this version works as expected. Using the debugger again on v2.2.6, I see that the mObjects variable is actually a HashSet still!

For some reason, the v2.2.6 build that my PR's changes were released in does not have the change in the decompiled binary aar! It is in the source code jar though, which is even more confusing. And the change is in the v2.3.0 binary.

@arriolac @barbeau any ideas as to why the build would have come out like this? Is it possible other changes aren't making it into the binary output too, at least until the following release?

[solcott]

@jeffdgr8 I just tried 2.3.0 and it is working correctly for me. This PR can be closed.

[barbeau]

@solcott and @jeffdgr8 Thanks for the additional info and testing!

@solcott Even though this change shouldn't impact ordering, @jeffdgr8 makes a good argument in https://github.com/googlemaps/android-maps-utils/pull/1008#discussion_r736085968 for still including this change due to it's impact on .equals() and .contains().

Would you be willing to add a unit test based on the code @jeffdgr8 included in https://github.com/googlemaps/android-maps-utils/pull/1008#discussion_r736085968 that currently fails on the main branch but passes after this change?

@arriolac @barbeau any ideas as to why the build would have come out like this? Is it possible other changes aren't making it into the binary output too, at least until the following release?

@jeffdgr8 Thanks for pointing this out. I'm not sure why this would happen - we can take a closer look. I opened https://github.com/googlemaps/android-maps-utils/issues/1016.

[kikoso]

Hello @solcott , @barbeau . What is the current status of this Pull Request? Should we still add some Unit Tests to cover the failing test? Does it make sense to get this merged, since the issue seems to be gone in the current main branch?

[barbeau]

IIRC similar to my comment above, @jeffdgr8 made a good argument in https://github.com/googlemaps/android-maps-utils/pull/1008#discussion_r736085968 for still including this change due to it's impact on .equals() and .contains(), so we should add a unit test based on the code @jeffdgr8 included in https://github.com/googlemaps/android-maps-utils/pull/1008#discussion_r736085968 that currently fails on the main branch but passes after this change.