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
Improve Google Drive Preset #2921 #3270
base: develop
Are you sure you want to change the base?
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 (
|
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
PATTERN
predicate is correctly defined to match directory names starting with "GoogleDrive-".32-40: The logic in
getLocations
method correctly checks for the presence of Google Drive locations and handles fallbacks. Ensure that theisLocationPresent
method correctly resolves theLOCATION
path.43-44: The
isPresent
method 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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#empty
orfalse
where 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please filter for directories.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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