Use ServiceManagement API to enable/disable login items (used to start an app after login) #45
Conversation
legacy API otherwise
WalkthroughThe recent changes integrate Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant JNI as JNI Layer
participant SKY as SKYLaunchService
participant macOS as macOS Login Items
JNI->>SKY: isLoginItemEnabled()
SKY->>macOS: Check Login Item Status
macOS-->>SKY: Status
SKY-->>JNI: Status
JNI->>SKY: enableLoginItem()
SKY->>macOS: Enable Login Item
macOS-->>SKY: Success/Failure
SKY-->>JNI: Success/Failure
JNI->>SKY: disableLoginItem()
SKY->>macOS: Disable Login Item
macOS-->>SKY: Success/Failure
SKY-->>JNI: Success/Failure
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m (1 hunks)
Additional comments not posted (5)
src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m (5)
11-11: Ensure the new importServiceManagement/ServiceManagement.his necessary and correctly placed.Verification successful
The import of
ServiceManagement/ServiceManagement.his necessary due to multiple usages ofSMAppServicein the file.
SMAppServiceis used in several lines withinsrc/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity of the new import. # Test: Search for usage of `SMAppService` in the file. Expect: At least one usage. rg --type objc 'SMAppService' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 536
75-98: Evaluate the error handling inenableLoginItem.Verification successful
The extracted context shows that the
NSErrorinstances are being properly checked and logged in theenableLoginItemfunction. Specifically, the error is logged withNSLogif theregisterAndReturnErrormethod fails, and the function returnsNOto indicate failure.
- The error handling in the
enableLoginItemfunction is appropriate as it logs the error description and returns a failure status.Locations in the codebase:
src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mlines 75-98Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling in enabling login items. # Test: Search for error handling patterns in the enable function. Expect: Proper error handling. rg --type objc 'NSError' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 195
Script:
#!/bin/bash # Extract the full context of NSError usage to verify error handling in enableLoginItem function. rg -A 5 -B 5 'NSError\* error = nil;' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 1691
13-38: Review the migration logic inmigrateSharedFileListLoginItem.Verification successful
To verify the migration logic and error handling in the
migrateSharedFileListLoginItemfunction, we need to ensure that theNSErrorinstances are properly used and that the function handles potential errors and edge cases correctly. The initial script output shows three instances ofNSError* error = nil;in the file, but we need to confirm their usage.Let's review the specific lines where
NSErroris used to ensure proper error handling.
The
migrateSharedFileListLoginItemfunction includes proper error handling by checking theNSErrorinstance after attempting to register the app service and logging an error message if the registration fails. This indicates that the migration logic handles potential errors correctly.
- The error handling in the
migrateSharedFileListLoginItemfunction is appropriate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the migration logic and error handling. # Test: Search for error handling patterns in the migration function. Expect: Proper error handling. rg --type objc 'NSError' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 195
Script:
#!/bin/bash # Description: Extract lines containing NSError to review their usage in the function. # Extract lines containing NSError and a few lines before and after for context. rg --type objc 'NSError' -A 3 -B 3 src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 1398
102-133: Assess the error handling indisableLoginItem.Verification successful
The
disableLoginItemfunction has proper error handling for both macOS 13 and above, as well as for earlier versions. For macOS 13 and above, it checks for errors usingNSErrorand logs an appropriate message if an error occurs. For earlier versions, it logs an error message if it fails to create the shared file list.
- The error handling for macOS 13 and above is done using
NSErrorand logs the error description.- For macOS versions below 13, it logs an error message if the shared file list cannot be created.
This confirms that the error handling in the
disableLoginItemfunction is appropriately implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling in disabling login items. # Test: Search for error handling patterns in the disable function. Expect: Proper error handling. rg --type objc 'NSError' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 195
Script:
#!/bin/bash # Description: Locate the definition of the disableLoginItem function and inspect the usage of NSError within it. # Search for the disableLoginItem function definition ast-grep --lang objc --pattern 'disableLoginItem($_) { $$$ }' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.mLength of output: 247
Script:
#!/bin/bash # Description: Locate the definition of the disableLoginItem function and inspect the usage of NSError within it. # Search for the disableLoginItem function definition rg 'disableLoginItem' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m -A 20 -B 5Length of output: 1454
Script:
#!/bin/bash # Description: Extract the complete disableLoginItem function for thorough inspection. # Extract the complete disableLoginItem function definition rg 'Java_org_cryptomator_macos_autostart_MacLaunchServices_00024Native_disableLoginItem' src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m -A 50 -B 5Length of output: 1846
41-70: Check the version-dependent logic inisLoginItemEnabled.
@tobihagemann Hi, I was surprised too. The documentation is quite poor in this part and mixed too, all in all quite unusable from my point of view.
You are very welcome! |
|
@purejava I did some refactorings and just pushed them without asking, because it would've taken longer to explain and request the changes. 😄 My goal was to deduplicate some code, make it more readable, and also use more ObjC patterns as I'm used to. (Oh yeah, and a small thing that we use tabs instead of spaces.) Can you please take a look and review the changes? To be honest, I didn't test them thoroughly. 😅 |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/main/native/Integrations.xcodeproj/project.pbxproj (5 hunks)
- src/main/native/SKYLaunchService.h (1 hunks)
- src/main/native/SKYLaunchService.m (1 hunks)
- src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/native/SKYLaunchService.h
Files skipped from review as they are similar to previous changes (1)
- src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m
Additional comments not posted (4)
src/main/native/SKYLaunchService.m (3)
30-42: Check error handling inenableLoginItemfor macOS 13 and later.Verification successful
Error handling in
enableLoginItemfor macOS 13 and later is properly logged and managed. The method logs the error usingNSLogand returnsNOif the registration fails. ThedisableLoginItemmethod follows a similar pattern for unregistration.
enableLoginItemmethod:
- Logs the error using
NSLog.- Returns
NOif the registration fails.
disableLoginItemmethod:
- Logs the error using
NSLog.- Returns
NOif the unregistration fails.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all error paths in `enableLoginItem` are properly logged and handled. # Test: Search for error handling patterns in the function. rg --type objc $'NSError \*error;'Length of output: 141
Script:
#!/bin/bash # Extract the full implementation of the `enableLoginItem` method and any other methods containing `NSError *error;` in `src/main/native/SKYLaunchService.m`. # Extract the `enableLoginItem` method implementation ast-grep --lang objc --pattern $'+ (BOOL)enableLoginItem { $$$ }' src/main/native/SKYLaunchService.m # Extract other methods containing `NSError *error;` rg --type objc --context 10 'NSError \*error;' src/main/native/SKYLaunchService.mLength of output: 1070
14-28: Ensure migration logic from legacy to new API is robust.Verification successful
The
disableLegacyLoginItemmethod is implemented insrc/main/native/SKYLaunchService.mand appears to correctly handle the disabling of legacy login items by interacting with theLSSharedFileList. The method iterates through the shared file list items and resolves their paths to match the application's path, ensuring proper removal.
- The method
disableLegacyLoginItemis correctly implemented to disable legacy login items.- The migration logic from legacy to new API in the
isLoginItemEnabledmethod is robust.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the migration logic from legacy to new API by checking if the legacy items are properly disabled after migration. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type objc $'disableLegacyLoginItem'Length of output: 251
Script:
#!/bin/bash # Description: Retrieve the implementation of the disableLegacyLoginItem method to verify its correctness. # Search for the implementation of disableLegacyLoginItem in the Objective-C files. ast-grep --lang objc --pattern $'- (BOOL)disableLegacyLoginItem { $$$ }'Length of output: 184
Script:
#!/bin/bash # Description: Retrieve the implementation of the disableLegacyLoginItem method to verify its correctness. # Search for the implementation of disableLegacyLoginItem in the Objective-C files. rg --type objc --pcre2 --multiline --multiline-dotall '^\s*-\s*\(BOOL\)\s*disableLegacyLoginItem\s*\{[^}]*\}'Length of output: 111
Script:
#!/bin/bash # Description: Locate the definition of the disableLegacyLoginItem method to verify its presence and correctness. # Search for the method signature disableLegacyLoginItem in the Objective-C files. rg --type objc 'disableLegacyLoginItem'Length of output: 251
Script:
#!/bin/bash # Description: Retrieve the full implementation of the disableLegacyLoginItem method to verify its correctness. # Extract the full implementation of the disableLegacyLoginItem method in the Objective-C files. rg --type objc --context 10 'disableLegacyLoginItem' src/main/native/SKYLaunchService.mLength of output: 2086
44-56: Review the consistency of error handling indisableLoginItem.Verification successful
The
disableLoginItemmethod handles errors differently based on the macOS version. For macOS 13 and above, it usesSMAppService.mainAppService unregisterAndReturnError:&errorand logs the error if it fails. For macOS versions below 13, it callsdisableLegacyLoginItem, but we need to verify howdisableLegacyLoginItemhandles errors to ensure consistency.To proceed, we need to inspect the
disableLegacyLoginItemmethod to understand its error handling mechanism.
To verify the consistency of error handling in the
disableLoginItemmethod, we need to locate and inspect thedisableLegacyLoginItemmethod. Sinceast-grepdoes not support Objective-C, we will usergto search for the method definition.
To verify the consistency of error handling, we need to inspect the implementation of the
disableLegacyLoginItemmethod found insrc/main/native/SKYLaunchService.m.
To fully verify the consistency of error handling, we need to extract the complete implementation of the
disableLegacyLoginItemmethod.
The
disableLegacyLoginItemmethod handles errors by logging a message when it fails to create the shared file list, similar to how thedisableLoginItemmethod logs errors when unregistering the login item fails. This indicates a consistent approach to error handling across different macOS versions.
disableLoginItemlogs an error message ifunregisterAndReturnErrorfails.disableLegacyLoginItemlogs an error message if it fails to create the shared file list.Both methods use
NSLogto report errors, ensuring consistency in error handling.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure consistent error handling across different macOS versions in `disableLoginItem`. # Test: Search for error handling patterns in the function. rg --type objc $'NSError \*error;'Length of output: 141
Script:
#!/bin/bash # Description: Inspect the `disableLegacyLoginItem` method to understand its error handling mechanism. # Search for the `disableLegacyLoginItem` method definition in the codebase. ast-grep --lang objc --pattern 'disableLegacyLoginItem'Length of output: 164
Script:
#!/bin/bash # Description: Search for the `disableLegacyLoginItem` method definition in the codebase. # Use `rg` to find the `disableLegacyLoginItem` method definition. rg 'disableLegacyLoginItem' --type objcLength of output: 251
Script:
#!/bin/bash # Description: Extract the `disableLegacyLoginItem` method implementation from `src/main/native/SKYLaunchService.m`. # Use `rg` to extract the `disableLegacyLoginItem` method implementation. rg --context 10 'disableLegacyLoginItem' src/main/native/SKYLaunchService.mLength of output: 2074
Script:
#!/bin/bash # Description: Extract the complete `disableLegacyLoginItem` method implementation from `src/main/native/SKYLaunchService.m`. # Use `rg` to extract the complete `disableLegacyLoginItem` method implementation. rg --context 20 'disableLegacyLoginItem' src/main/native/SKYLaunchService.mLength of output: 4033
src/main/native/Integrations.xcodeproj/project.pbxproj (1)
10-11: Confirm integration ofSKYLaunchServicefiles into the project.Ensure that the new files are correctly referenced and integrated into the project build phases and groups.
Also applies to: 24-25, 61-62, 83-83, 145-145
That's perfectly fine @tobihagemann! I haven't written an entire ObjC class before, so this was much more efficient and from reading your code, I've learned about it. Regarding the code formatting: both, the IntelliJ IDE and Sublime Text refused to help in formatting. It looks, as if Xcode is doing a better job here. 🙂
I did and tested again. The code looks good and works well. I am approving the commit. The PR can get merged now. Thanks for the review and your addition. |
As discussed in cryptomator/cryptomator#3417, we need to replace the deprecated
kLSSharedFileListSessionLoginItems.This PR does:
SMAppServiceAPI to enable/disable login items (used to start an app after login)SMAppServiceAPI without user interactionSMAppServiceAPI is available since macOS 13, the legacy API is used on macOS 11 and 12, theSMAppServiceAPI for macOS 13+