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.

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

Removed swing from default file dir detection #9222

Merged
merged 23 commits into from
Oct 17, 2022
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3e640f5
Save files again relative to bib file
tobiasdiez Oct 7, 2022
7ab692b
Merge branch 'main' into default_file_dir
tobiasdiez Oct 10, 2022
e50a684
Merge remote-tracking branch 'upstream/main' into default_file_dir
calixtus Oct 12, 2022
de27af5
Reworded
calixtus Oct 12, 2022
0749128
Reintroduced default file chooser directory without swing
calixtus Oct 12, 2022
7dc926e
Update CHANGELOG.md
calixtus Oct 12, 2022
6cb05d3
Update CHANGELOG.md
calixtus Oct 12, 2022
dfd6411
Update NativeDesktop.java
calixtus Oct 12, 2022
28624ff
Reworked ADR
calixtus Oct 12, 2022
3ea936d
Merge remote-tracking branch 'upstream/default_file_dir' into default…
calixtus Oct 12, 2022
1026011
Added mac directory
calixtus Oct 12, 2022
b95b021
Removed comments
calixtus Oct 12, 2022
eab433c
Added linux support
calixtus Oct 12, 2022
c2597e7
Merge remote-tracking branch 'upstream/main' into default_file_dir
Siedlerchr Oct 17, 2022
c90c543
Revert "Save files again relative to bib file"
Siedlerchr Oct 17, 2022
6b28d08
Update src/main/java/org/jabref/gui/desktop/os/Windows.java
Siedlerchr Oct 17, 2022
1082344
fix merge errors
Siedlerchr Oct 17, 2022
fc4eccc
Merge remote-tracking branch 'upstream/default_file_dir' into default…
Siedlerchr Oct 17, 2022
a51570d
fix wrong method call
Siedlerchr Oct 17, 2022
5dd4bd8
remove defautl
Siedlerchr Oct 17, 2022
aa7e7a9
reintroduce method to avoid calling get two times
Siedlerchr Oct 17, 2022
44aa9dc
Introduced getPath method
calixtus Oct 17, 2022
486b8b2
Update src/main/java/org/jabref/gui/desktop/os/Linux.java
Siedlerchr Oct 17, 2022
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- A user can now add arbitrary data into `study.yml`. JabRef just ignores this data. [#9124](https://github.com/JabRef/jabref/pull/9124)
- We reworked the External Changes Resolver dialog. [#9021](https://github.com/JabRef/jabref/pull/9021)
- The fallback directory of the file folder now is the general file directory. In case there was a directory configured for a library and this directory was not found, JabRef placed the PDF next to the .bib file and not into the general file directory.
- The global default directory for storing PDFs is now the subdirectory "JabRef" in the user's home.
- The global default directory for storing PDFs is now the documents folder in the user's home.
- We reworked the Define study parameters dialog. [#9123](https://github.com/JabRef/jabref/pull/9123)
- We simplified the actions to fast-resolve duplicates to 'Keep Left', 'Keep Right', 'Keep Both' and 'Keep Merged'. [#9056](https://github.com/JabRef/jabref/issues/9056)
- We fixed an issue where a message about changed metadata would occur on saving although nothing changed. [#9159](https://github.com/JabRef/jabref/issues/9159)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
nav_order: 26
parent: Decision Records
---
# Use Swing's FileChooser to Determine Default Directory
# Use Java Native Access to Determine Default Directory

## Context and Problem Statement

Expand All @@ -20,11 +20,11 @@ How to determine the "best" directory native for the OS the user runs.
* Use Swing's FileChooser to Determine Default Directory
* Use `user.home`
* [AppDirs](https://github.com/harawata/appdirs)
* [Java Native Access](https://github.com/java-native-access/jna)

## Decision Outcome

Chosen option: "Use Swing's FileChooser to Determine Default Directory", because
comes out best (see below).
Chosen option: "Java Native Access", because comes out best (see below).

## Pros and Cons of the Options

Expand All @@ -51,7 +51,13 @@ There is `System.getProperty("user.home");`.
> AppDirs is a small java library which provides a path to the platform dependent special folder/directory.

* Good, because already used in JabRef
* Bad, because does not use "MyDocuments" on Windows, but rather `C:\Users\<Account>\AppData\<AppAuthor>\<AppName>` as basis
* Bad, because does not use `Documents` on Windows, but rather `C:\Users\<Account>\AppData\<AppAuthor>\<AppName>` as basis

### Java Native Access

* Good, because no additional dependency required, as it is already loaded by AppDirs
* Good, because it is well maintained and widely used
* Good, because it provides direct access to `Documents` and other system variables

## More Information

Expand Down
1 change: 1 addition & 0 deletions src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
requires com.fasterxml.jackson.dataformat.yaml;
requires com.fasterxml.jackson.datatype.jsr310;
requires net.harawata.appdirs;
requires com.sun.jna.platform;

requires org.eclipse.jgit;
uses org.eclipse.jgit.transport.SshSessionFactory;
Expand Down
19 changes: 1 addition & 18 deletions src/main/java/org/jabref/gui/desktop/JabRefDesktop.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
import java.util.Optional;
import java.util.regex.Pattern;

import javax.swing.filechooser.FileSystemView;

import org.jabref.architecture.AllowedToUseSwing;
import org.jabref.gui.DialogService;
import org.jabref.gui.Globals;
import org.jabref.gui.desktop.os.DefaultDesktop;
Expand Down Expand Up @@ -40,7 +37,6 @@
* TODO: Replace by http://docs.oracle.com/javase/7/docs/api/java/awt/Desktop.html
* http://stackoverflow.com/questions/18004150/desktop-api-is-not-supported-on-the-current-platform
*/
@AllowedToUseSwing("Needs access to swing for the user's os dependent file chooser path")
public class JabRefDesktop {

private static final Logger LOGGER = LoggerFactory.getLogger(JabRefDesktop.class);
Expand Down Expand Up @@ -292,20 +288,7 @@ public static void openConsole(File file, PreferencesService preferencesService,
}
}

/**
* Get the user's default file chooser directory
*
* @return The path to the directory
*/
public static String getDefaultFileChooserDirectory() {
// Implementation: ADR-0026

// We do not return a subdirectory "JabRef", because
// - the directory might not exist at this point of the method
// - we might not have the rights to create a directory
// - getters should not have any side effect
return FileSystemView.getFileSystemView().getDefaultDirectory().getPath();
}


// TODO: Move to OS.java
public static NativeDesktop getNativeDesktop() {
Expand Down
11 changes: 10 additions & 1 deletion src/main/java/org/jabref/gui/desktop/os/Linux.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Locale;
import java.util.Objects;
import java.util.Optional;

import org.jabref.architecture.AllowedToUseAwt;
Expand Down Expand Up @@ -134,7 +135,7 @@ public void openConsole(String absolutePath, DialogService dialogService) throws
if (emulatorName != null) {
emulatorName = emulatorName.substring(emulatorName.lastIndexOf(File.separator) + 1);

String[] cmd = {};
String[] cmd;
if (emulatorName.contains("gnome")) {
cmd = new String[] {"gnome-terminal", "--working-directory=", absolutePath};
} else if (emulatorName.contains("xfce4")) {
Expand Down Expand Up @@ -167,4 +168,12 @@ public String detectProgramPath(String programName, String directoryName) {
public Path getApplicationDirectory() {
return Path.of("/usr/lib/");
}

@Override
public Path getDefaultFileChooserDirectory() {
return Path.of(Objects.requireNonNullElse(
System.getenv("XDG_DOCUMENTS_DIR"),
System.getProperty("users.home") + "/Documents")
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
14 changes: 13 additions & 1 deletion src/main/java/org/jabref/gui/desktop/os/NativeDesktop.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ public interface NativeDesktop {
*
* @param filePath The filename.
* @param application Link to the app that opens the file.
* @throws IOException
*/
void openFileWithApplication(String filePath, String application) throws IOException;

Expand All @@ -31,6 +30,19 @@ public interface NativeDesktop {
*/
Path getApplicationDirectory();

/**
* Get the user's default file chooser directory
*
* @return The path to the directory
*/
default Path getDefaultFileChooserDirectory() {
// We do not return a subdirectory "JabRef", because
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
// - the directory might not exist at this point of the method
// - we might not have the rights to create a directory
// - getters should not have any side effect
return Path.of(System.getProperty("user.home"));
}

/**
* Returns the path to the system's user directory.
*
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/org/jabref/gui/desktop/os/OSX.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,9 @@ public String detectProgramPath(String programName, String directoryName) {
public Path getApplicationDirectory() {
return Path.of("/Applications");
}

@Override
public Path getDefaultFileChooserDirectory() {
return Path.of(System.getProperty("user.home") + "/Documents");
}
}
24 changes: 24 additions & 0 deletions src/main/java/org/jabref/gui/desktop/os/Windows.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,16 @@
import org.jabref.gui.externalfiletype.ExternalFileType;
import org.jabref.gui.externalfiletype.ExternalFileTypes;

import com.sun.jna.platform.win32.KnownFolders;
import com.sun.jna.platform.win32.Shell32Util;
import com.sun.jna.platform.win32.ShlObj;
import com.sun.jna.platform.win32.Win32Exception;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Windows implements NativeDesktop {
private static final Logger LOGGER = LoggerFactory.getLogger(Windows.class);

private static final String DEFAULT_EXECUTABLE_EXTENSION = ".exe";

@Override
Expand Down Expand Up @@ -70,6 +79,21 @@ public Path getApplicationDirectory() {
return getUserDirectory();
}

@Override
public Path getDefaultFileChooserDirectory() {
try {
try {
return Path.of(Shell32Util.getKnownFolderPath(KnownFolders.FOLDERID_Documents));
} catch (UnsatisfiedLinkError e) {
// Windows Vista or earlier
return Path.of(Shell32Util.getFolderPath(ShlObj.CSIDL_MYDOCUMENTS));
}
} catch (Win32Exception e) {
LOGGER.error("Error accessing folder", e);
return Path.of(System.getProperty("user.home"));
}
}

@Override
public void openFileWithApplication(String filePath, String application) throws IOException {
new ProcessBuilder(Path.of(application).toString(), Path.of(filePath).toString()).start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public LinkedFilesTabViewModel(DialogService dialogService, PreferencesService p
@Override
public void setValues() {
// External files preferences / Attached files preferences / File preferences
mainFileDirectoryProperty.setValue(filePreferences.getFileDirectory().orElse(Path.of("")).toString());
mainFileDirectoryProperty.setValue(filePreferences.getMainFileDirectory().orElse(Path.of("")).toString());
useMainFileDirectoryProperty.setValue(!filePreferences.shouldStoreFilesRelativeToBibFile());
useBibLocationAsPrimaryProperty.setValue(filePreferences.shouldStoreFilesRelativeToBibFile());
fulltextIndex.setValue(filePreferences.shouldFulltextIndexLinkedFiles());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public List<Path> getFileDirectories(FilePreferences preferences) {
});
} else {
// Main file directory
preferences.getFileDirectory().ifPresent(fileDirs::add);
preferences.getMainFileDirectory().ifPresent(fileDirs::add);
}

return fileDirs.stream().map(Path::toAbsolutePath).collect(Collectors.toList());
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/preferences/FilePreferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public String getUser() {
return user.getValue();
}

public Optional<Path> getFileDirectory() {
public Optional<Path> getMainFileDirectory() {
if (StringUtil.isBlank(mainFileDirectory.getValue())) {
return Optional.empty();
} else {
Expand Down
17 changes: 3 additions & 14 deletions src/main/java/org/jabref/preferences/JabRefPreferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -2179,19 +2179,6 @@ public FieldWriterPreferences getFieldWriterPreferences() {
getFieldContentParserPreferences());
}

/**
* Ensures that the main file directory is a non-empty String.
* The directory is <emph>NOT</emph> created, because creation of the directory is the task of the respective methods.
*
* @param originalDirectory the directory as configured
*/
private String determineMainFileDirectory(String originalDirectory) {
if ((originalDirectory != null) && !originalDirectory.isEmpty()) {
// A non-empty directory is kept
return originalDirectory;
}
return JabRefDesktop.getDefaultFileChooserDirectory();
}

@Override
public FilePreferences getFilePreferences() {
Expand All @@ -2201,7 +2188,9 @@ public FilePreferences getFilePreferences() {

filePreferences = new FilePreferences(
getInternalPreferences().getUser(),
determineMainFileDirectory(get(MAIN_FILE_DIRECTORY)),
StringUtil.isNotBlank(get(MAIN_FILE_DIRECTORY))
Copy link
Member Author

Choose a reason for hiding this comment

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

Refactor to

Suggested change
StringUtil.isNotBlank(get(MAIN_FILE_DIRECTORY))
getPath(MAIN_FILE_DIRECTORY).orElse(JabRefDesktop.getNativeDesktop().getDefaultFileChooserDirectory())

Copy link
Member

Choose a reason for hiding this comment

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

There's so such nethod. And I don't see the benefit of wrapping it an optional here.

Copy link
Member

Choose a reason for hiding this comment

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

StringUtil.isNotBlank(...) is - I believe - the equivalent to `... != null && !....isEmpty()", so should simplify determineMainFileDirectory

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, method get is called twice. Maybe we can extract the var from the constructor call

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we store quite a few paths in the preferences, it makes sense to introduce a helper for it in my opinion. You can also introduce a second default arg:

getPath(string name, Path defaultVal) {
   val = get(name)
   return StringUtil.isNotBlank(val) ? Path.of(val) : defaultVal
}

Copy link
Member

Choose a reason for hiding this comment

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

I've reintroduced a method

? get(MAIN_FILE_DIRECTORY)
: JabRefDesktop.getNativeDesktop().getDefaultFileChooserDirectory().toString(),
getBoolean(STORE_RELATIVE_TO_BIB),
get(IMPORT_FILENAMEPATTERN),
get(IMPORT_FILEDIRPATTERN),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.jabref.logic.pdf.search.indexing;

import java.io.IOException;
import java.nio.file.Path;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -35,12 +34,12 @@ public void setup() {
when(databaseContext.getFileDirectories(Mockito.any())).thenReturn(Collections.singletonList(Path.of("src/test/resources/pdfs")));
this.filePreferences = mock(FilePreferences.class);
when(filePreferences.getUser()).thenReturn("test");
when(filePreferences.getFileDirectory()).thenReturn(Optional.empty());
when(filePreferences.getMainFileDirectory()).thenReturn(Optional.empty());
when(filePreferences.shouldStoreFilesRelativeToBibFile()).thenReturn(true);
}

@Test
public void unknownFileTestShouldReturnEmptyList() throws IOException {
public void unknownFileTestShouldReturnEmptyList() {
// given
BibEntry entry = new BibEntry();
entry.setFiles(Collections.singletonList(new LinkedFile("Wrong path", "NOT_PRESENT.pdf", "Type")));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,9 @@ void getFileDirectoriesWithMetadata() {
@Test
void getUserFileDirectoryIfAllAreEmpty() {
when(fileDirPrefs.shouldStoreFilesRelativeToBibFile()).thenReturn(false);
Path userDirJabRef = JabRefDesktop.getNativeDesktop().getDefaultFileChooserDirectory();

Path userDirJabRef = Path.of(JabRefDesktop.getDefaultFileChooserDirectory(), "JabRef");

when(fileDirPrefs.getFileDirectory()).thenReturn(Optional.of(userDirJabRef));
when(fileDirPrefs.getMainFileDirectory()).thenReturn(Optional.of(userDirJabRef));
BibDatabaseContext database = new BibDatabaseContext();
database.setDatabasePath(Path.of("biblio.bib"));
assertEquals(Collections.singletonList(userDirJabRef), database.getFileDirectories(fileDirPrefs));
Expand Down