Manually manage GoogleApiClient #148
Conversation
|
/cc @mehmetf, @collinjackson, @tvolkert |
| @@ -1,3 +1,9 @@ | |||
| ## 0.2.1 | |||
There was a problem hiding this comment.
@collinjackson Is the codelab locked to a plugin version? Do I need to update anything in the codelab regarding flutter/flutter#10688?
|
I'll update the codelab to the latest after you land this. |
| new GoogleSignInPlugin(fragmentActivity, new BackgroundTaskRunner(1), REQUEST_CODE); | ||
| new GoogleSignInPlugin(activity, new BackgroundTaskRunner(1), REQUEST_CODE); | ||
| registrar.addActivityResultListener(instance); | ||
| registrar.publish(instance); |
There was a problem hiding this comment.
Do we plan on anything extracting this instance via PluginRegistry.valuePublishedByPlugin()?
There was a problem hiding this comment.
Got catch. That line was left over from some experimenting I did. Removed.
| result.startResolutionForResult(activity, REQUEST_CODE_RESOLVE_ERROR); | ||
| } catch (SendIntentException e) { | ||
| // There was an error with the resolution intent. Try again. | ||
| googleApiClient.connect(); |
There was a problem hiding this comment.
Is there any reason we'd want to set resolvingError to false again here? How do we break out of the failure loop? Eventually, it seems like we'd want to call finishWithError()
There was a problem hiding this comment.
You're right. I changed it to report the error.
| result.getErrorCode(), | ||
| REQUEST_CODE_RESOLVE_ERROR, | ||
| new DialogInterface.OnCancelListener() { | ||
| public void onCancel(DialogInterface dialog) { |
There was a problem hiding this comment.
Does the user have any choice other than to cancel?
There was a problem hiding this comment.
Depends on the error. One example: if the error is "Your play services are out of date", there is a button to trigger installation of the play services. Once this is completed, we will be informed about this via the onActivityResult callback.
| REQUEST_CODE_RESOLVE_ERROR, | ||
| new DialogInterface.OnCancelListener() { | ||
| public void onCancel(DialogInterface dialog) { | ||
| if (pendingOperation != null && pendingOperation.method.equals(METHOD_INIT)) { |
There was a problem hiding this comment.
If we're not in the middle of an init, should we try to reconnect?
There was a problem hiding this comment.
If we bring up an ErrorDialog, that usually means the user has to take action to fix it. If he/she cancels out of fixing the error, there is no point in attempting a re-connect. We'll just end up in the same error state.
| if (!googleApiClient.isConnecting() && !googleApiClient.isConnected()) { | ||
| googleApiClient.connect(); | ||
| } | ||
| } else if (pendingOperation != null && pendingOperation.method.equals(METHOD_INIT)) { |
There was a problem hiding this comment.
In this case, should we finishWithError() no matter what the state of pendingOperation?
There was a problem hiding this comment.
This code path can only be active during on "init" step (when the dart side told us to connect to the GoogleAPI) or when the activity is returning active after a connection had been established before. In the former case we need to tell the dart side that their requested action (init) failed. In the later case there is no pending operation (since the reconnect is implicit) and there is nobody listening on the native side to receive this error.
|
|
||
| @Override | ||
| public void onActivitySaveInstanceState(Activity activity, Bundle outState) { | ||
| outState.putBoolean(STATE_RESOLVING_ERROR, GoogleSignInPlugin.this.resolvingError); |
There was a problem hiding this comment.
You shouldn't need the GoogleSignInPlugin.this.
| @Override | ||
| public void onActivityStopped(Activity activity) { | ||
| if (activity == GoogleSignInPlugin.this.activity && googleApiClient != null) { | ||
| googleApiClient.disconnect(); |
There was a problem hiding this comment.
Is there any harm in calling disconnect() if we're not connected? If so, then we should check the connection state here.
There was a problem hiding this comment.
It's a no-op if no connection has been established.
| _addMethodCall('signInSilently'); | ||
| Future<GoogleSignInAccount> signInSilently() { | ||
| return _addMethodCall('signInSilently').catchError((dynamic _) { | ||
| // ignore, we promised to be silent. |
There was a problem hiding this comment.
I don't think we want to swallow all errors here. The problem was that the OEM code was throwing too aggressively. Once that's fixed, I think we want to propagate normal exceptions here.
There was a problem hiding this comment.
Added a TODO to revisit when we fix the OEM side.
| @@ -1,3 +1,9 @@ | |||
| ## 0.2.1 | |||
|
|
|||
| * Plugin can (once again) be used in apps that extend `FlutterActivity` | |||
There was a problem hiding this comment.
You can remove the dependency on com.android.support from build.gradle now as well.
|
@tvolkert PTAL. |
* removed auto-manage * ++ * clean-up * small fixes * Add changelog * review comments
4 participants
This allows users to use the sign-in plugin in a regular
FlutterActivity(instead of just aFlutterFragmentActivity)Fixes flutter/flutter#10688.
Fixes flutter/flutter#10690.
In addition to that, this also includes tiny fixes for problems I discovered along the way:
signInSilentlywill now never throw (after all, an exception is not really silent).initstep fails, we'll now attempt to retry init when a new sign-in is triggered. (Previously, if init failed for any reason, you had to restart the app to try signing-in again).