Feature: Implementation of Quick Access API for KDE Dolphin #85
Conversation
WalkthroughThe updates introduce the 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: 3
Outside diff range, codebase verification and nitpick comments (1)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
28-30: Fix the typo in the class-level comment.The comment has a typo: "Implemenation" should be "Implementation."
- * Implemenation of the {@link QuickAccessService} for KDE desktop environments using Dolphin file browser. + * Implementation of the {@link QuickAccessService} for KDE desktop environments using Dolphin file browser.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1 hunks)
- src/main/resources/xbel-1.0.dtd (1 hunks)
- src/main/resources/xbel-1.0.xsd (1 hunks)
- src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesIT.java (1 hunks)
- src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java (1 hunks)
- src/test/resources/user-places.xbel (1 hunks)
Files skipped from review due to trivial changes (2)
- src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesIT.java
- src/test/java/org/cryptomator/linux/quickaccess/DolphinPlacesTest.java
Additional comments not posted (19)
src/main/resources/xbel-1.0.dtd (7)
43-47: Ensure thexbelelement definition is correct.The
xbelelement is defined with the necessary attributes and child elements. It appears to be correct and adheres to the XBEL standard.
48-48: Ensure thetitleelement definition is correct.The
titleelement is defined to contain parsed character data. It appears to be correct.
52-57: Ensure theinfoelement definition is correct.The
infoelement is defined to contain one or moremetadataelements. It appears to be correct.
54-57: Ensure themetadataelement definition is correct.The
metadataelement is defined to be empty and has anownerattribute. It appears to be correct.
61-65: Ensure thefolderelement definition is correct.The
folderelement is defined with the necessary attributes and child elements. It appears to be correct and adheres to the XBEL standard.
69-73: Ensure thebookmarkelement definition is correct.The
bookmarkelement is defined with the necessary attributes and child elements. It appears to be correct and adheres to the XBEL standard.
91-94: Ensure thealiaselement definition is correct.The
aliaselement is defined with the necessary attribute and references other elements. It appears to be correct and adheres to the XBEL standard.src/test/resources/user-places.xbel (5)
1-2: Ensure the XML declaration and DOCTYPE are correct.The XML declaration and DOCTYPE are defined correctly. They appear to be correct and adhere to the XML standards.
4-16: Ensure theinfoelement and its child elements are correct.The
infoelement contains metadata related to the KDE environment. It appears to be correct and adheres to the XBEL schema.
18-137: Ensure thebookmarkelements and their child elements are correct.The
bookmarkelements contain various bookmarks with metadata. They appear to be correct and adhere to the XBEL schema.
150-157: Ensure theseparatorelement and its child elements are correct.The
separatorelement contains metadata related to the KDE environment. It appears to be correct and adheres to the XBEL schema.
178-178: Ensure the closingxbelelement is correct.The closing
xbelelement is defined correctly. It appears to be correct and adheres to the XBEL schema.src/main/resources/xbel-1.0.xsd (7)
38-48: Ensure thexbelelement definition is correct.The
xbelelement is defined with the necessary attributes and child elements. It appears to be correct and adheres to the XBEL standard.
59-59: Ensure thetitleelement definition is correct.The
titleelement is defined to contain a string. It appears to be correct.
61-67: Ensure theinfoelement definition is correct.The
infoelement is defined to contain one or moremetadataelements. It appears to be correct.
68-75: Ensure themetadataelement definition is correct.The
metadataelement is defined to contain any elements and has anownerattribute. It appears to be correct.
80-90: Ensure thefolderelement definition is correct.The
folderelement is defined with the necessary attributes and child elements. It appears to be correct and adheres to the XBEL standard.
103-112: Ensure thebookmarkelement definition is correct.The
bookmarkelement is defined with the necessary attributes and child elements. It appears to be correct and adheres to the XBEL standard.
135-142: Ensure thealiaselement definition is correct.The
aliaselement is defined with the necessary attribute and references other elements. It appears to be correct and adheres to the XBEL standard.
| private static class DolphinPlacesEntry implements QuickAccessEntry { | ||
|
|
||
| private final String id; | ||
| private volatile boolean isRemoved = false; | ||
|
|
||
| DolphinPlacesEntry(String id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| @Override | ||
| public void remove() throws QuickAccessServiceException { | ||
| try { | ||
| MODIFY_LOCK.lock(); | ||
| if (isRemoved) { | ||
| return; | ||
| } | ||
| if (Files.size(PLACES_FILE) > MAX_FILE_SIZE) { | ||
| throw new IOException("File %s exceeds size of %d bytes".formatted(PLACES_FILE, MAX_FILE_SIZE)); | ||
| } | ||
| var placesContent = Files.readString(PLACES_FILE); | ||
| int idIndex = placesContent.lastIndexOf(id); | ||
| if (idIndex == -1) { | ||
| isRemoved = true; | ||
| return; //we assume someone has removed our entry | ||
| } | ||
| //validate | ||
| xmlValidator.validate(new StreamSource(new StringReader(placesContent))); | ||
| //modify | ||
| var placesContentPart1 = placesContent.substring(0, idIndex); | ||
| int openingTagIndex = placesContentPart1.lastIndexOf("<bookmark href="); | ||
| var contentToWrite1 = placesContentPart1.substring(0, openingTagIndex).stripTrailing(); | ||
|
|
||
| int closingTagIndex = placesContent.indexOf("</bookmark>", idIndex); | ||
| var part2Tmp = placesContent.substring(closingTagIndex + "</bookmark>".length()).split("\\v*", 2); //removing leading vertical whitespaces | ||
| var contentToWrite2 = part2Tmp.length == 1 ? part2Tmp[0] : part2Tmp[1]; | ||
|
|
||
| try (var writer = Files.newBufferedWriter(TMP_FILE, StandardCharsets.UTF_8, StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) { | ||
| writer.write(contentToWrite1); | ||
| writer.newLine(); | ||
| writer.write(contentToWrite2); | ||
| } | ||
| // save | ||
| Files.move(TMP_FILE, PLACES_FILE, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); | ||
| isRemoved = true; | ||
| } catch (IOException | SAXException e) { | ||
| throw new QuickAccessServiceException("Removing entry from KDE places file failed.", e); | ||
| } finally { | ||
| MODIFY_LOCK.unlock(); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in the remove method.
The remove method appears to handle errors correctly by catching exceptions and unlocking the file in the finally block. However, consider logging the exceptions for better debugging.
} catch (IOException | SAXException e) {
throw new QuickAccessServiceException("Removing entry from KDE places file failed.", e);
} finally {
MODIFY_LOCK.unlock();
+ // Consider logging the exception
+ if (e != null) {
+ logger.error("Exception occurred while removing entry from KDE places file.", e);
+ }
}Committable suggestion was skipped due to low confidence.
| public QuickAccessService.QuickAccessEntry add(Path target, String displayName) throws QuickAccessServiceException { | ||
| String id = UUID.randomUUID().toString(); | ||
| try { | ||
| MODIFY_LOCK.lock(); | ||
| if (Files.size(PLACES_FILE) > MAX_FILE_SIZE) { | ||
| throw new IOException("File %s exceeds size of %d bytes".formatted(PLACES_FILE, MAX_FILE_SIZE)); | ||
| } | ||
| var placesContent = Files.readString(PLACES_FILE); | ||
| //validate | ||
| xmlValidator.validate(new StreamSource(new StringReader(placesContent))); | ||
| // modify | ||
| int insertIndex = placesContent.lastIndexOf("</xbel>"); //cannot be -1 due to validation | ||
| try (var writer = Files.newBufferedWriter(TMP_FILE, StandardCharsets.UTF_8, StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) { | ||
| writer.write(placesContent, 0, insertIndex); | ||
| writer.newLine(); | ||
| writer.write(ENTRY_TEMPLATE.formatted(target.toUri(), displayName, id).indent(1)); | ||
| writer.newLine(); | ||
| writer.write(placesContent, insertIndex, placesContent.length() - insertIndex); | ||
| } | ||
| // save | ||
| Files.move(TMP_FILE, PLACES_FILE, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); | ||
| return new DolphinPlacesEntry(id); | ||
| } catch (SAXException | IOException e) { | ||
| throw new QuickAccessServiceException("Adding entry to KDE places file failed.", e); | ||
| } finally { | ||
| MODIFY_LOCK.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in the add method.
The add method appears to handle errors correctly by catching exceptions and unlocking the file in the finally block. However, consider logging the exceptions for better debugging.
} catch (SAXException | IOException e) {
throw new QuickAccessServiceException("Adding entry to KDE places file failed.", e);
} finally {
MODIFY_LOCK.unlock();
+ // Consider logging the exception
+ if (e != null) {
+ logger.error("Exception occurred while adding entry to KDE places file.", e);
+ }
}Committable suggestion was skipped due to low confidence.
| @CheckAvailability | ||
| public static boolean isSupported() { | ||
| try { | ||
| var nautilusExistsProc = new ProcessBuilder().command("test", "`command -v dolphin`").start(); | ||
| if (nautilusExistsProc.waitFor(5000, TimeUnit.MILLISECONDS)) { | ||
| return nautilusExistsProc.exitValue() == 0; | ||
| } | ||
| } catch (IOException | InterruptedException e) { | ||
| //NO-OP | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Ensure proper error handling in the isSupported method.
The isSupported method appears to handle errors by ignoring them (NO-OP). Consider logging the exceptions for better debugging.
} catch (IOException | InterruptedException e) {
//NO-OP
+ // Consider logging the exception
+ logger.error("Exception occurred while checking if Dolphin is supported.", 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.
| @CheckAvailability | |
| public static boolean isSupported() { | |
| try { | |
| var nautilusExistsProc = new ProcessBuilder().command("test", "`command -v dolphin`").start(); | |
| if (nautilusExistsProc.waitFor(5000, TimeUnit.MILLISECONDS)) { | |
| return nautilusExistsProc.exitValue() == 0; | |
| } | |
| } catch (IOException | InterruptedException e) { | |
| //NO-OP | |
| } | |
| return false; | |
| @CheckAvailability | |
| public static boolean isSupported() { | |
| try { | |
| var nautilusExistsProc = new ProcessBuilder().command("test", "`command -v dolphin`").start(); | |
| if (nautilusExistsProc.waitFor(5000, TimeUnit.MILLISECONDS)) { | |
| return nautilusExistsProc.exitValue() == 0; | |
| } | |
| } catch (IOException | InterruptedException e) { | |
| //NO-OP | |
| // Consider logging the exception | |
| logger.error("Exception occurred while checking if Dolphin is supported.", e); | |
| } | |
| return false; |
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I have a few questions/notes:
- What are
src/main/resources/xbel-1.0.dtdandsrc/test/resources/user-places.xbelused for? - Our temp file is not guarded against concurrent modifications. This is fine for now, but will cause problems if we ever switch to a multi-process architecture.
- What happens if the application crashes?
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
The temp file does not have to be guarded against concurrent modifications. It is a temporary file of Cryptomator (even indicated as such due to the file ending) and lives normally only for a very short time. We can safely assume, that users or processes do not edit randomly some forgein file. Regarding multiprocess architecture: My current vision is, that a vault process only contains the mounting. Any additional/convenience action will still reside in Cryptomator. An there will be only one Cryptomator instance.
If an entry is present in the XBEL file, it stays there. But the user can remove the entry by himself over the Dolphin UI. So there is no need of "automatic" cleanup.
|
* use correct regex for vertical whitespace splitting * only match tag start + tag name
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/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
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/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
infeo
force-pushed
the
feature/quick-access-dolphin
branch
from
66c29cc
to
a5dca5b
Compare
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/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
There was a problem hiding this comment.
LGTM apart from two last notes.
The temp file does not have to be guarded against concurrent modifications. It is a temporary file of Cryptomator (even indicated as such due to the file ending) and lives normally only for a very short time. We can safely assume, that users or processes do not edit randomly some forgein file.
Regarding multiprocess architecture: My current vision is, that a vault process only contains the mounting. Any additional/convenience action will still reside in Cryptomator. An there will be only one Cryptomator instance.
I was primarily referring to the second scenario, but in that case I concur.
If an entry is present in the XBEL file, it stays there. But the user can remove the entry by himself over the Dolphin UI. So there is no need of "automatic" cleanup.
👍
xbel-1.0.dtdis the original definition. I converted it with the IDE into a xsd file, but wanted to keep the source.src/test/resources/user-places.xbelwas intended for testing, but is obsolete now.
In that case I'd suggest deleting src/test/resources/user-places.xbel.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (2)
37-40: Consider adding a comment to explain the choice of constants.Adding comments to explain the choice of constants such as
MAX_FILE_SIZEand the paths forPLACES_FILEandTMP_FILEcan improve code readability and maintainability.+ // Maximum file size for the places file (32 KB) private static final int MAX_FILE_SIZE = 1 << 15; //xml is quite verbose + // Path to the KDE places file private static final Path PLACES_FILE = Path.of(System.getProperty("user.home"), ".local/share/user-places.xbel"); + // Path to the temporary file used for atomic operations private static final Path TMP_FILE = Path.of(System.getProperty("java.io.tmpdir"), "user-places.xbel.cryptomator.tmp");
148-157: Improve exception message inindexOfEntryOpeningTagmethod.The exception message in the
indexOfEntryOpeningTagmethod can be improved for clarity.- throw new IllegalStateException("File " + PLACES_FILE + " is valid xbel file, but does not contain opening bookmark tag."); + throw new IllegalStateException("The file " + PLACES_FILE + " is a valid XBEL file but does not contain an opening bookmark tag.");
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1 hunks)
Additional context used
Learnings (2)
Common learnings
Learnt from: infeo PR: cryptomator/integrations-linux#77 File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50 Timestamp: 2024-07-12T20:09:01.834Z Learning: In `NautilusBookmarks`, file operations are handled by the JDK, which manages the closing of any opened resources, so try-with-resources is not necessary.src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1)
Learnt from: infeo PR: cryptomator/integrations-linux#77 File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50 Timestamp: 2024-07-12T20:09:01.834Z Learning: The `add` method in `NautilusBookmarks` handles the case where the `BOOKMARKS_FILE` does not exist by catching the IOException and wrapping it in a `QuickAccessServiceException`.
Additional comments not posted (3)
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (3)
68-96: Ensure proper error handling in theaddmethod.The
addmethod appears to handle errors correctly by catching exceptions and unlocking the file in thefinallyblock. However, consider logging the exceptions for better debugging.} catch (SAXException | IOException e) { throw new QuickAccessServiceException("Adding entry to KDE places file failed.", e); } finally { MODIFY_LOCK.unlock(); + // Consider logging the exception + if (e != null) { + logger.error("Exception occurred while adding entry to KDE places file.", e); + } }
108-145: Ensure proper error handling in theremovemethod.The
removemethod appears to handle errors correctly by catching exceptions and unlocking the file in thefinallyblock. However, consider logging the exceptions for better debugging.} catch (IOException | SAXException e) { throw new QuickAccessServiceException("Removing entry from KDE places file failed.", e); } finally { MODIFY_LOCK.unlock(); + // Consider logging the exception + if (e != null) { + logger.error("Exception occurred while removing entry from KDE places file.", e); + } }
160-170: Ensure proper error handling in theisSupportedmethod.The
isSupportedmethod appears to handle errors by ignoring them (NO-OP). Consider logging the exceptions for better debugging.} catch (IOException | InterruptedException e) { //NO-OP + // Consider logging the exception + logger.error("Exception occurred while checking if Dolphin is supported.", e); }
| static { | ||
| SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); | ||
| try (var schemaDefinition = DolphinPlaces.class.getResourceAsStream("/xbel-1.0.xsd")) { | ||
| Source schemaFile = new StreamSource(schemaDefinition); | ||
| xmlValidator = factory.newSchema(schemaFile).newValidator(); | ||
| } catch (IOException | SAXException e) { | ||
| throw new IllegalStateException("Failed to load included XBEL schema definition file.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper resource handling in static initializations.
The static block initializes the XML validator. Ensure that resources are properly handled, and consider logging the exception for better debugging.
- } catch (IOException | SAXException e) {
+ } catch (IOException | SAXException e) {
+ // Log the exception for better debugging
+ logger.error("Failed to load included XBEL schema definition file.", e);
throw new IllegalStateException("Failed to load included XBEL schema definition file.", 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.
| static { | |
| SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); | |
| try (var schemaDefinition = DolphinPlaces.class.getResourceAsStream("/xbel-1.0.xsd")) { | |
| Source schemaFile = new StreamSource(schemaDefinition); | |
| xmlValidator = factory.newSchema(schemaFile).newValidator(); | |
| } catch (IOException | SAXException e) { | |
| throw new IllegalStateException("Failed to load included XBEL schema definition file.", e); | |
| } | |
| } | |
| static { | |
| SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); | |
| try (var schemaDefinition = DolphinPlaces.class.getResourceAsStream("/xbel-1.0.xsd")) { | |
| Source schemaFile = new StreamSource(schemaDefinition); | |
| xmlValidator = factory.newSchema(schemaFile).newValidator(); | |
| } catch (IOException | SAXException e) { | |
| // Log the exception for better debugging | |
| logger.error("Failed to load included XBEL schema definition file.", e); | |
| throw new IllegalStateException("Failed to load included XBEL schema definition file.", e); | |
| } | |
| } |
src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Outdated
Show resolved
Hide resolved
I tried this (against better knowledge I'd like to say). Honestly, the API to read and transform is vey clunky and far from readable. And the result is not the original document but a little bit different (e.g. the dtd node gets lost). And lastly, before getting into the API and account for every pitfall, i'll stayed with regex. With my approach we just use |
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/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/linux/quickaccess/DolphinPlaces.java
Additional context used
Learnings (1)
Common learnings
Learnt from: infeo PR: cryptomator/integrations-linux#77 File: src/main/java/org/cryptomator/linux/quickaccess/NautilusBookmarks.java:31-50 Timestamp: 2024-07-12T20:09:01.834Z Learning: In `NautilusBookmarks`, file operations are handled by the JDK, which manages the closing of any opened resources, so try-with-resources is not necessary.
This PR adds an implementation of the Quick Access API for KDE Dolphin.
This implementation defines the quick access area as Dolphin places section. The section allows a custom label and icon for the bookmark.
To add an entry, an xml object is added to the file
~/.local/share/user-places.xbelis added. The file format is XBEL. Before editing the file, it is validated against the XBEL schema definition.The entry is removed by editing the user-places.xbel file. To ensure data consistency, the file is never edited directly. Instead a copy is made and then with an atomic_move the original file overriden. Additionally, only one thread at a time can either add or remove an entry.