Removed swing from default file dir detection #9222
Conversation
tobiasdiez
added
the
status: ready-for-review
|
AS per @koppor said, the average user does look in his documents for files and not where the bib file is located (that might be the downloads files). |
Siedlerchr
removed
the
status: ready-for-review
|
Is there a test for:
This is important for shared .bib files where users use Mac OS X and the path is configured for Windows. Moreover, storing relative to the bib file is default, so I don't get why there is a change needed in the logic here
|
| private final List<Path> fileDirForDatabase; | ||
|
|
||
| public FileLinkPreferences(String mainFileDirectory, List<Path> fileDirForDatabase) { | ||
| public FileLinkPreferences(Optional<Path> mainFileDirectory, List<Path> fileDirForDatabase) { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
This is mostly to support new FileLinkPreferences(getFilePreferences().getFileDirectory(),...); where getFileDirectory returns an optional.
There was a problem hiding this comment.
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()"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
next quick thought without deeper thinking 😅 : why is the mainFileDirectory not just the first Value of fileDirForDatabase?
There was a problem hiding this comment.
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.
|
I can totally relate on one hand to @tobiasdiez idea of getting rid of swing to reduce overhead and I remember we had this conversation before. On the other hand we still use swing for all the undo/redo stuff, so sadly there is no short term advantage here and as i understand the default behaviour if jabref is already in favor of tobias intuition. |
|
As far as I remember, the Undo/Redo stuff doesn't start and require the awt toolkit. It's essentially a data holder. This is similar to the JavaFX collections that also can be used without the JavaFX toolkit. I don't think this special case is so important that we pull in the awt toolkit, nor go down the JNI rabbit hole. But I'm happy to use another easy workaround... |
|
Current behavior: See org.jabref.model.database.BibDatabaseContext#getFileDirectories // 1. Metadata user-specific directory
metaData.getUserFileDirectory(preferences.getUser())
.ifPresent(userFileDirectory -> fileDirs.add(getFileDirectoryPath(userFileDirectory)));
// 2. Metadata general directory
metaData.getDefaultFileDirectory()
.ifPresent(metaDataDirectory -> fileDirs.add(getFileDirectoryPath(metaDataDirectory)));
// 3. BIB file directory or Main file directory
// fileDirs.isEmpty in the case, 1) no user-specific file directory and 2) no general file directory is set
// (in the metadata of the bib file)
if (fileDirs.isEmpty() && preferences.shouldStoreFilesRelativeToBibFile()) {
...
} else {
// Main file directory
preferences.getFileDirectory().ifPresent(fileDirs::add);
}Thus, the behavior @tobiasdiez encounters happens in the follwing
Special case in step 3: In case no meta data directory is set AND files should NOT be stored related to bib file, then the main file directory is used. That means, the encountered behavior can only occur, if a meta directory is set AND none of these directories exist (because of step 4). This was the intended "hack" of #9113. @tobias, please explain how the following issue arises at your side:
The current implementation seems to follow this behavior (see above). |
|
We created ADR-0026 at #9235. We close this PR until we find the root cause of the issue at @tobiasdiez's machine. In parallel, we are thinking of contributing to AppDirs. |
|
Just try to start the JabRef cli from #9217 in a terminal (without any window server running) and you get: While we might want to argue that this use case (running JabRef without a window server) is not the most relevant, the error log shows however that the full awt toolkit is initialized by the call to the default file chooser: |
tobiasdiez
added
the
status: ready-for-review
tobiasdiez
changed the title
|
I experimented a bit. How about this bad boy: It seems there is no additional dependency needed, at least my ide is not complaining. |
|
@calixtus I am not sure about the access to the internal of com sun modules. Maybe needs some flags in for modules |
|
JNA is not an internal api, but publicly well documented and widely used. |
|
|
|
This would have to be added to the module-info.java: |
|
Try if this doesn't break jpackage |
|
Nice suggestion @calixtus. I think its best to submit this addition as a PR to AppDirs, which already takes care of the special handling of the different OS's. Since this is a blocker for a 5.8 release in my opinion, should we first merge this PR here and once AppDirs provides the necessary functionality we can re-introduce the default file dir? |
|
I will suggest it to AppDirs, but I think it may be a bit out of focus for them, since the name if the package and it's whole architecture seems to me clearly designed to hide every other KnownFolder from public access in this package. I'd rather opt for implementing a similar approach in our OS or sthg like package. |
* upstream/main: Make autosave tick in shared database opening dialog active (#9258) Bump controlsfx from 11.1.1 to 11.1.2 (#9261) Bump actions/configure-pages from 1 to 2 (#9262) Bump hmarr/auto-approve-action from 2.4.0 to 3.0.0 (#9259) Bump gittools/actions from 0.9.13 to 0.9.14 (#9260) Fix for resizing cleanup entries dialog (#9240) Refresh example styles Squashed 'buildres/csl/csl-locales/' changes from cb98d36691..e243665390 Squashed 'buildres/csl/csl-styles/' changes from 7bde3e4..4eee79a Fix casing Update contributing.md Remove obsolete Jekyll howto Updates Just-the-Docs from 0.3.3 to 0.4.0.rc3 (#9249) Place subgroups w.r.t. alphabetical ordering (#9228) DOAB Fetcher now fetches ISBN and imprint fields where possible (#9229) ISSUE-9145: implement isbn fetcher (#9205) Remove explicit javafx jmod stuff (#9245) New Crowdin updates (#9239)
This reverts commit 3e640f5. # Conflicts: # CHANGELOG.md # src/main/java/org/jabref/preferences/FilePreferences.java # src/main/java/org/jabref/preferences/JabRefPreferences.java # src/test/java/org/jabref/model/database/BibDatabaseContextTest.java
…_file_dir * upstream/default_file_dir: Update src/main/java/org/jabref/gui/desktop/os/Windows.java
|
This PR is now ready. I refactored the Optional stuff again, and Carl implemented the JNI access. |
|
This is odd, because swing is now back in the package |
| @@ -2201,7 +2188,9 @@ public FilePreferences getFilePreferences() { | |||
|
|
|||
| filePreferences = new FilePreferences( | |||
| getInternalPreferences().getUser(), | |||
| determineMainFileDirectory(get(MAIN_FILE_DIRECTORY)), | |||
| StringUtil.isNotBlank(get(MAIN_FILE_DIRECTORY)) | |||
There was a problem hiding this comment.
Refactor to
| StringUtil.isNotBlank(get(MAIN_FILE_DIRECTORY)) | |
| getPath(MAIN_FILE_DIRECTORY).orElse(JabRefDesktop.getNativeDesktop().getDefaultFileChooserDirectory()) |
There was a problem hiding this comment.
There's so such nethod. And I don't see the benefit of wrapping it an optional here.
There was a problem hiding this comment.
StringUtil.isNotBlank(...) is - I believe - the equivalent to `... != null && !....isEmpty()", so should simplify determineMainFileDirectory
There was a problem hiding this comment.
On the other hand, method get is called twice. Maybe we can extract the var from the constructor call
There was a problem hiding this comment.
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
}
|
tests are failing since |
* upstream/main: Removed swing from default file dir detection (#9222) # Conflicts: # src/main/java/org/jabref/gui/desktop/os/Linux.java
* upstream/main: Removed swing from default file dir detection (#9222) Make autosave tick in shared database opening dialog active (#9258) Bump controlsfx from 11.1.1 to 11.1.2 (#9261) Bump actions/configure-pages from 1 to 2 (#9262) Bump hmarr/auto-approve-action from 2.4.0 to 3.0.0 (#9259) Bump gittools/actions from 0.9.13 to 0.9.14 (#9260)
* Save files again relative to bib file * Reworded * Reintroduced default file chooser directory without swing * Update CHANGELOG.md * Update CHANGELOG.md * Update NativeDesktop.java * Reworked ADR * Added mac directory * Removed comments * Added linux support * Revert "Save files again relative to bib file" This reverts commit 3e640f5. # Conflicts: # CHANGELOG.md # src/main/java/org/jabref/preferences/FilePreferences.java # src/main/java/org/jabref/preferences/JabRefPreferences.java # src/test/java/org/jabref/model/database/BibDatabaseContextTest.java * Update src/main/java/org/jabref/gui/desktop/os/Windows.java * fix merge errors * fix wrong method call * remove defautl fix checkstyle * reintroduce method to avoid calling get two times * Introduced getPath method * Update src/main/java/org/jabref/gui/desktop/os/Linux.java fix env var Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
* upstream/main: (28 commits) Minor code improvements in library properties dialog (#9265) Adjust and make tests for BibTex/Biblatex/CSL mapping more accurate by adding Apa 7th edition (#9255) Removed swing from default file dir detection (#9222) Make autosave tick in shared database opening dialog active (#9258) Bump controlsfx from 11.1.1 to 11.1.2 (#9261) Bump actions/configure-pages from 1 to 2 (#9262) Bump hmarr/auto-approve-action from 2.4.0 to 3.0.0 (#9259) Bump gittools/actions from 0.9.13 to 0.9.14 (#9260) Fix for resizing cleanup entries dialog (#9240) Refresh example styles Squashed 'buildres/csl/csl-locales/' changes from cb98d36691..e243665390 Squashed 'buildres/csl/csl-styles/' changes from 7bde3e4..4eee79a Fix casing Update contributing.md Remove obsolete Jekyll howto Updates Just-the-Docs from 0.3.3 to 0.4.0.rc3 (#9249) Place subgroups w.r.t. alphabetical ordering (#9228) DOAB Fetcher now fetches ISBN and imprint fields where possible (#9229) ISSUE-9145: implement isbn fetcher (#9205) Remove explicit javafx jmod stuff (#9245) ...
* upstream/main: [WIP] Add GitHub action: Greetings.yml - Automates advice to JabRefs first time code contributors (JabRef#9272) Minor code improvements in library properties dialog (JabRef#9265) Adjust and make tests for BibTex/Biblatex/CSL mapping more accurate by adding Apa 7th edition (JabRef#9255) Removed swing from default file dir detection (JabRef#9222) Make autosave tick in shared database opening dialog active (JabRef#9258) Bump controlsfx from 11.1.1 to 11.1.2 (JabRef#9261) Bump actions/configure-pages from 1 to 2 (JabRef#9262) Bump hmarr/auto-approve-action from 2.4.0 to 3.0.0 (JabRef#9259) Bump gittools/actions from 0.9.13 to 0.9.14 (JabRef#9260) Fix for resizing cleanup entries dialog (JabRef#9240) Refresh example styles Squashed 'buildres/csl/csl-locales/' changes from cb98d36691..e243665390 Squashed 'buildres/csl/csl-styles/' changes from 7bde3e4..4eee79a Fix casing Update contributing.md Remove obsolete Jekyll howto Updates Just-the-Docs from 0.3.3 to 0.4.0.rc3 (JabRef#9249) Place subgroups w.r.t. alphabetical ordering (JabRef#9228) DOAB Fetcher now fetches ISBN and imprint fields where possible (JabRef#9229)
We revert parts of #9113 and remove the default main file dir again since this loads the full awt toolkit which has a large memory and cpu overhead:
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)