Improve Google Drive Preset #2921 #3270
Conversation
|
|
WalkthroughThe project has been updated to enhance the Google Drive location presets, particularly focusing on macOS and Windows platforms. The changes include dynamic path detection for various locales and account configurations, as well as a potential UI refactor to handle a more extensive list of cloud locations. This addresses the need for better Google Drive integration across different user setups. Changes
Assessment against linked issues
PoemAs the moon doth rise and the night is nigh, 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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- The code now defaults to searching through '~/Library/CloudStorage/GoogleDrive-*/*' before using the fallback options.
…2921 - Change imports accordingly in module-info.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/main/java/module-info.java (2 hunks)
- src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java (1 hunks)
- src/main/java/org/cryptomator/common/locationpresets/GoogleDriveWindowsLocationPresetsProvider.java (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/main/java/module-info.java
- src/main/java/org/cryptomator/common/locationpresets/GoogleDriveWindowsLocationPresetsProvider.java
Additional comments: 3
src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java (3)
22-22: The
PATTERNpredicate is correctly defined to match directory names starting with "GoogleDrive-".32-40: The logic in
getLocationsmethod correctly checks for the presence of Google Drive locations and handles fallbacks. Ensure that theisLocationPresentmethod correctly resolves theLOCATIONpath.43-44: The
isPresentmethod correctly combines the check for the standard location and the fallback locations.
src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java
Show resolved
Hide resolved
| public static boolean isLocationPresent() { | ||
| try (DirectoryStream<Path> stream = Files.newDirectoryStream(LOCATION, "GoogleDrive-*")) { | ||
| return stream.iterator().hasNext(); | ||
| } catch (IOException e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Consider catching UncheckedIOException in isLocationPresent to handle potential runtime exceptions from the stream operations.
try (DirectoryStream<Path> stream = Files.newDirectoryStream(LOCATION, "GoogleDrive-*")) {
return stream.iterator().hasNext();
} catch (IOException e) {
return false;
+ } catch (UncheckedIOException e) {
+ return false;
}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.
| public static boolean isLocationPresent() { | |
| try (DirectoryStream<Path> stream = Files.newDirectoryStream(LOCATION, "GoogleDrive-*")) { | |
| return stream.iterator().hasNext(); | |
| } catch (IOException e) { | |
| return false; | |
| } | |
| public static boolean isLocationPresent() { | |
| try (DirectoryStream<Path> stream = Files.newDirectoryStream(LOCATION, "GoogleDrive-*")) { | |
| return stream.iterator().hasNext(); | |
| } catch (IOException e) { | |
| return false; | |
| } catch (UncheckedIOException e) { | |
| return false; | |
| } |
src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java
Show resolved
Hide resolved
| private Stream<LocationPreset> getCloudStorageDirLocations() { | ||
| try (var dirStream = Files.list(LOCATION)) { | ||
| var presets = dirStream.filter(path -> Files.isDirectory(path) && PATTERN.test(path.getFileName().toString())) | ||
| .flatMap(accountPath -> { | ||
| try { | ||
| return Files.list(accountPath) | ||
| .filter(Files::isDirectory) | ||
| .map(drivePath -> new LocationPreset(getDriveLocationString(accountPath, drivePath), drivePath)); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }).toList(); | ||
| return presets.stream(); | ||
| } | ||
| catch (IOException | UncheckedIOException e) { | ||
| return Stream.of(); | ||
| } |
There was a problem hiding this comment.
Refine the exception handling in getCloudStorageDirLocations to avoid throwing RuntimeException. Consider logging the exception and returning an empty stream instead.
try (var dirStream = Files.list(LOCATION)) {
// ... existing code ...
} catch (IOException e) {
- throw new RuntimeException(e);
+ // Log the exception and handle it appropriately
+ return Stream.empty();
}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.
| private Stream<LocationPreset> getCloudStorageDirLocations() { | |
| try (var dirStream = Files.list(LOCATION)) { | |
| var presets = dirStream.filter(path -> Files.isDirectory(path) && PATTERN.test(path.getFileName().toString())) | |
| .flatMap(accountPath -> { | |
| try { | |
| return Files.list(accountPath) | |
| .filter(Files::isDirectory) | |
| .map(drivePath -> new LocationPreset(getDriveLocationString(accountPath, drivePath), drivePath)); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } | |
| }).toList(); | |
| return presets.stream(); | |
| } | |
| catch (IOException | UncheckedIOException e) { | |
| return Stream.of(); | |
| } | |
| private Stream<LocationPreset> getCloudStorageDirLocations() { | |
| try (var dirStream = Files.list(LOCATION)) { | |
| var presets = dirStream.filter(path -> Files.isDirectory(path) && PATTERN.test(path.getFileName().toString())) | |
| .flatMap(accountPath -> { | |
| try { | |
| return Files.list(accountPath) | |
| .filter(Files::isDirectory) | |
| .map(drivePath -> new LocationPreset(getDriveLocationString(accountPath, drivePath), drivePath)); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } | |
| }).toList(); | |
| return presets.stream(); | |
| } | |
| catch (IOException | UncheckedIOException e) { | |
| // Log the exception and handle it appropriately | |
| return Stream.empty(); | |
| } | |
| } |
There was a problem hiding this comment.
Or throw a new UncheckedIOException(e), which will also result in an empty stream due to the outer catch block.
The suggestion by @coderabbitai has the advantage to only skip the accountPath in which the exception occurred, retaining other results.
Regardless, I would suggest to move the code from within the flatMap to a new private function for increased readability.
There was a problem hiding this comment.
@overheadhunter Throwing a new UncheckedIOException(e) is indeed a valid alternative, and I concur that refactoring the code within the flatMap into a private method would enhance readability. Here's how it could look:
private Stream<LocationPreset> getCloudStorageDirLocations() {
try (var dirStream = Files.list(LOCATION)) {
return dirStream.filter(path -> Files.isDirectory(path) && PATTERN.test(path.getFileName().toString()))
.flatMap(this::getPresetsFromAccountPath)
.toList().stream();
} catch (IOException | UncheckedIOException e) {
// Log the exception and handle it appropriately
return Stream.empty();
}
}
private Stream<LocationPreset> getPresetsFromAccountPath(Path accountPath) {
try {
return Files.list(accountPath)
.filter(Files::isDirectory)
.map(drivePath -> new LocationPreset(getDriveLocationString(accountPath, drivePath), drivePath));
} catch (IOException e) {
// Log the exception and handle it appropriately
return Stream.empty();
}
}This separates the concerns and makes the stream operations more manageable.
src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java
Show resolved
Hide resolved
|
Hey @siard-y, Thank you for your patience! |
There was a problem hiding this comment.
Again: Thank you very much for your contribution! None of my notes are major issues, but should be addressed regardless.
A few notes in general:
- I am not sure if there is always a drive dir or if it's possible for the account dir to already contain the cloud content.
- This PR duplicates a few things, especially the fallback paths.
- This PR does not use
com.google.drivefs.plist; see: Not possible to create a vault in Google Drive in OS X #1930 (comment) - The bot is onto something here. https://github.com/cryptomator/cryptomator/pull/3270/files#r1440610556 -- I'd suggest catching all IOEs and UIOEs, printing them and then returning
Stream#emptyorfalsewhere appropriate. - Finally we need to keep the dialog size in mind.
Maybe @infeo can continue from here on.
|
|
||
| @OperatingSystem(MAC) | ||
| public final class GoogleDriveMacLocationPresetsProvider implements LocationPresetsProvider { | ||
| private static final Path LOCATION = LocationPresetsProvider.resolveLocation("~/Library/CloudStorage/").toAbsolutePath(); |
There was a problem hiding this comment.
I am not a big fan of the name LOCATION since it's actually the parent, maybe think of something like ROOT_LOCATION.
| return isLocationPresent() || FALLBACK_LOCATIONS.stream().anyMatch(Files::isDirectory); | ||
| } | ||
|
|
||
| public static boolean isLocationPresent() { |
There was a problem hiding this comment.
Again, not a fan of this name.
src/main/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java
Show resolved
Hide resolved
| try (var dirStream = Files.list(LOCATION)) { | ||
| var presets = dirStream.filter(path -> Files.isDirectory(path) && PATTERN.test(path.getFileName().toString())) |
There was a problem hiding this comment.
Files#newDirectoryStream might be a better choice here as well.
|
|
||
| public static boolean isLocationPresent() { | ||
| try (DirectoryStream<Path> stream = Files.newDirectoryStream(LOCATION, "GoogleDrive-*")) { | ||
| return stream.iterator().hasNext(); |
There was a problem hiding this comment.
Please filter for directories.
There was a problem hiding this comment.
or directly use my bad, this is no regular stream.stream.anyMatch(Files::isDirectory)
My suggestion:
StreamSupport.stream(stream.spliterator(), false).anyMatch(Files::isDirectory)| } else if(FALLBACK_LOCATIONS.stream().anyMatch(Files::isDirectory)) { | ||
| return getFallbackLocation(); | ||
| } else { | ||
| return Stream.of(); |
There was a problem hiding this comment.
This can be simplified. getFallbackLocation will return an empty stream if nothing matches.
| .filter(Files::isDirectory) | ||
| .map(drivePath -> new LocationPreset(getDriveLocationString(accountPath, drivePath), drivePath)); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
This should behave the same way as the outer catch.
Good question. @tobihagemann do you have any insights on Google Drive behaviour?
Also here, we need some Google Drive on macOS knowledge.
The current approach seems sufficient and robust to me. Adding more to it would require handling of failed file reading. An then still, we could fail because the directory is not readable. I would skip this for now.
Also thought about that, but need to test what happens on an overflow. (e.g. where are the ellipsis placed) |
This pull request addresses issue #2921, my first open-source contribution.
When creating a vault on MacOS, the code will now look through
~/Library/GoogleCloud/GoogleDrive-*/*if it exists. Otherwise it will use the fallback options, which remain in the code.Here is a quick example of how it looks in the UI for now:
closes #2921
Summary by CodeRabbit
New Features
Refactor
Documentation