[local_auth_darwin] MacOS Support #6267
Conversation
…m/packages into macos-support
|
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. |
|
Thanks for the contribution! This appears to be very similar to #5766; does this need to be a new PR rather than leaving comments on that one about any suggested changes? |
|
@stuartmorgan That pr doesn't seem to have any momentum and is outdated so I figured I would take this on. |
packages/local_auth/local_auth_darwin/example/ios/Runner.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
...l_auth/local_auth_darwin/example/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme
Outdated
Show resolved
Hide resolved
packages/local_auth/local_auth_darwin/lib/types/auth_messages_darwin.dart
Outdated
Show resolved
Hide resolved
packages/local_auth/local_auth_darwin/example/macos/RunnerTests/RunnerTests.swift
Outdated
Show resolved
Hide resolved
…uthMessages This reverts commit 676b0eb.
|
Is this ready for re-review? There are still unaddressed comments and failing tests, so I assumed it was a work-in-progress. |
|
I could use some assistance on the comments @stuartmorgan left me. I am not seeing a clean way to use the Platform.isMacos and still have the dart test cases pass. I am also not well versed in testing the alert construction as you requested. |
Per my earlier comment, you can wrap those calls in something that can have an alternate implementation injected for unit tests, using standard dependency injection patterns. |
All fixed. I addressed all your comments @stuartmorgan . This pr is ready for re-review. |
|
@alexrabin-sentracam I'll re-review when I have time (I currently have some very high priority work that will delay it); it's not necessary to constantly merge the branch in the meantime. |
|
@stuartmorgan Okay sounds good. Thank you. |
| @implementation FLADefaultAlertFactory | ||
|
|
||
| #if TARGET_OS_OSX | ||
| - (NSAlert *)createNSAlert { |
There was a problem hiding this comment.
Nit: I would just call this alert, for consistency with the iOS version (see next comment).
| return [[NSAlert alloc] init]; | ||
| } | ||
| #elif TARGET_OS_IOS | ||
| - (UIAlertController *)createUIAlert:(NSString *)message { |
There was a problem hiding this comment.
Injections wrappers should be as thin as possible, since by definition anything in this method won't be tested. So this should be alertControllerWithTitle:message:preferredStyle:, and be a direct passthrough.
| @@ -18,6 +18,24 @@ - (LAContext *)createAuthContext { | |||
| } | |||
| @end | |||
|
|
|||
| @interface FLADefaultAlertFactory : NSObject <FLADAlertFactory> | |||
There was a problem hiding this comment.
This needs a declaration comment, per the style guide.
| @@ -49,24 +67,37 @@ - (instancetype)initWithOptions:(nonnull FLADAuthOptions *)options | |||
| @interface FLALocalAuthPlugin () | |||
| @property(nonatomic, strong, nullable) FLAStickyAuthState *lastCallState; | |||
| @property(nonatomic, strong) NSObject<FLADAuthContextFactory> *authContextFactory; | |||
| @property(nonatomic, strong) NSObject<FLADAlertFactory> *alertFactory; | |||
There was a problem hiding this comment.
These need declaration comments, per the style guide. (It's an oversight that the others don't have them.)
| - (instancetype)init { | ||
| return [self initWithContextFactory:[[FLADefaultAuthContextFactory alloc] init]]; | ||
| - (instancetype)initWithRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar { | ||
| self = [super init]; |
There was a problem hiding this comment.
This should be calling the init with the most parameters, not re-implementing initialization.
| - (instancetype)initWithNSAlert:(NSAlert *)alert { | ||
| self = [super init]; | ||
| if (self) { | ||
| _alert = alert; |
There was a problem hiding this comment.
These aren't doing copies, which violates the current API contract. (This will be resolved if you don't declare them as copy though.)
| // Tests that the alert window is the same as the registrar window | ||
| XCTAssertEqual(alertFactory.alert.window, reg.view.window); | ||
| // TODO(stuartmorgan): Fix this; this was the pre-Pigeon-migration | ||
| // behavior, so is preserved as part of the migration, but a failed |
There was a problem hiding this comment.
How can an entirely new platform implementation have previous behavior that needs to be preserved?
| StubAlertFactory *alertFactory; | ||
| #if TARGET_OS_OSX | ||
|
|
||
| id mockNSAlert = OCMClassMock([NSAlert class]); |
There was a problem hiding this comment.
Why is this a mock? The only thing that this appears to be used for in the test is to read a public property back out of it, which can be done with a real object, avoiding the need for a mock.
| FlutterError *_Nullable error) { | ||
| XCTAssertTrue([NSThread isMainThread]); | ||
| // Tests that the alert window is the same as the registrar window | ||
| XCTAssertEqual(alertFactory.alert.window, reg.view.window); |
There was a problem hiding this comment.
I would expect there to also be a test that if the user provides a custom message, the alert has that message.
| [self waitForExpectationsWithTimeout:kTimeout handler:nil]; | ||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
You've changed the iOS codepaths to make the alert testable; there needs to be at least one new test covering those changes.
Adds macOS support for local_auth_darwin
Cancelled Example:
Success Example
Error Example
List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#140685
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.