[go: nahoru, domu]
Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod to 5.0 #2127

Merged
merged 10 commits into from
Oct 29, 2019
Merged

[google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod to 5.0 #2127

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
710283b
Fix Deprecated API Usage issue
collinjackson Sep 30, 2019
9b7eac5
Update to 5.0 compatibility
collinjackson Sep 30, 2019
89187b5
reformat
collinjackson Sep 30, 2019
46d23e3
Update to use [registrar messenger]
collinjackson Oct 1, 2019
f0bb3ce
Merge remote-tracking branch 'origin/master' into google_sign_in
collinjackson Oct 22, 2019
52ed349
Implement code review feedback
collinjackson Oct 22, 2019
34abc18
Remove dead code
collinjackson Oct 22, 2019
2faa78b
Merge remote-tracking branch 'origin/master' into google_sign_in
collinjackson Oct 25, 2019
bb5c3eb
format
collinjackson Oct 25, 2019
e3a79a8
Merge remote-tracking branch 'origin/master' into google_sign_in
collinjackson Oct 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/google_sign_in/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 4.0.10

* Update iOS CocoaPod dependency to 5.0 to fix deprecated API usage issue.

## 4.0.9

* Update and migrate iOS example project.
Expand Down
16 changes: 7 additions & 9 deletions packages/google_sign_in/ios/Classes/GoogleSignInPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
details:error.localizedDescription];
}

@interface FLTGoogleSignInPlugin () <GIDSignInDelegate, GIDSignInUIDelegate>
@interface FLTGoogleSignInPlugin () <GIDSignInDelegate>
@end

@implementation FLTGoogleSignInPlugin {
Expand All @@ -40,16 +40,16 @@ + (void)registerWithRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {
FlutterMethodChannel *channel =
[FlutterMethodChannel methodChannelWithName:@"plugins.flutter.io/google_sign_in"
binaryMessenger:[registrar messenger]];
FLTGoogleSignInPlugin *instance = [[FLTGoogleSignInPlugin alloc] init];
FLTGoogleSignInPlugin *instance = [[FLTGoogleSignInPlugin alloc] initWithController:[registrar messenger]];
Copy link
Member

Choose a reason for hiding this comment

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

initWithController:[registrar messenger]]

This is going to be a problem for you. You are ignoring types and forcing a binary messenger to be a view controller? Once you fix that, you'll find that the binary messenger isn't really a view controller anymore.

Copy link
Contributor Author
@collinjackson collinjackson Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should I do something like this?

    UIViewController *controller = [UIApplication sharedApplication].keyWindow.rootViewController;	
    [GIDSignIn sharedInstance].presentingViewController = controller;

Or should the app developer manually pass their FlutterViewController to the plugin? I'm not sure what the best approach is here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if grabbing the rootViewController here is going to work for you since the rootViewController might not still be the same rootViewController when GIDSignIn actually wants to present something. Also, I'm not sure what it would do if the rootViewController was a UINavigationController and its topmost view is a FlutterViewController, would it display correctly? You might have to try that out.

You might want to move the setter to the handle method call, is it possible GIDSignIn will want to display something without a method having triggered it?

Copy link
Member

Choose a reason for hiding this comment

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

Also, i don't know if sending the FlutterViewController will work since I don't think there is a mechanism to do that?

Copy link
Contributor Author
@collinjackson collinjackson Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that there probably isn't a mechanism to that.

I'm fine with grabbing the rootViewController at the time that the sign-in happens if you think that's a reasonable approach. It seems like the sign-in method is the only time that this gets used.

Copy link
Contributor Author
@collinjackson collinjackson Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated the approach as described, PTAL

[registrar addApplicationDelegate:instance];
[registrar addMethodCallDelegate:instance channel:channel];
}

- (instancetype)init {
- (instancetype)initWithController:(id)controller {
Copy link
Member
@jmagman jmagman Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This changes the designated initializer from -init to -initWithController:

GoogleSignInPlugin.h can indicate this:

#import <Flutter/Flutter.h>

NS_ASSUME_NONNULL_BEGIN

@interface FLTGoogleSignInPlugin : NSObject <FlutterPlugin>

- (instancetype)initWithController:(id)controller NS_DESIGNATED_INITIALIZER;
- (instancetype)init NS_UNAVAILABLE;

@end

NS_ASSUME_NONNULL_END

Also, it looks like this should be a UIViewController instead of an id (meaning a type check on [registar.messenger]).

self = [super init];
if (self) {
[GIDSignIn sharedInstance].delegate = self;
[GIDSignIn sharedInstance].uiDelegate = self;
[GIDSignIn sharedInstance].presentingViewController = controller;
Copy link
Member

Choose a reason for hiding this comment

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

That header says:

// The view controller used to present |SFSafariViewContoller| on iOS 9 and 10.
@property(nonatomic, weak) UIViewController *presentingViewController;

Does that mean this is only applicable for iOS 9 and 10?


// On the iOS simulator, we get "Broken pipe" errors after sign-in for some
// unknown reason. We can avoid crashing the app by ignoring them.
Expand Down Expand Up @@ -84,10 +84,10 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result
}
} else if ([call.method isEqualToString:@"signInSilently"]) {
if ([self setAccountRequest:result]) {
[[GIDSignIn sharedInstance] signInSilently];
[[GIDSignIn sharedInstance] restorePreviousSignIn];
}
} else if ([call.method isEqualToString:@"isSignedIn"]) {
result(@([[GIDSignIn sharedInstance] hasAuthInKeychain]));
result(@([[GIDSignIn sharedInstance] hasPreviousSignIn]));
} else if ([call.method isEqualToString:@"signIn"]) {
if ([self setAccountRequest:result]) {
@try {
Expand Down Expand Up @@ -136,9 +136,7 @@ - (BOOL)setAccountRequest:(FlutterResult)request {
- (BOOL)application:(UIApplication *)app openURL:(NSURL *)url options:(NSDictionary *)options {
NSString *sourceApplication = options[UIApplicationOpenURLOptionsSourceApplicationKey];
id annotation = options[UIApplicationOpenURLOptionsAnnotationKey];
collinjackson marked this conversation as resolved.
Show resolved Hide resolved
return [[GIDSignIn sharedInstance] handleURL:url
sourceApplication:sourceApplication
annotation:annotation];
return [[GIDSignIn sharedInstance] handleURL:url];
}

#pragma mark - <GIDSignInUIDelegate> protocol
Expand Down
2 changes: 1 addition & 1 deletion packages/google_sign_in/ios/google_sign_in.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ Enables Google Sign-In in Flutter apps.
s.source_files = 'Classes/**/*'
s.public_header_files = 'Classes/**/*.h'
s.dependency 'Flutter'
s.dependency 'GoogleSignIn', '~> 4.0'
s.dependency 'GoogleSignIn', '~> 5.0'
s.static_framework = true
end
2 changes: 1 addition & 1 deletion packages/google_sign_in/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: Flutter plugin for Google Sign-In, a secure authentication system
for signing in with a Google account on Android and iOS.
author: Flutter Team <flutter-dev@googlegroups.com>
homepage: https://github.com/flutter/plugins/tree/master/packages/google_sign_in
version: 4.0.9
version: 4.0.10
Copy link
@Knupper Knupper Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

should we not update the version to 5.x.x as well to indicate wich GoogleSignIn Version is used inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally we don't update the Dart and wrapped SDK versions in lockstep. In this case, we're not making a breaking change to the semantics of the Dart plugin, so I don't think a major version increment is needed.


flutter:
plugin:
Expand Down