[google_sign_in] PR 2/4 Migrate the plugin class from Objective-C to Swift. - #12655
[google_sign_in] PR 2/4 Migrate the plugin class from Objective-C to Swift.#12655victogomez-cs wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the google_sign_in_ios plugin class from Objective-C to Swift, replacing FLTGoogleSignInPlugin with GoogleSignInPlugin and adding an Objective-C exception catcher helper. Feedback on the changes suggests simplifying the sanitizedUserInfo function by using a non-optional parameter, updating flutterError to handle the optional error mapping, and correcting a signature mismatch in the scene(_:openURLContexts:) delegate method to return Void instead of Bool while reusing the handleURLs helper.
| private func sanitizedUserInfo(_ value: Any?) -> Any { | ||
| switch value { | ||
| case let error as NSError: | ||
| return [ | ||
| "domain": error.domain, | ||
| "code": "\(error.code)", | ||
| "localizedDescription": error.localizedDescription, | ||
| "userInfo": sanitizedUserInfo(error.userInfo), | ||
| ] | ||
| case let string as String: | ||
| return string | ||
| case let url as URL: | ||
| return url.absoluteString | ||
| case let number as NSNumber: | ||
| return number | ||
| case let array as [Any]: | ||
| return array.map { sanitizedUserInfo($0) } | ||
| case let dict as [AnyHashable: Any]: | ||
| var safeValues: [AnyHashable: Any] = [:] | ||
| safeValues.reserveCapacity(dict.count) | ||
| for (key, nestedValue) in dict { | ||
| safeValues[key] = sanitizedUserInfo(nestedValue) | ||
| } | ||
| return safeValues | ||
| default: | ||
| if let value { | ||
| return "[Unsupported type: \(String(describing: type(of: value)))]" | ||
| } | ||
| return "[Unsupported type: nil]" | ||
| } | ||
| } |
There was a problem hiding this comment.
sanitizedUserInfo can be simplified by making its parameter non-optional Any. Since userInfo dictionaries and nested values are typically non-optional when traversed, we can avoid the awkward default case check for nil and make the code more robust.
private func sanitizedUserInfo(_ value: Any) -> Any {
switch value {
case let error as NSError:
return [
"domain": error.domain,
"code": "\(error.code)",
"localizedDescription": error.localizedDescription,
"userInfo": sanitizedUserInfo(error.userInfo),
]
case let string as String:
return string
case let url as URL:
return url.absoluteString
case let number as NSNumber:
return number
case let array as [Any]:
return array.map { sanitizedUserInfo($0) }
case let dict as [AnyHashable: Any]:
var safeValues: [AnyHashable: Any] = [:]
safeValues.reserveCapacity(dict.count)
for (key, nestedValue) in dict {
safeValues[key] = sanitizedUserInfo(nestedValue)
}
return safeValues
default:
return "[Unsupported type: \(String(describing: type(of: value)))]"
}
}There was a problem hiding this comment.
Thanks, going to keep Any? here. The Obj-C helper (FSISanitizedUserInfo) takes a nullable id and is called with a nil NSError (and thus nil userInfo) from FSIFlutterErrorForNSError. The default nil branch is that path, not leftover optionality. Tightening this to Any would require changing how nil errors are mapped, which we don't want in this port
| private func flutterError(from error: NSError?) -> FlutterError { | ||
| return FlutterError( | ||
| code: String(format: "%@: %ld", error?.domain ?? "(null)", error?.code ?? 0), | ||
| message: error?.localizedDescription, | ||
| details: sanitizedUserInfo(error?.userInfo)) | ||
| } |
There was a problem hiding this comment.
If sanitizedUserInfo is updated to accept a non-optional Any, we can safely map the optional error here to pass its userInfo only when it is non-nil. This also correctly preserves nil details when error is nil instead of returning "[Unsupported type: nil]".
| private func flutterError(from error: NSError?) -> FlutterError { | |
| return FlutterError( | |
| code: String(format: "%@: %ld", error?.domain ?? "(null)", error?.code ?? 0), | |
| message: error?.localizedDescription, | |
| details: sanitizedUserInfo(error?.userInfo)) | |
| } | |
| private func flutterError(from error: NSError?) -> FlutterError { | |
| return FlutterError( | |
| code: String(format: "%@: %ld", error?.domain ?? "(null)", error?.code ?? 0), | |
| message: error?.localizedDescription, | |
| details: error.map { sanitizedUserInfo($0.userInfo) }) | |
| } |
There was a problem hiding this comment.
This would change the nil-NSError mapping. Obj-C does details:FSISanitizedUserInfo(error.userInfo) even when error is nil, so the Dart side gets a sanitized placeholder string, not nil details. error.map { ... } would send details: nil instead. Leaving the current call as-is to stay aligned with FSIFlutterErrorForNSError
| public func scene(_ scene: UIScene, openURLContexts urlContexts: Set<UIOpenURLContext>) -> Bool | ||
| { | ||
| var handled = false | ||
| for context in urlContexts { | ||
| handled = signIn.handle(context.url) || handled | ||
| } | ||
| return handled | ||
| } |
There was a problem hiding this comment.
The delegate method scene(_:openURLContexts:) in UISceneDelegate (and FlutterSceneLifeCycleDelegate) does not return a value (it returns Void). Implementing it with -> Bool is a signature mismatch. Additionally, the implementation duplicates the URL handling logic. We can simplify this by calling the existing handleURLs helper method and removing the return type.
public func scene(_ scene: UIScene, openURLContexts urlContexts: Set<UIOpenURLContext>) {
handleURLs(urlContexts.map { $0.url })
}There was a problem hiding this comment.
UISceneDelegate.scene(_:openURLContexts:) is Void, but this method is FlutterSceneLifeCycleDelegate, which is - (BOOL)scene:openURLContexts:, Flutter uses the return value to decide whether another plugin should see the URL. The old Obj-C implementation was void and didn't actually match that protocol. Returning Bool here is intentional.
handleURLs is a test helper that deliberately ignores the handle result so we can cover scene URLs without constructing UIOpenURLContext. Routing scene through it would drop the Bool Flutter needs. Leaving this as-is
f4ccb35 to
34f5240
Compare
34f5240 to
9dce861
Compare
1 participant
Migrates
FLTGoogleSignInPluginfrom Objective-C to Swift (GoogleSignInPlugin.swift). GID SDK wrappers andViewProviderstay Obj-C for this PR.Intended as a 1:1 move of configure, restorePreviousSignIn, signIn, refreshedAuthorizationTokens, addScopes, signOut, disconnect, error-code mapping, URL handling, and the iOS view-controller / macOS window presentation paths.
presentingViewController/presentingWindowon the Obj-C wrapper protocol become nullable. That matches existing runtime behavior:FSIViewProvider.viewControllerwas already nullable.The
-6EMM mapping inpigeonErrorCode(for:)is kept as a numeric code becauseGIDSignInError.emmis not imported into Swift.Bumps
google_sign_in_iosto 6.3.3.PR 2/4 of the Obj-C → Swift migration. Depends on PR 1/4 (SPM packaging). Continues flutter/flutter#119103
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2