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

Jump to bottom

fix: MapObjectManager.Collection object order #972

Merged
merged 2 commits into from
Sep 14, 2021
Merged

fix: MapObjectManager.Collection object order #972

merged 2 commits into from
Sep 14, 2021

Conversation

jeffdgr8
Copy link
Contributor

Thank you for opening a Pull Request!

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
  • Will this cause breaking changes to existing Java or Kotlin integrations? If so, ensure the commit has a BREAKING CHANGE footer so when this change is integrated a major version update is triggered. See: https://www.conventionalcommits.org/en/v1.0.0/

Fixes #772 🦕

dependabot bot and others added 2 commits September 14, 2021 12:25
@dependabot @jeffdgr8
chore(deps): bump play-services-base from 17.2.1 to 17.6.0 (googlemap…
751277c 
…s#846)

Bumps play-services-base from 17.2.1 to 17.6.0.

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@jeffdgr8
Use LinkHashSet to maintain set order
6400768 
Closes googlemaps#772
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 14, 2021
@jeffdgr8 jeffdgr8 mentioned this pull request Sep 14, 2021
Closed
barbeau
barbeau reviewed Sep 14, 2021
View reviewed changes
Copy link
Collaborator

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

@jeffdgr8 Thanks for working on this! A few comments in-line.

library-v3/build.gradle
@@ -56,7 +56,7 @@ dependencies {
implementation 'com.google.android.libraries.maps:maps:3.1.0-beta'
implementation 'com.android.volley:volley:1.2.1' // TODO - Remove this after Maps SDK v3 beta includes Volley versions on Maven Central
implementation 'com.google.android.gms:play-services-basement:17.6.0'
implementation 'com.google.android.gms:play-services-base:17.2.1'
implementation 'com.google.android.gms:play-services-base:17.6.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this dependency change needed? If not, could you please revert for this PR, and if so, could you please explain why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came over from this commit on the old master branch. It's the only of the 5 commits ahead of the new main branch master had that haven't since been updated to newer dependencies already on main. Looks like the chore bot is now doing those dependency updates on main. Might be able to delete the master branch now entirely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, strange - thanks for pointing that out. Not sure why dependabot didn't bump that in main.

@arriolac thoughts on removing master branch entirely? It might reduce confusion for folks that have it cloned locally on their machine who haven't realized that the main branch switched to main.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ to deleting master completely to avoid any confusion. Will go ahead and do that.

library/src/main/java/com/google/maps/android/collections/MapObjectManager.java
@@ -24,7 +24,7 @@

import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should mNamedCollections and mAllObjects on lines 43/44 be changed to LinkedHashMaps for predictable order as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just confirmed that neither of those collections are able to be iterated over, so consistent ordering should never be an issue there. Only get(), put(), and remove() operations are performed on them internally and there's no API that returns these collections to an external caller.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks, that looks right.

barbeau
barbeau approved these changes Sep 14, 2021
View reviewed changes
Copy link
Collaborator

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @jeffdgr8!

@barbeau barbeau merged commit e6276a9 into googlemaps:main Sep 14, 2021
@barbeau barbeau mentioned this pull request Sep 14, 2021
Closed
@jeffdgr8 jeffdgr8 deleted the use-linkedhashset branch September 15, 2021 00:54
googlemaps-bot pushed a commit that referenced this pull request Sep 15, 2021
@semantic-release-bot
chore(release): 2.2.6 [skip ci]
94663c3 
## [2.2.6](v2.2.5...v2.2.6) (2021-09-15)

### Bug Fixes

* MapObjectManager.Collection object order ([#972](#972)) ([e6276a9](e6276a9)), closes [#846](#846)
* Update appcompat to avoid "OnClickXmlDetector called context.getMainProject()" lint error ([#974](#974)) ([dbfdbc7](dbfdbc7))
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 2.2.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@solcott solcott mentioned this pull request Oct 12, 2021
Open
@barbeau barbeau mentioned this pull request Oct 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arriolac arriolac arriolac left review comments

@barbeau barbeau barbeau approved these changes

Assignees

@barbeau barbeau

Labels
cla: yes This human has signed the Contributor License Agreement. released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Object order in MapObjectManager.Collection is random
4 participants
@jeffdgr8 @googlemaps-bot @arriolac @barbeau