Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep object order in unmodifiable collection returned by MapObjectMan… #1008

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Keep object order in unmodifiable collection returned by MapObjectMan… #1008

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/src/main/java/com/google/maps/android/collections/MapObjectManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public void clear() {
}

protected java.util.Collection<O> getObjects() {
return Collections.unmodifiableCollection(mObjects);
return Collections.unmodifiableSet(mObjects);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@jeffdgr8 jeffdgr8 Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}