Skip to content

Add documentation for GoogleApiHelper#getSafeAutoManageId() and do a teeny bit of cleanup in the auth README #575

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.

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

Closed
wants to merge 2 commits into from

Conversation

SUPERCILEX
Copy link
Collaborator

@samtstern I recently added Firebase app indexing to my app and noticed that it was crashing if I tried to sign in or out. Turns out I was duplicating enableAutoManage ids with FirebaseUI so I think we should make GoogleApiHelper#getSafeAutoManageId() part of the public api so people can use it to get around this problem.

@@ -352,7 +370,7 @@ represented in the following diagram:

![FirebaseUI authentication flow on Android](flow.png)

### UI customization
## UI customization
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This header is part of the Table of Contents so it should be on the same level as configuration and using-firebaseui-for-authentication.

auth/README.md Outdated
help prevent memory leaks. However, using `enableAutoManage` can be risky because the method will throw
an `IllegalStateException` if another `GoogleApiClient` is being managed with the same id.
To help solve this problem, FirebaseUI provides a helper method to get a safe id that is
not being used: `GoogleApiHelper#getSafeAutoManageId()`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@samtstern I think the api is stable enough for this, do you agree?

@samtstern
Copy link
Contributor

Hey @SUPERCILEX I don't think this should be part of the public API for a few reasons:

  1. It's tangential to our library's core function, so if this was a pattern we wanted people to use we'd do better to just publish a snippet somewhere (Github Gist, StackOverflow, etc)
  2. It's really just wrapping AtomicInteger, so we're not providing much value
  3. The GoogleApiClient engineers made a decision to not just auto-increment IDs internally so that users would have to track their clients, so while for us this pattern works it's not something we want everyone to do without thinking.

Gonna close this one since the other README fixes can be part of another PR, hope that makes sense!

@samtstern samtstern closed this Feb 6, 2017
@SUPERCILEX
Copy link
Collaborator Author

@samtstern your arguments totally make sense on not making it part of the public API, but as a counter proposal I think we should at least start the atomic integer with a value of 10 or something. What do you think? This should remove the need for devs to use getSafeAutoManageId() because we don't start with the default ID anymore. Also, I didn't think about this but we weren't using auto manage before so this is technically breaking backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants