-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Add ability to create games sign in #511
Add ability to create games sign in #511
Conversation
b0a7574
to
b318fc8
Compare
b318fc8
to
b0aead4
Compare
There was a problem hiding this 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!
/// Equivalent to GoogleSignInOptions.DEFAULT_SIGN_IN | ||
static const String defaultSignInOptions = "defaultSignIn"; | ||
|
||
/// Equivalent to GoogleSignInOptions.DEFAULT_GAMES_Sign_In |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be DEFAULT_GAMES_SIGN_IN
static const String defaultGamesSignInOptions = "defaultGamesSignIn"; | ||
|
||
/// The options to pass to GoogleSignInOptions.Builder() | ||
final String builderOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is named after an implementation detail on the Android side (the builder). Maybe rename to defaultConfiguration
, signInOptions
or similar? Renaming should be done also for the MethodChannel argument name.
The String type may be too wide. Perhaps use an enum instead?
@@ -142,7 +151,21 @@ class GoogleSignIn { | |||
/// The [hostedDomain] argument specifies a hosted domain restriction. By | |||
/// setting this, sign in will be restricted to accounts of the user in the | |||
/// specified domain. By default, the list of accounts will not be restricted. | |||
GoogleSignIn({this.scopes, this.hostedDomain}); | |||
GoogleSignIn({this.builderOptions, this.scopes, this.hostedDomain}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to assert that the builderOptions
has a valid value. (With an enum rather than a String we don't need that.)
@@ -380,6 +403,7 @@ private void onSignInResult(GoogleSignInResult result) { | |||
if (result.isSuccess()) { | |||
GoogleSignInAccount account = result.getSignInAccount(); | |||
currentAccount = account; | |||
registrar.publish(account); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were planning to deprecate the value publishing API. Can't potential clients use the GoogleSignIn API directly to get the account?
new GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_SIGN_IN).requestEmail(); | ||
GoogleSignInOptions.Builder optionsBuilder; | ||
|
||
if (builderOptions == null) builderOptions = DEFAULT_SIGN_IN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be necessary, as we ensure the parameter has a non-null value on the Dart side.
@mravn-google Thanks for the review. I believe I addressed all of your notes. Swapping to a direct API call in my downstream plugin worked. Let me know if there's anything else I can clean up. |
4bd76da
to
f66a147
Compare
result([FlutterError errorWithCode:@"missing-config" | ||
message:@"GoogleService-Info.plist file not found" | ||
if ([((NSNumber *)call.arguments[@"signInOption"]) isEqualToNumber:[NSNumber | ||
numberWithInt:1]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe simpler with
NSNumber* signInOption = call.arguments[@"signInOption"];
if ([signInOption intValue] == 1) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Added some additional comments.
@@ -127,6 +129,9 @@ class GoogleSignIn { | |||
static const MethodChannel channel = | |||
const MethodChannel('plugins.flutter.io/google_sign_in'); | |||
|
|||
/// The options to pass to GoogleSignInOptions.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not mention the Android API's builder here, but instead document what SignInOption
is, in its own right. And we should document that gamesSignIn
isn't supported on iOS.
GoogleSignIn({this.scopes, this.hostedDomain}); | ||
GoogleSignIn({this.signInOption, this.scopes, this.hostedDomain}); | ||
|
||
/// Factory for creating default sign in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Dartdoc opening paragraph should end in a .
Same for GoogleSignIn.gamesSignIn
below.
hostedDomain: hostedDomain); | ||
} | ||
|
||
/// Factory for creating sign in suitable for games |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document that this does not work on iOS.
@@ -13,6 +13,8 @@ import 'src/common.dart'; | |||
export 'src/common.dart'; | |||
export 'widgets.dart'; | |||
|
|||
enum SignInOption { defaultSignIn, gamesSignIn } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call these standard
and games
. The SignIn
part is already given by the enum type name.
GoogleSignIn({this.signInOption, this.scopes, this.hostedDomain}); | ||
|
||
/// Factory for creating default sign in | ||
factory GoogleSignIn.defaultSignIn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename the factory constructors GoogleSignIn.standard
and GoogleSignIn.games
, again to avoid repeating the SignIn
part in client code.
@@ -173,7 +192,8 @@ class GoogleSignIn { | |||
|
|||
Future<void> _ensureInitialized() { | |||
if (_initialization == null) { | |||
_initialization = channel.invokeMethod("init", <String, dynamic>{ | |||
_initialization = channel.invokeMethod('init', <String, dynamic>{ | |||
'signInOption': (signInOption ?? SignInOption.defaultSignIn).index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe keep this as a String in the channel communication and on the platform side. 0 and 1 are just magic constants otherwise, with non-obvious relationship to the Dart enum.
5338b6a
to
e65ec7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks a lot! Can I ask you to address the compile error and formatting discrepancies reported by Travis? |
message:@"Games sign in is not supported on iOS" | ||
details:nil]); | ||
NSString *signInOption = call.arguments[@"signInOption"]; | ||
if ([signInOption isEqualToString] == @"SignInOption.games") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. You meant [sigInOption isEqualtoString:@"SignInOption.games"]
:-)
e136849
to
fd38c40
Compare
Published after #519 |
This introduces an additional
init
argument so that a games sign in experience can be created in Android. The native iOS code will return an error if attempting to create a games sign in.Additionally, the Android plugin publishes the
GoogleSignInAccount
so that it can be accessed by other native code.