Move GoogleApiClient.Builder to onStart/Stop methods #10523
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generated by 🚫 dangerJS |
|
You can test the changes on this Pull Request by downloading the APK here. |
9 tasks
develric
reviewed
Sep 29, 2019
| public void onStop() { | ||
| super.onStop(); | ||
| mGoogleApiClient.stopAutoManage(getActivity()); | ||
| if (mGoogleApiClient != null && mGoogleApiClient.isConnected()) { |
There was a problem hiding this comment.
Maybe we can move the null check on mGoogleApiClient a level above so to protect also the access to stopAutoManage. wdyt?
There was a problem hiding this comment.
you're right. I've originally had the full check there but it was crashing. In this case we should split the check.
develric
reviewed
Sep 29, 2019
| public void onStop() { | ||
| super.onStop(); | ||
| mGoogleApiClient.stopAutoManage(getActivity()); | ||
| if (mGoogleApiClient != null && mGoogleApiClient.isConnected()) { |
There was a problem hiding this comment.
Same comment as here. Maybe I would also consider to add same null check logic at line 230 for consistency. Let me know what you think 😊.
develric
reviewed
Sep 29, 2019
There was a problem hiding this comment.
I also was not able to reproduce the error, but moving the code to onStart/onStop as you did in your PR but leaving the stopAutoManage inside the isConnected check I was able to get (even if not in a reproducible and always consistent way) conditions in which the client was not connected and got the exception.
I tested both the login and signup flows as you suggested: I did not get errors and was able to complete the flows.
I left a couple of comments for your review, but I agree with your analysis and proposed solution (good catch! 😎).
anitaa1990
added a commit
that referenced
this pull request
Nov 13, 2019
4b8e83512d Added logic to redirect to discovery error if xmlrpc is blocked on site 2c3223bbe7 Reverted logic to check for Jetpack availability during discovery errors since it can return a false negative 846dc2ba99 Handle site not found discovery error which can occur even with Jetpack available e1028df7f4 Modified logic to check for discovery errors first before checking for Jetpack 16c1bbe249 Modified error message for xmlrpc errors 732a0486e7 Modified logic to initiate discovery from LoginBaseDiscoveryFragment fdfb9df9b7 Added logic to redirect to discovery error screen, even if Jetpack is available 0833d5ca77 Add flag to check if Jetpack is available if a discovery error takes place aa7f8cc9d9 Added logic to port over changes from the LoginSiteAddressFragment to utilise LoginBaseDiscoveryFragment e1f2ff2d37 Modified discovery error listener to handle the various error scenarios from the calling fragment a1bc462454 Added option to verify email only if login is for self hosted credentials c2f2bba7a8 Added new string labels for magic link sent label b4b9dbcabb Merge branch 'feature/sign-in-with-self-hosted-credentials' into issue/1482-step2-magic-link-signin-changes 32d2c25fd9 Handle corner case in discovery process and redirect to email screen if site is wp.com d96252d875 Fixed nitpicks in code/xml 9b227c817b Design changes for request magic link screen a8712882c1 Hide keyboard before redirecting to the magic link/no jetpack screen e4e8210da2 Added logic to initiate discovery process for xmlrpc endpoint 2c35450824 Added logic to pass the input site address to forgot password flow, if the xmlrpc url is empty f9ff33ec87 Added separate method to redirect to username screen in LoginListener f71a4065b0 Revert discovery initiation changes to the site fragment 1e3e9cf367 Added logic to fetch SiteModel based on the site url 71ec9bdaec Added logic to redirect to no Jetpack screen when site credentials are entered ad42794306 Revert adding a new method to LoginListener, instead use an existing listener method 8f3649b4b5 Added logic to fetch jetpack user email from site credentials and redirect to magic link screen aa0d39d792 Pass site xmlrpc url to email and jetpack required screen 360480c6ff Initiate discovery process even if jetpack is not installed/active/connected 39b0ee010f Added logic to redirect Woo users to email login or jetpack required screen efd18a8cef Revert the connection info endpoint implementation and update error message when site is not found 96e0ff714f Added label to the top of the LoginUsernamePasswordFragment for Woo login only 082d1e1304 Display login with site credentials link button in login email fragment 27b07bf3e1 Merge commit 'f89f408ee83d29d59dfe5740b2ba26c51dcd00df' into issue/1446-login-lib-merge 2406304ffa Merge pull request #27 from wordpress-mobile/google_login_fix 18ddefd54b Merge pull request #10523 from wordpress-mobile/feature/move_api_client_builder_to_start_stop a87f1ec203 Update Jitpack dependency urls to use the www subdomain 24bb535488 Implement PR comments b63bb72af9 Remove api client cleanup from onDetach bd3410e2ad Move GoogleApiClient.Builder to onStart/Stop methods d70141070b Add commit message for jetpack validation fix 80b21579cf Use only the isJetpackConnected property for jetpack validation c3f9b19157 Merge pull request #10321 from wordpress-mobile/login/fix-login-tracks 0b734cf390 Merge pull request #10270 from wordpress-mobile/issue/9720-login-subtree-update-squashed eb90f72905 Merge pull request #1312 from woocommerce/merge-login-lib bd6086802b Merge commit 'aec4a8317d3188866c02556c182cd8d09d9b0272' into merge-login-lib 378a45be3e Fix leak of anonymous OnCancelListener reference 71cfd27b21 Merge branch 'develop' into amanda/leak_canary 42cf2d56ec Merge pull request #1292 from woocommerce/release/2.3 67de44ca60 Merge branch 'release/2.3' into merge-release/2.2.1-release/2.3 227c48b323 Merge pull request #1277 from woocommerce/merge/WordPress-Login-Flow-Android/24 0123a10b6b Reverted changes to login build.gradle ff16da7e70 Reverted back to SDK 28 (we can update to 29 once Android Q is out of beta) d433fad0df Merge commit '9bb4617dddea713295ce4b53a60aa2a271f318a2' squashed b0b4f17f8a Merge commit 'f6c4fbfadce780aaebc745154c29affe46978545' into issue/9720-login-subtree-update-squashed 66695843b0 Upgraded login library to SDK 29 ff4a69e142 Add leak canary and fix leaks git-subtree-dir: libs/login git-subtree-split: 4b8e83512da7628a11c4b075cf79ac6e3b9864ec
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10357
I think (and I might be wrong) that there are 2 possible issues. One is that sometimes
onDestroyandonDetachare not called. The other one is that in theSignupEmailFragmentwe were checking whether the client is connected before we calledstopAutoManageand I don't think that's always the case (I was able to make that crash after moving the logic toonStartandonStop).This PR is not really based on evidence so I might be very wrong :-).
To test:
Update release notes:
RELEASE-NOTES.txtif necessary.