Keep object order in unmodifiable collection returned by MapObjectMan…

[solcott]

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

[google-cla]

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).

📝 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!

[barbeau]

@solcott Thanks for the PR!

Could you explain more why this is needed? Did you still encounter the same issue #772 even after PR #972 was merged?

If so it would be useful to see some code that's generating the error, ideally in the form of unit tests.

My understanding of Collections.unmodifiableX() is that it's just a wrapper around the underlying data structure so it should still respect the ordering enforced by the implementation, in our case the LinkedHashSet.

Collection.iterator and Set.iterator both have effectively the same language in the Javadocs:

There are no guarantees concerning the order in which the elements are returned (unless this collection is an instance of some class that provides a guarantee).

cc @jeffdgr8 as the contributor of PR #972

[jeffdgr8]

Looking at the source for UnmodifiableSet, it subclasses UnmodifiableCollection, additionally overriding equals() and hashCode() to delegate to the wrapped collection.

I would not expect this change to affect the iteration order, which should have been resolved in my PR. But I would expect this change to allow for comparisons between multiple versions of the wrapped collection itself. For example:

...
PolygonManager.Collection polygonCollection = polygonManager.newCollection();
Collection<Polygon> collection1 = polygonCollection.getPolygons();
Collection<Polygon> collection2 = polygonCollection.getPolygons();
Set<PolygonManager.Collection> polygonCollectionSet = new HashSet<>();
polygonCollectionSet.add(collection1);

// currently each of these may be false, but this change should make them true:
collection1.equals(collection2);
polygonCollectionSet.contains(collection2);

@solcott do you have a similar usage that requires comparisons of these MapObject.Collections for equality?

I think this is a good change either way, as it would allow for these kinds of comparisons. Collection itself doesn't stipulate a contract for equals() beyond Object.equals, but Set does, which is the underlying collection we're using here.

If this is the contract we want to support and there are use cases for it, it may make sense to change the signature of getObjects() and related functions that call it to explicitly return a Set rather than the more general Collection type. That way it would enforce this behavior expectation at the API level. This would be a non-breaking safe API change, since Set is a Collection.

[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 #1008 (comment) 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 #1008 (comment) 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 #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 #1008 (comment) 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 #1008 (comment) that currently fails on the main branch but passes after this change.