Feature: Autostart for DE following the freedesktop standard #80
Conversation
WalkthroughThese changes introduce a new JVM property, Changes
Poem
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 Configuration 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: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- README.md (1 hunks)
- src/main/java/module-info.java (2 hunks)
- src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java (1 hunks)
- src/main/resources/META-INF/services/org.cryptomator.integrations.autostart.AutoStartProvider (1 hunks)
- src/test/java/org/cryptomator/linux/autostart/FreeDesktopAutoStartIT.java (1 hunks)
Additional context used
LanguageTool
README.md
[style] ~7-~7: This phrase is redundant (‘G’ stands for ‘graphic’). Use simply “svgs”.
Context: ...r- specifies the directory from which svg images for the tray icon are loaded *cryptom...(ACRONYM_TAUTOLOGY)
Markdownlint
README.md
8-8: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
7-7: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
8-8: null
Files should end with a single newline character(MD047, single-trailing-newline)
Additional comments not posted (8)
src/main/resources/META-INF/services/org.cryptomator.integrations.autostart.AutoStartProvider (1)
1-1: LGTM! Verify the class existence.The changes are approved.
However, ensure that the class
org.cryptomator.linux.autostart.FreedesktopAutoStartServiceexists and is correctly named.Verification successful
Class Verified:
The class
org.cryptomator.linux.autostart.FreedesktopAutoStartServiceexists in the codebase.
- Location:
src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the class `FreedesktopAutoStartService`. # Test: Search for the class file. Expect: The class file should exist. fd 'FreedesktopAutoStartService.java'Length of output: 116
src/test/java/org/cryptomator/linux/autostart/FreeDesktopAutoStartIT.java (2)
12-12: Consider making the service instance final.The service instance can be made final for better immutability.
- FreedesktopAutoStartService inTest = new FreedesktopAutoStartService(); + final FreedesktopAutoStartService inTest = new FreedesktopAutoStartService();
21-21: Remove unnecessary blank line.There is an unnecessary blank line between the test methods.
-src/main/java/module-info.java (1)
5-5: LGTM! Verify the import and provision.The changes are approved.
However, ensure that the import and provision for
FreedesktopAutoStartServiceare accurate.Also applies to: 19-19
Verification successful
Verification Successful: The import and provision for
FreedesktopAutoStartServiceare accurate.
- The file
FreedesktopAutoStartService.javaexists in the expected directory.AutoStartProvideris correctly provided withFreedesktopAutoStartServicein themodule-info.javafile.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import and provision for `FreedesktopAutoStartService`. # Test: Search for the class file. Expect: The class file should exist. fd 'FreedesktopAutoStartService.java'Length of output: 116
Script:
#!/bin/bash # Description: Verify the provision of `AutoStartProvider` with `FreedesktopAutoStartService`. # Test: Search for the provision of `AutoStartProvider` with `FreedesktopAutoStartService`. rg 'AutoStartProvider.*FreedesktopAutoStartService'Length of output: 146
src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java (4)
42-52: Ensure proper handling ofexecValueandxdgConfigDirString.The constructor correctly initializes the
autostartFileand handles theexecValuebased on system properties. However, consider adding a check to ensureexecValueis not empty after the fallback logic.- execValue = ProcessHandle.current().info().command().orElse(""); + execValue = ProcessHandle.current().info().command().orElseThrow(() -> new IllegalStateException("Command path not found"));
72-75: LGTM!The
isEnabledmethod correctly checks if theautostartFileexists.
1-41: LGTM!The class-level annotations and field definitions are appropriate and correctly used.
77-81: Verify the correctness ofisSupportedand address the TODO comment.The method checks if the parent directory of
autostartFileexists, which is a good initial check. However, the TODO comment suggests further research might be needed to ensure comprehensive support.Do you want me to assist with the research on which Desktop Environments support this feature?
| @@ -5,4 +5,4 @@ Linux-specific implemenations of the [integrations-api](https://github.com/crypt | |||
|
|
|||
| This project uses the following JVM properties: | |||
| * `cryptomator.integrationsLinux.trayIconsDir` - specifies the directory from which svg images for the tray icon are loaded | |||
|
|
|||
| * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator | |||
There was a problem hiding this comment.
Ensure the file ends with a single newline character.
The file should end with a single newline character.
- * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator
+ * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator
+Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator | |
| * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator | |
Tools
Markdownlint
8-8: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
8-8: null
Files should end with a single newline character(MD047, single-trailing-newline)
Remove trailing spaces.
There is a trailing space at the end of the line.
- * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator
+ * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting CryptomatorCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator | |
| * `cryptomator.integrationsLinux.autoStartCmd` - specifies the command used for starting Cryptomator |
Tools
Markdownlint
8-8: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
8-8: null
Files should end with a single newline character(MD047, single-trailing-newline)
| @Override | ||
| public void disable() throws ToggleAutoStartFailedException { | ||
| try { | ||
| Files.deleteIfExists(autostartFile); | ||
| } catch (IOException e) { | ||
| throw new ToggleAutoStartFailedException("Failed to deactivate Cryptomator autostart for GNOME desktop environment.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add logging for exceptions in disable method.
While the method correctly handles IOException by throwing ToggleAutoStartFailedException, consider logging the exception details for better debugging.
- throw new ToggleAutoStartFailedException("Failed to deactivate Cryptomator autostart for GNOME desktop environment.", e);
+ LOG.error("Failed to delete autostart file", e);
+ throw new ToggleAutoStartFailedException("Failed to deactivate Cryptomator autostart for GNOME desktop environment.", e);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void disable() throws ToggleAutoStartFailedException { | |
| try { | |
| Files.deleteIfExists(autostartFile); | |
| } catch (IOException e) { | |
| throw new ToggleAutoStartFailedException("Failed to deactivate Cryptomator autostart for GNOME desktop environment.", e); | |
| } | |
| } | |
| @Override | |
| public void disable() throws ToggleAutoStartFailedException { | |
| try { | |
| Files.deleteIfExists(autostartFile); | |
| } catch (IOException e) { | |
| LOG.error("Failed to delete autostart file", e); | |
| throw new ToggleAutoStartFailedException("Failed to deactivate Cryptomator autostart for GNOME desktop environment.", e); | |
| } | |
| } |
| @Override | ||
| public void enable() throws ToggleAutoStartFailedException { | ||
| try { | ||
| Files.writeString(autostartFile, content, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING); | ||
| } catch (IOException e) { | ||
| throw new ToggleAutoStartFailedException("Failed to activate Cryptomator autostart for GNOME desktop environment.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add logging for exceptions in enable method.
While the method correctly handles IOException by throwing ToggleAutoStartFailedException, consider logging the exception details for better debugging.
- throw new ToggleAutoStartFailedException("Failed to activate Cryptomator autostart for GNOME desktop environment.", e);
+ LOG.error("Failed to write autostart file", e);
+ throw new ToggleAutoStartFailedException("Failed to activate Cryptomator autostart for GNOME desktop environment.", e);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void enable() throws ToggleAutoStartFailedException { | |
| try { | |
| Files.writeString(autostartFile, content, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING); | |
| } catch (IOException e) { | |
| throw new ToggleAutoStartFailedException("Failed to activate Cryptomator autostart for GNOME desktop environment.", e); | |
| } | |
| } | |
| @Override | |
| public void enable() throws ToggleAutoStartFailedException { | |
| try { | |
| Files.writeString(autostartFile, content, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING); | |
| } catch (IOException e) { | |
| LOG.error("Failed to write autostart file", e); | |
| throw new ToggleAutoStartFailedException("Failed to activate Cryptomator autostart for GNOME desktop environment.", e); | |
| } | |
| } |
| import org.junit.jupiter.api.TestMethodOrder; | ||
|
|
||
| @TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
| public class FreeDesktopAutoStartIT { |
There was a problem hiding this comment.
Class name should be consistent.
The class name FreeDesktopAutoStartIT should be consistent with the service class name FreedesktopAutoStartService.
- public class FreeDesktopAutoStartIT {
+ public class FreedesktopAutoStartIT {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class FreeDesktopAutoStartIT { | |
| public class FreedesktopAutoStartIT { |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java (1)
43-53: Constructor review ofFreedesktopAutoStartService.The constructor handles the default value for
XDG_CONFIG_HOMEand initializesautostartFileandcontent. UsingProcessHandleas a fallback for the command is a clever use of Java APIs. However, ensure that an empty command (execValue) does not lead to unintended behavior or security issues.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java (1 hunks)
- src/test/java/org/cryptomator/linux/autostart/FreedesktopAutoStartIT.java (1 hunks)
Additional comments not posted (2)
src/test/java/org/cryptomator/linux/autostart/FreedesktopAutoStartIT.java (2)
14-19: Review oftestAutostartEnablemethod.This test checks if the autostart can be enabled without throwing an exception and verifies the enabled state. It's good to see use of
assertDoesNotThrowto explicitly check for no exceptions. However, consider adding more assertions to verify the actual effects on the filesystem or system configuration, if possible.
22-27: Review oftestAutostartDisablemethod.Similar to the enable test, this test checks for no exceptions and a correct disabled state. Again, additional assertions to check the actual file system or system state changes would make these tests more robust.
| @TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
| public class FreedesktopAutoStartIT { | ||
|
|
||
| FreedesktopAutoStartService inTest = new FreedesktopAutoStartService(); |
There was a problem hiding this comment.
Initialize FreedesktopAutoStartService directly in tests.
Direct initialization of inTest as a field might not be the best approach in tests, especially if state between tests needs to be isolated. Consider using @BeforeEach for initialization to ensure tests do not affect each other.
| @CheckAvailability | ||
| public boolean isSupported() { | ||
| //TODO: might need to research which Desktop Environments support this | ||
| return hasExecValue && Files.exists(autostartFile.getParent()); | ||
| } |
There was a problem hiding this comment.
Review of isSupported method.
The method checks if the autostart is supported by verifying the execValue and the existence of the parent directory of the autostart file. The TODO comment suggests further research is needed. It would be beneficial to address this TODO to ensure the method's reliability across different environments.
Would you like me to help with the research or should I open a GitHub issue to track this task?
src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/autostart/FreedesktopAutoStartService.java
Show resolved
Hide resolved
|
@overheadhunter Fyi, i synchronized the methods |
This PR adds an implementation of the AutoStart SPI: The
FreeDesktopAutostartService.The implementation is based on the freedesktop autostart specificiation.