-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support SSO error handling when ignore authorization step. #294
base: master
Are you sure you want to change the base?
Support SSO error handling when ignore authorization step. #294
Conversation
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.
Sorry about the late review, requested some changes.
private func setupAppSwitchingObserver() { | ||
let observer = AppSwitchingObserver() | ||
observer.onTrigger = { | ||
DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { |
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.
Why is there a DispatchQueue.asyncAfter
here?
token = NotificationCenter.default.addObserver(forName: UIApplication.didBecomeActiveNotification, object: nil, queue: nil) { [weak self] (_) in | ||
guard let `self` = self else { return } | ||
guard self.isEnabled else { return } | ||
self.onTrigger?() |
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.
Would prefer if this was written:
guard let _self = self, _self.isEnabled else { return }
_self.onTrigger?()
Alternatively,
guard self?.isEnabled ?? false else { return }
self?.onTrigger?()
@@ -95,6 +95,7 @@ public class Swifter { | |||
|
|||
internal struct CallbackNotification { | |||
static let optionsURLKey = "SwifterCallbackNotificationOptionsURLKey" | |||
static let optionsUserCancelKey = "SwifterCallbackNotificationOptionsUserCancelKey" |
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: optionsUserCancelledKey
When user using sso, user can skip authorization step and switch master app.
This case is should deal as cancel.
For example, LINE-SDK(LINE is famous messaging app for jpn) implements this case using timeout logic.
https://github.com/line/line-sdk-ios-swift/blob/master/LineSDK/LineSDK/Login/LoginProcess.swift#L173
This PR is implemented same logic as LINE-SDK.
Please review this. 👍