-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/mac support #46
base: master
Are you sure you want to change the base?
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.
This is such a great work 👍 I'm glad that I can finally run Sentinel on macOS.
Based on your changes, I've added some comments so please check them out.
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.
I would recommend to use buildable folders and re-arrange file structure a bit. I'm okay with you to continue using e.g. Example for iOS
and Example for macOS
folder naming like it was already used (but they were groups).
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.
Additionally, try to use the same naming definition for targets; e.g. use Example-iOS
and Example-macOS
.
func applicationWillTerminate(_ aNotification: Notification) { | ||
// Insert code here to tear down your application | ||
} |
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.
Remove methods that are not used.
func applicationSupportsSecureRestorableState(_ app: NSApplication) -> Bool { | ||
return true | ||
} | ||
|
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.
Remove redundant new line.
override func viewDidLoad() { | ||
super.viewDidLoad() | ||
|
||
// Do any additional setup after loading the view. | ||
} | ||
|
||
override var representedObject: Any? { | ||
didSet { | ||
// Update the view, if already loaded. | ||
} | ||
} |
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.
Remove methods and properties which overrides are empty.
@@ -10,3 +11,9 @@ target 'Sentinel_Example' do | |||
inherit! :search_paths | |||
end | |||
end | |||
|
|||
target 'MacOSExample' do |
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.
Update target names for both iOS and macOS - as mentioned in the previous comment.
.init(title: "Model", value: getModelIdentifier() ?? ""), | ||
.init(title: "Name", value: ProcessInfo.processInfo.hostName), | ||
.init(title: "System version", value: systemVersion), | ||
.init(title: "UUID", value: hardwareDeviceUUID ?? "???") | ||
]) | ||
#else | ||
CustomInfoTool.Section(title: "Device", items: [ | ||
.init(title: "Model", value: UIDevice.current.model), | ||
.init(title: "Name", value: UIDevice.current.name), | ||
.init(title: "System version", value: systemVersion), | ||
.init(title: "UUID", value: UIDevice.current.identifierForVendor?.uuidString ?? "???"), | ||
.init(title: "Battery state", value: batteryState), | ||
.init(title: "Proximity state", value: UIDevice.current.proximityState ? "Close" : "Far") |
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.
Replace .init
methods with real class names.
Also, additionally, if the objects of the array are the same, you could instead return the array from a method instead creating the whole object inside the if-else
branches. Even better approach here would be a factory method or abstract factory (due to other methods), but I believe this can be added and improved in other PRs.
|
||
/// The trigger type which is triggered on the specified notification name. | ||
public static func notification(forName name: Notification.Name) -> Trigger { | ||
NotificationTrigger(notificationName: name) | ||
} | ||
#if !os(macOS) |
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 it be better to use #if os(iOS)
? Because with this, e.g. tvOS
or watchOS
in the future would support this. I know we will not add the possibility for that, but it is easier to work with constrained things than the opposite.
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.
Also, add a note to the comment (or discussion), on which platforms are those methods available.
@@ -19,7 +19,9 @@ struct TextEditingToolView: View { | |||
|
|||
Button(action: { | |||
viewModel.didPressSave(viewModel.value) | |||
presentationMode.wrappedValue.dismiss() | |||
#if !os(macOS) |
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.
Can we use os(iOS)
here as well?
}) | ||
})) | ||
.frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading) | ||
|
||
Button(action: { | ||
viewModel.didPressDelete() | ||
presentationMode.wrappedValue.dismiss() | ||
#if !os(macOS) |
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.
Same here as well.
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.
Regarding the SwiftUI example, I think this is a bit redundant in the current form. We have a new project that we need to maintain, while the implementation is different only for view part (app delegate counterpart is same).
I would rather propose to add the SwiftUI code in the existing example in one of the provided forms:
- Create two screens (for iOS and macOS targets) - one with UIKit and another with SwiftUI examples (with summoning Sentinel)
- Create a new target that will work on both iOS and macOS
Also, one additional note; I've seen you've done a lot of work, but you've mixed that into bigger PRs, e.g. podspec update, macOS support, preferences update. On the other PR there are multiple topics as well. Try to separate them in future PR for easier review process and separation of concerns. The work you did is great, but this can make things easier to understand and will produce quicker review times. |
Summary
Added macOS support with a new SwiftUI example
Changes
Type
Additional information
Description
I've mentioned everything in the changes section
Checklist
Additional notes
After this gets reviewed, I would like to try to push team members to try out the new version where possible so we can test it out. Also, I would like to improve the current state of the navigation on macOS