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 1 commit
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: 0 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We removed "last-search-date" from the SLR feature, because the last-search-date can be deducted from the git logs. [#9116](https://github.com/JabRef/jabref/pull/9116)
- 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.
- 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
10 changes: 0 additions & 10 deletions src/main/java/org/jabref/architecture/AllowedToUseSwing.java

This file was deleted.

19 changes: 0 additions & 19 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,21 +288,6 @@ 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() {
// Property "user.home" might be "difficult" on Windows
// See https://stackoverflow.com/a/586917/873282 for a longer discussion
// The proposed solution is to use Swing's FileSystemView
// See https://stackoverflow.com/a/32914568/873282
// As of 2022, System.getProperty("user.home") returns c:\Users\USERNAME on Windows 10, whereas
// the FileSystemView returns C:\Users\USERNAME\Documents, which is the "better" directory
return FileSystemView.getFileSystemView().getDefaultDirectory().getPath();
}

// TODO: Move to OS.java
public static NativeDesktop getNativeDesktop() {
if (OS.WINDOWS) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/logic/layout/format/FileLink.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public String format(String field) {
// ugly hack, the export routine has set a global variable before
// starting the export, which contains the database's file directory:
if (prefs.getFileDirForDatabase() == null) {
dirs = Collections.singletonList(Path.of(prefs.getMainFileDirectory()));
dirs = prefs.getMainFileDirectory().map(List::of).orElse(Collections.emptyList());
} else {
dirs = prefs.getFileDirForDatabase();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@

import java.nio.file.Path;
import java.util.List;
import java.util.Optional;

public class FileLinkPreferences {

private final String mainFileDirectory;
private final Optional<Path> mainFileDirectory;
private final List<Path> fileDirForDatabase;

public FileLinkPreferences(String mainFileDirectory, List<Path> fileDirForDatabase) {
public FileLinkPreferences(Optional<Path> mainFileDirectory, List<Path> fileDirForDatabase) {
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like the optional as a method argument here. Does not seem to provide any advantage of just returning Optional.offNullable in getMainFileDirecty but bad readability, and construction of a superfluous optional object...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mostly to support new FileLinkPreferences(getFilePreferences().getFileDirectory(),...); where getFileDirectory returns an optional.

Copy link
Member

Choose a reason for hiding this comment

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

Just thinking loud:

  • Should mainFileDirectory ever be empty? Do we have a default value for this -> .orElse()? Reason: Have guaranteed non-null objects to avoid later checking for nulls or empty.
  • Maybe overload the constructor to avoid "Optional.empty()"

Copy link
Member Author

Choose a reason for hiding this comment

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

Olly's idea was to add a default for mainFileDirectory. But in case we don't find a nice solution for getting the user doc folder, the idea of this PR is to make it clear that the main file directory might not be present.

Copy link
Member

Choose a reason for hiding this comment

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

next quick thought without deeper thinking 😅 : why is the mainFileDirectory not just the first Value of fileDirForDatabase?

Copy link
Member

Choose a reason for hiding this comment

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

Olly's idea was to add a default for mainFileDirectory.

Yes!

But in case we don't find a nice solution for getting the user doc folder,

AWT is the currently best solution - AWT is still bundled with Java.

this.mainFileDirectory = mainFileDirectory;
this.fileDirForDatabase = fileDirForDatabase;
}

public String getMainFileDirectory() {
public Optional<Path> getMainFileDirectory() {
return mainFileDirectory;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public String format(String field) {
// ugly hack, the export routine has set a global variable before
// starting the export, which contains the database's file directory:
if ((prefs.getFileDirForDatabase() == null) || prefs.getFileDirForDatabase().isEmpty()) {
dirs = Collections.singletonList(Path.of(prefs.getMainFileDirectory()));
dirs = prefs.getMainFileDirectory().map(List::of).orElse(Collections.emptyList());
} else {
dirs = prefs.getFileDirForDatabase();
}
Expand Down
19 changes: 10 additions & 9 deletions src/main/java/org/jabref/preferences/FilePreferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@
import javafx.collections.ObservableSet;

import org.jabref.gui.externalfiletype.ExternalFileType;
import org.jabref.gui.util.OptionalObjectProperty;
import org.jabref.model.strings.StringUtil;

public class FilePreferences {

public static final String[] DEFAULT_FILENAME_PATTERNS = new String[] {"[bibtexkey]", "[bibtexkey] - [title]"};

private final StringProperty user = new SimpleStringProperty();
private final SimpleStringProperty mainFileDirectory = new SimpleStringProperty();
private final OptionalObjectProperty<Path> mainFileDirectory = OptionalObjectProperty.empty();
private final BooleanProperty storeFilesRelativeToBibFile = new SimpleBooleanProperty();
private final StringProperty fileNamePattern = new SimpleStringProperty();
private final StringProperty fileDirectoryPattern = new SimpleStringProperty();
Expand All @@ -42,7 +43,7 @@ public FilePreferences(String user,
Path workingDirectory,
Set<ExternalFileType> externalFileTypes) {
this.user.setValue(user);
this.mainFileDirectory.setValue(mainFileDirectory);
this.setMainFileDirectory(mainFileDirectory);
this.storeFilesRelativeToBibFile.setValue(storeFilesRelativeToBibFile);
this.fileNamePattern.setValue(fileNamePattern);
this.fileDirectoryPattern.setValue(fileDirectoryPattern);
Expand All @@ -57,19 +58,19 @@ public String getUser() {
}

public Optional<Path> getFileDirectory() {
if (StringUtil.isBlank(mainFileDirectory.getValue())) {
return Optional.empty();
} else {
return Optional.of(Path.of(mainFileDirectory.getValue()));
}
return mainFileDirectory.getValue();
}

public StringProperty mainFileDirectoryProperty() {
public OptionalObjectProperty<Path> mainFileDirectoryProperty() {
return mainFileDirectory;
}

public void setMainFileDirectory(String mainFileDirectory) {
this.mainFileDirectory.set(mainFileDirectory);
if (StringUtil.isBlank(mainFileDirectory)) {
this.mainFileDirectory.setValue(Optional.empty());
} else {
this.mainFileDirectory.setValue(Optional.of(Path.of(mainFileDirectory)));
}
}

public boolean shouldStoreFilesRelativeToBibFile() {
Expand Down
20 changes: 3 additions & 17 deletions src/main/java/org/jabref/preferences/JabRefPreferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ public void importPreferences(Path file) throws JabRefException {
@Override
public FileLinkPreferences getFileLinkPreferences() {
return new FileLinkPreferences(
getFilePreferences().mainFileDirectoryProperty().get(),
getFilePreferences().getFileDirectory(),
fileDirForDatabase);
}

Expand Down Expand Up @@ -2179,20 +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() {
if (Objects.nonNull(filePreferences)) {
Expand All @@ -2201,7 +2187,7 @@ public FilePreferences getFilePreferences() {

filePreferences = new FilePreferences(
getInternalPreferences().getUser(),
determineMainFileDirectory(get(MAIN_FILE_DIRECTORY)),
get(MAIN_FILE_DIRECTORY),
getBoolean(STORE_RELATIVE_TO_BIB),
get(IMPORT_FILENAMEPATTERN),
get(IMPORT_FILEDIRPATTERN),
Expand All @@ -2211,7 +2197,7 @@ public FilePreferences getFilePreferences() {
ExternalFileTypes.fromString(get(EXTERNAL_FILE_TYPES))
);

EasyBind.listen(filePreferences.mainFileDirectoryProperty(), (obs, oldValue, newValue) -> put(MAIN_FILE_DIRECTORY, newValue));
EasyBind.listen(filePreferences.mainFileDirectoryProperty(), (obs, oldValue, newValue) -> put(MAIN_FILE_DIRECTORY, newValue.map(Path::toString).orElse("")));
EasyBind.listen(filePreferences.storeFilesRelativeToBibFileProperty(), (obs, oldValue, newValue) -> putBoolean(STORE_RELATIVE_TO_BIB, newValue));
EasyBind.listen(filePreferences.fileNamePatternProperty(), (obs, oldValue, newValue) -> put(IMPORT_FILENAMEPATTERN, newValue));
EasyBind.listen(filePreferences.fileDirectoryPatternProperty(), (obs, oldValue, newValue) -> put(IMPORT_FILEDIRPATTERN, newValue));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ public static void doNotUseApacheCommonsLang3(JavaClasses classes) {
@ArchTest
public static void doNotUseSwing(JavaClasses classes) {
// This checks for all Swing packages, but not the UndoManager
noClasses().that().areNotAnnotatedWith(AllowedToUseSwing.class)
.should().accessClassesThat()
noClasses().should().accessClassesThat()
.resideInAnyPackage("javax.swing",
"javax.swing.border..",
"javax.swing.colorchooser..",
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/org/jabref/logic/layout/LayoutTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.io.StringReader;
import java.nio.file.Path;
import java.util.Collections;
import java.util.Optional;

import org.jabref.logic.layout.format.FileLinkPreferences;
import org.jabref.logic.layout.format.NameFormatterPreferences;
Expand Down Expand Up @@ -135,7 +136,7 @@ void beginConditionals() throws IOException {
@Test
void wrapFileLinksExpandFile() throws IOException {
when(layoutFormatterPreferences.getFileLinkPreferences()).thenReturn(
new FileLinkPreferences("", Collections.singletonList(Path.of("src/test/resources/pdfs/"))));
new FileLinkPreferences(Optional.empty(), Collections.singletonList(Path.of("src/test/resources/pdfs/"))));
BibEntry entry = new BibEntry(StandardEntryType.Article);
entry.addFile(new LinkedFile("Test file", Path.of("encrypted.pdf"), "PDF"));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.Collections;
import java.util.Optional;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -17,7 +18,7 @@ class WrapFileLinksTest {

@BeforeEach
void setUp() {
FileLinkPreferences preferences = new FileLinkPreferences("", Collections.emptyList());
FileLinkPreferences preferences = new FileLinkPreferences(Optional.empty(), Collections.emptyList());
formatter = new WrapFileLinks(preferences);
}

Expand Down Expand Up @@ -97,7 +98,7 @@ void testEndingBracket() {

@Test
void testPath() throws IOException {
FileLinkPreferences preferences = new FileLinkPreferences("",
FileLinkPreferences preferences = new FileLinkPreferences(Optional.empty(),
Collections.singletonList(Path.of("src/test/resources/pdfs/")));
formatter = new WrapFileLinks(preferences);
formatter.setArgument("\\p");
Expand All @@ -107,7 +108,7 @@ void testPath() throws IOException {

@Test
void testPathFallBackToGeneratedDir() throws IOException {
FileLinkPreferences preferences = new FileLinkPreferences("src/test/resources/pdfs/",
FileLinkPreferences preferences = new FileLinkPreferences(Optional.of(Path.of("src/test/resources/pdfs/")),
Collections.emptyList());
formatter = new WrapFileLinks(preferences);
formatter.setArgument("\\p");
Expand All @@ -117,7 +118,7 @@ void testPathFallBackToGeneratedDir() throws IOException {

@Test
void testPathReturnsRelativePathIfNotFound() {
FileLinkPreferences preferences = new FileLinkPreferences("",
FileLinkPreferences preferences = new FileLinkPreferences(Optional.empty(),
Collections.singletonList(Path.of("src/test/resources/pdfs/")));
formatter = new WrapFileLinks(preferences);
formatter.setArgument("\\p");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import java.util.List;
import java.util.Optional;

import org.jabref.gui.desktop.JabRefDesktop;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.types.IEEETranEntryType;
import org.jabref.model.metadata.MetaData;
Expand Down Expand Up @@ -94,13 +93,12 @@ void getFileDirectoriesWithMetadata() {
@Test
void getUserFileDirectoryIfAllAreEmpty() {
when(fileDirPrefs.shouldStoreFilesRelativeToBibFile()).thenReturn(false);
Path userDirGlobal = Path.of("some random path").toAbsolutePath();
when(fileDirPrefs.getFileDirectory()).thenReturn(Optional.of(userDirGlobal));

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

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

@Test
Expand Down