[local_auth] Convert iOS/macOS to Swift #9459
Conversation
There was a problem hiding this comment.
I did this conversion manually (as a way to practice Swift); no auto-conversion or AI tools involved. I deliberately did it in-place, pretty much line by line, to simplify side-by-side review. I've left notes about changes that aren't just direct language conversion below.
There was a problem hiding this comment.
This can mostly be compared side-by-side to FLALocalAuthPlugin_Test.h
There was a problem hiding this comment.
This should be renamed in a follow-up; I didn't rename it here because there are enough changes that git would probably not show it as a diff, making review harder.
| @@ -2,6 +2,7 @@ | |||
| // Use of this source code is governed by a BSD-style license that can be | |||
| // found in the LICENSE file. | |||
|
|
|||
| import LocalAuthentication | |||
There was a problem hiding this comment.
This should always have been here, but transitive includes in Obj-C masked the missing include.
| @@ -16,24 +17,24 @@ import XCTest | |||
| private let timeout: TimeInterval = 30.0 | |||
|
|
|||
| /// A context factory that returns preset contexts. | |||
| final class StubAuthContextFactory: NSObject, FLADAuthContextFactory { | |||
| final class StubAuthContextFactory: AuthContextFactory { | |||
There was a problem hiding this comment.
I removed NSObject from all the test implementations since the protocols aren't Obj-C now, so it was just cruft.
| var contexts: [FLADAuthContext] | ||
| init(contexts: [FLADAuthContext]) { | ||
| var contexts: [AuthContext] | ||
| init(contexts: [AuthContext]) { |
There was a problem hiding this comment.
All the Pigeon-generated type names changed because there's no more prefixing.
|
|
||
| /// A flutter plugin for local authentication. | ||
| // TODO(stuartmorgan): Remove the @unchecked Sendable, and convert to strict thread enforcement. | ||
| // This was added to minimize the changes while converting from Swift to Objective-C. |
There was a problem hiding this comment.
This was unfortunate, but it was the best I could come up with to fix all the warnings without significant restructuring. Without this, the places where we dispatch from a potentially-background-thread-callback back to a main thread call to a method on self triggers warnings about self not being Sendable.
There are ways to fix that with more static methods, passing more state around instead of using self, etc., but they would require refactoring that I didn't want to combine with the language conversion as it would have made side-by-side review nearly impossible.
| self.handleError(authError, options: options, strings: strings, completion: completion) | ||
| } else { | ||
| // This should not happen according to docs, but if it ever does the plugin should still | ||
| // fire the completion. |
There was a problem hiding this comment.
This branch is new. In Obj-C, we just assumed the docs weren't lying about the error being set if it returned false, but that's not type safe in Swift, and since docs have lied about nullability before I wanted a fallback instead of a crash from force-unwrapping.
| animated: true, | ||
| completion: nil) | ||
| } else { | ||
| // TODO(stuartmorgan): Create a new error code for failure to show UI, and return it here. |
There was a problem hiding this comment.
This branch is new. We were failing to handle the case where there is no view before, so I added minimal handling for now.
| self.handleResult(succeeded: false, completion: completion) | ||
| } | ||
| } else { | ||
| alert.runModal() |
There was a problem hiding this comment.
This branch is new. We were failing to handle the case where there is no view before.
We could fail like on iOS (see below), but since macOS has a simple way to handle showing an alert without a view I went with that option.
| } else { | ||
| // The Obj-C declaration of evaluatePolicy defines the callback type as NSError*, but the | ||
| // Swift version is (any Error)?, so provide a fallback in case somehow the type is not | ||
| // NSError. |
There was a problem hiding this comment.
This branch is new, for the reason described here.
This comment was marked as resolved.
This comment was marked as resolved.
Rewrites the iOS/macOS implementation in Swift. This is an in-place rewrite with minimal changes, to minimize the chances of breakage, and to simplify review. Future refactorings can improve the Swift structure (e.g., fully adopting thread enforcement).
flutter/flutter#119104
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).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 ↩3