Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve Google Drive Preset #2921 #3270

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Improve Google Drive Preset #2921 #3270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17057e8
Add Google Drive presets improvement. cryptomator/2921
siard-y Jan 2, 2024
d85c6c8
Create separate files for GoogleDrive presets providers. cryptomator/…
siard-y Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
import org.cryptomator.common.locationpresets.DropboxLinuxLocationPresetsProvider;
import org.cryptomator.common.locationpresets.DropboxMacLocationPresetsProvider;
import org.cryptomator.common.locationpresets.DropboxWindowsLocationPresetsProvider;
import org.cryptomator.common.locationpresets.GoogleDriveLocationPresetsProvider;
import org.cryptomator.common.locationpresets.GoogleDriveMacLocationPresetsProvider;
import org.cryptomator.common.locationpresets.GoogleDriveWindowsLocationPresetsProvider;
import org.cryptomator.common.locationpresets.ICloudMacLocationPresetsProvider;
import org.cryptomator.common.locationpresets.ICloudWindowsLocationPresetsProvider;
import org.cryptomator.common.locationpresets.LeitzcloudLocationPresetsProvider;
Expand Down Expand Up @@ -56,7 +57,7 @@
provides Configurator with LogbackConfiguratorFactory;
provides LocationPresetsProvider with //
DropboxWindowsLocationPresetsProvider, DropboxMacLocationPresetsProvider, DropboxLinuxLocationPresetsProvider, //
GoogleDriveLocationPresetsProvider, //
GoogleDriveMacLocationPresetsProvider, GoogleDriveWindowsLocationPresetsProvider, //
ICloudWindowsLocationPresetsProvider, ICloudMacLocationPresetsProvider, //
LeitzcloudLocationPresetsProvider, //
MegaLocationPresetsProvider, //
Expand Down
107 changes: 107 additions & 0 deletions ...in/java/org/cryptomator/common/locationpresets/GoogleDriveMacLocationPresetsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package org.cryptomator.common.locationpresets;

import org.cryptomator.integrations.common.CheckAvailability;
import org.cryptomator.integrations.common.OperatingSystem;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import java.util.function.Predicate;
import java.util.regex.Pattern;
import java.util.stream.Stream;

import static org.cryptomator.integrations.common.OperatingSystem.Value.MAC;

@OperatingSystem(MAC)
public final class GoogleDriveMacLocationPresetsProvider implements LocationPresetsProvider {
private static final Path LOCATION = LocationPresetsProvider.resolveLocation("~/Library/CloudStorage/").toAbsolutePath();
JaniruTEC marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

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.

private static final Predicate<String> PATTERN = Pattern.compile("^GoogleDrive-[^/]+$").asMatchPredicate();

private static final List<Path> FALLBACK_LOCATIONS = Arrays.asList( //
LocationPresetsProvider.resolveLocation("~/GoogleDrive/My Drive"), //
LocationPresetsProvider.resolveLocation("~/Google Drive/My Drive"), //
LocationPresetsProvider.resolveLocation("~/GoogleDrive"), //
LocationPresetsProvider.resolveLocation("~/Google Drive") //
JaniruTEC marked this conversation as resolved.
Show resolved Hide resolved
);

@Override
public Stream<LocationPreset> getLocations() {
if(isLocationPresent()) {
return getCloudStorageDirLocations();
} else if(FALLBACK_LOCATIONS.stream().anyMatch(Files::isDirectory)) {
return getFallbackLocation();
} else {
return Stream.of();
Comment on lines +35 to +38
Copy link
Contributor

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.

}
}

@CheckAvailability
public static boolean isPresent() {
return isLocationPresent() || FALLBACK_LOCATIONS.stream().anyMatch(Files::isDirectory);
}

public static boolean isLocationPresent() {
Copy link
Contributor

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.

try (DirectoryStream<Path> stream = Files.newDirectoryStream(LOCATION, "GoogleDrive-*")) {
return stream.iterator().hasNext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please filter for directories.

Copy link
Member

@infeo infeo Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or directly use stream.anyMatch(Files::isDirectory) my bad, this is no regular stream.
My suggestion:

StreamSupport.stream(stream.spliterator(), false).anyMatch(Files::isDirectory)

} catch (IOException e) {
return false;
}
Comment on lines +47 to +52
Copy link

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.

Suggested change
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;
}

}

/**
* Returns Google Drive preset String.
*
* @param accountPath The path to the Google Drive account directory (e.g. {@code ~/Library/CloudStorage/GoogleDrive-username})
* @param drivePath The path to the Google Drive file directory, within the account directory. (e.g. {@code ~/Library/CloudStorage/GoogleDrive-username/drive_name})
* @return {@code String}. For example: "Google Drive - username - drive_name"
*/
private String getDriveLocationString(Path accountPath, Path drivePath) {
String accountName = accountPath.getFileName().toString().replace("GoogleDrive-", "");
String driveName = drivePath.getFileName().toString();

return STR."Google Drive - \{accountName} - \{driveName}";
infeo marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Handles searching through {@code ~/Library/CloudStorage/} for directories with the "{@code GoogleDrive-*}" pattern,
* and returns the corresponding presets.
*
* @return {@code Stream<LocationPreset>}. Displays as "{@code Google Drive - username - drive_name}"
*/
private Stream<LocationPreset> getCloudStorageDirLocations() {
try (var dirStream = Files.list(LOCATION)) {
var presets = dirStream.filter(path -> Files.isDirectory(path) && PATTERN.test(path.getFileName().toString()))
Comment on lines +76 to +77
Copy link
Contributor

@JaniruTEC JaniruTEC Jan 5, 2024 •
edited
Loading

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.

.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);
Copy link
Contributor

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.

}
}).toList();
return presets.stream();
infeo marked this conversation as resolved.
Show resolved Hide resolved
}
catch (IOException | UncheckedIOException e) {
return Stream.of();
}
Comment on lines +75 to +91
Copy link

@coderabbitai coderabbitai bot Jan 3, 2024 •
edited
Loading

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.

Suggested change
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();
}
}

Copy link
Member

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.

Copy link

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.

}

/**
* Uses {@code FALLBACK_LOCATIONS} for directories as fallback, if {@code ~/Library/CloudStorage/} isn't present.
* Returns the corresponding presets.
*
* @return {@code Stream<LocationPreset>}. Displays as "{@code Google Drive}"
*/
private Stream<LocationPreset> getFallbackLocation() {
return FALLBACK_LOCATIONS.stream() //
.filter(Files::isDirectory) //
.map(location -> new LocationPreset("Google Drive", location))
.findFirst()
.stream();
JaniruTEC marked this conversation as resolved.
Show resolved Hide resolved
}
}
5 changes: 1 addition & 4 deletions ...s/GoogleDriveLocationPresetsProvider.java → ...eDriveWindowsLocationPresetsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,11 @@
import java.util.List;
import java.util.stream.Stream;

import static org.cryptomator.integrations.common.OperatingSystem.Value.MAC;
import static org.cryptomator.integrations.common.OperatingSystem.Value.WINDOWS;

@OperatingSystem(WINDOWS)
@OperatingSystem(MAC)
@CheckAvailability
public final class GoogleDriveLocationPresetsProvider implements LocationPresetsProvider {
public final class GoogleDriveWindowsLocationPresetsProvider implements LocationPresetsProvider {

private static final List<Path> LOCATIONS = Arrays.asList( //
LocationPresetsProvider.resolveLocation("~/GoogleDrive/My Drive"), //
Expand All @@ -37,5 +35,4 @@ public Stream<LocationPreset> getLocations() {
.findFirst() //
.stream();
}

}