[local_auth_darwin] - Converts implementation to Swift #7586

Mairramer · 2024-09-05T16:22:20Z

Part of flutter/flutter#119015
Closes flutter/flutter#119104

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

# Conflicts: # packages/local_auth/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/include/local_auth_darwin/messages.g.h # packages/local_auth/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/messages.g.m

stuartmorgan · 2024-12-03T16:25:22Z

As a high-level comment on my specific comments above: as discussed earlier, the goal of a Swift conversion PR should be to convert the existing logic to Swift as directly as possible, modulo any language-specific idiomatic differences. A conversion is already a relatively high-risk change due to the magnitude of code being changed; intentionally mixing in refactors, subjective changes to implementation approach, etc. is highly undesirable in such a PR because it increases the risk even futher.

Any delta between the Obj-C logic and the Swift logic in this PR should be clearly and directly attributable to a language-specific reason.

If you have subjective disagreements about how the code is currently structured, please feel free to either make prequel PRs or follow-up PRs that make those changes, so they can be discussed, reviewed, and tested in isolation, rather than as part of a complete rewrite of the code.

Mairramer · 2024-12-05T12:57:10Z

As a high-level comment on my specific comments above: as discussed earlier, the goal of a Swift conversion PR should be to convert the existing logic to Swift as directly as possible, modulo any language-specific idiomatic differences. A conversion is already a relatively high-risk change due to the magnitude of code being changed; intentionally mixing in refactors, subjective changes to implementation approach, etc. is highly undesirable in such a PR because it increases the risk even futher.

Any delta between the Obj-C logic and the Swift logic in this PR should be clearly and directly attributable to a language-specific reason.

If you have subjective disagreements about how the code is currently structured, please feel free to either make prequel PRs or follow-up PRs that make those changes, so they can be discussed, reviewed, and tested in isolation, rather than as part of a complete rewrite of the code.

I am reworking some parts you mentioned again and trying to keep the test mocks as close as possible to how they were before. There are many things I would like to remove, like the factory. While I understand the reasoning behind its use, it's not something I personally prefer. I'm working on this more slowly because I have limited time to dedicate to it.

stuartmorgan · 2024-12-05T13:23:43Z

There are many things I would like to remove, like the factory. While I understand the reasoning behind its use, it's not something I personally prefer.

As I said in my previous comment:

If you have subjective disagreements about how the code is currently structured, please feel free to either make prequel PRs or follow-up PRs that make those changes, so they can be discussed, reviewed, and tested in isolation, rather than as part of a complete rewrite of the code.

This PR is not the place to make structural changes that are based on your personal preferences.

Mairramer · 2024-12-06T17:43:57Z

@stuartmorgan I made some adjustments and I'm closer to the Objective-C implementation. There are still a few things left to be done, and if you think it's appropriate, I can mark this PR as a draft and continue working on the remaining details

stuartmorgan · 2025-01-10T21:23:47Z

packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift

-      // TODO(stuartmorgan): Fix this; this was the pre-Pigeon-migration
-      // behavior, so is preserved as part of the migration, but a failed
-      // authentication should return failure, not an error that results in a
-      // PlatformException.


I'm still seeing it as removed.

stuartmorgan · 2025-01-13T15:58:47Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/AlertController.swift

+  extension UIAlertController: AlertController {
+    // TODO(Mairramer): Investigate if this can be removed once the issue is fixed.
+    // Workaround for an issue with window hierarchy during the migration from Objective-C to Swift.
+    // The window hierarchy is not always properly maintained after the migration, which can lead to


What does this mean exactly? The plugin should not be mutating the view hierarchy.

For some reason, this is happening. Before I went on vacation, I was looking into this.

stuartmorgan · 2025-01-13T16:00:46Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/AlertController.swift

+    open override func present(
+      _ presentingViewController: UIViewController, animated: Bool, completion: (() -> Void)?
+    ) {
+      DispatchQueue.main.async {


If there is code running on an non-main thread that wants to show an alert, that should be handled at the call site, not abstracted away in an extension.

stuartmorgan · 2025-01-13T16:02:04Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/AlertController.swift

+    // TODO(Mairramer): Check if is possible to remove the dispatch queue, and use the @MainActor annotation instead.
+    func createAlert() -> Alert {
+      var alert: Alert!
+      DispatchQueue.main.sync {


Same comment here. We certainly shouldn't be changing threading behavior in code that's not unit-testable. The Obj-C versions of the default implementations were, by explicit design, direct passthroughs to the underlying class, and that should be true for Swift as well.

stuartmorgan · 2025-01-13T16:06:17Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/LocalAuthPlugin.swift

+
+  #if os(macOS)
+    var view: NSView {
+      // TODO(Mairramer): Migrate check if is possible to remove the dispatch queue, and use the @MainActor annotation instead.


As in the other comments, the default implementation needs to be a direct passthrough, not containing complexities like threading logic that can't be unit tested.

What is the problem this is here to solve?

stuartmorgan · 2025-01-13T16:23:22Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/LocalAuthPlugin.swift

+        LAError.passcodeNotSet.rawValue,
+        LAError.authenticationFailed.rawValue:
+        handleError(authError: error, options: options, strings: strings, completion: completion)
+        return


Since Swift doesn't support implicit fallthrough in switch statments, aren't these returns redundant?

Yes, you're correct!

stuartmorgan · 2025-01-13T16:25:20Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/LocalAuthPlugin.swift

+      )
+      return
+    default:
+      result = .failure


Why have you changed the logic from returning .errorNotAvailable as the default to returning .failure as the default?

If I'm not mistaken, it was to establish a success/failure pattern and also, if I recall correctly, to avoid an exception(PlataformException) on the Dart side. (I really need to remember).

stuartmorgan · 2025-01-13T16:27:14Z

...h/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/LocalAuthPlugin.swift

+    completion: @escaping (Result<AuthResultDetails, Error>) -> Void
+  ) {
+    #if os(macOS)
+      DispatchQueue.main.async {


Are there cases where we are calling showAlert from non-main threads? If so, this should be outside the OS-level switch. If not, we shouldn't need this.

stuartmorgan · 2025-01-13T16:28:38Z

packages/local_auth/local_auth_darwin/CHANGELOG.md

@@ -1,5 +1,6 @@
-## NEXT
+## 1.4.3


Let's make this a minor version bump since it's a full rewrite, even though we don't intend any behavioral changes.

stuartmorgan · 2025-01-13T16:31:41Z

packages/local_auth/local_auth_darwin/darwin/Tests/FLALocalAuthPluginTests.swift

+      error = thrownError
+    } catch {
+      XCTFail("Unexpected error thrown: \(error)")
+    }
    XCTAssertNil(error)


I believe XCTAssertThrowsError is the standard pattern for testing throwing in Swift (applies throughout the file).

XCTAssertThrowsError is indeed the standard when you want to test if a function throws an error, but in this case, since we have a Result, it’s also common. It depends more on whether we want to capture that error.

Mairramer · 2025-01-22T13:37:04Z

Sorry for the delay (1 month away from code).

feat:: migrate objC to Swift

b49f5c5

github-actions bot added p: local_auth platform-ios platform-macos labels Sep 5, 2024

Mairramer and others added 6 commits September 5, 2024 13:23

fix

e4f1902

new changes

b86edf2

more work

c7b79c0

small fix

5b8bcf1

another fix

a77bd18

Merge branch 'main' into migrate-local_auth-to-swift

2a34903

Mairramer closed this Oct 19, 2024

refactor

fa42c8a

Mairramer reopened this Nov 6, 2024

Mairramer added 17 commits November 6, 2024 09:21

some improves

bea5a37

fix

37cf5e1

fix

db60196

fix

272451f

more code

f3b140b

more fixes

d471e2d

fix test

fe12dda

some clean ups

f1ba9a2

fix

b0420e6

add author

4f7d1fb

fix

1410b39

fix

6aaff99

fix

3fce86c

fix

942f173

fix

f9b51e7

fix

2e23838

fix

d6c736c

Mairramer and others added 4 commits December 6, 2024 10:16

some reworks

8996e4c

Merge branch 'main' into migrate-local_auth-to-swift

fbf4b38

some fixes

4639b88

new changes

5258731

Mairramer and others added 10 commits December 7, 2024 11:33

solve problems

9ebba78

some fixes

663ff05

some fixes

92f3d3f

fixes

350c6ab

fixes

c664e92

fix

dbea0df

fix

ce53e27

fix

b495f10

Update CHANGELOG.md

71e0d04

fix

5081932

Mairramer requested a review from stuartmorgan December 9, 2024 17:34

Mairramer and others added 6 commits December 9, 2024 14:48

fix

6cdf882

Merge branch 'main' into migrate-local_auth-to-swift

d0d3c98

fix

e1f94fe

fix

f088874

small changes

90e5a08

Merge branch 'main' into migrate-local_auth-to-swift

78939f5

stuartmorgan requested changes Jan 13, 2025

View reviewed changes

Merge branch 'main' into migrate-local_auth-to-swift

4358062

Mairramer marked this pull request as draft January 29, 2025 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[local_auth_darwin] - Converts implementation to Swift #7586

[local_auth_darwin] - Converts implementation to Swift #7586

Mairramer commented Sep 5, 2024 •

edited

Loading

stuartmorgan commented Dec 3, 2024

Mairramer commented Dec 5, 2024

stuartmorgan commented Dec 5, 2024

Mairramer commented Dec 6, 2024

stuartmorgan Jan 10, 2025

stuartmorgan Jan 13, 2025

Mairramer Jan 22, 2025

stuartmorgan Jan 13, 2025

stuartmorgan Jan 13, 2025

stuartmorgan Jan 13, 2025

stuartmorgan Jan 13, 2025

Mairramer Jan 22, 2025

stuartmorgan Jan 13, 2025

Mairramer Jan 22, 2025

stuartmorgan Jan 13, 2025

stuartmorgan Jan 13, 2025

stuartmorgan Jan 13, 2025

Mairramer Jan 22, 2025

Mairramer commented Jan 22, 2025

[local_auth_darwin] - Converts implementation to Swift #7586

Are you sure you want to change the base?

[local_auth_darwin] - Converts implementation to Swift #7586

Conversation

Mairramer commented Sep 5, 2024 • edited Loading

Pre-launch Checklist

stuartmorgan commented Dec 3, 2024

Mairramer commented Dec 5, 2024

stuartmorgan commented Dec 5, 2024

Mairramer commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mairramer commented Jan 22, 2025

Mairramer commented Sep 5, 2024 •

edited

Loading