Remove warnings when importing search entries in unsaved libraries #11111

subhramit · 2024-03-30T11:18:18Z

This is an attempt to make JabRef's handling of imports with an unsaved library more newcomer-friendly.

Description

Earlier, if there was an attempt to import entries via web search, with the source being Springer, Arxiv, etc. (any source that allows associated pdfs to be downloaded), we would get the following dialog if the library was not saved:

The problem was that:
(1) This dialog was not good UX (as pointed out by @koppor).
(2) This dialog would pop up multiple times - for example, if the user had selected 25 entries and clicked on import (without having saved the library), the dialog would come up and needed to be closed 25 times before returning to functionality. This was very inconvenient, especially for people who are new to JabRef.

bandicam.2024-03-30.19-03-12-106.mp4

Changes

The warning is no longer shown.

bandicam.2024-03-30.19-00-42-702.mp4

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
~~[] Tests created for changes (if applicable)~~
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

koppor · 2024-03-30T11:57:06Z

src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java

@@ -141,6 +142,11 @@ public void importEntries(List<BibEntry> entriesToImport, boolean shouldDownload
        // Remember the selection in the dialog
        preferences.getFilePreferences().setDownloadLinkedFiles(shouldDownloadFiles);

+        Optional<Path> targetDirectory = databaseContext.getFirstExistingFileDir(preferences.getFilePreferences());
+        if (targetDirectory.isEmpty()) {
+            dialogService.showErrorDialogAndWait(Localization.lang("Import entries"), Localization.lang("File directory needs to be set or does not exist.\nPlease save your library and check the preferences."));


While this is technically correct, the dialog is shown even if no PDFs will be downloaded at all.

Since the issue is an edge case, this fix is OK for me.

Yes, I did not want to make it specific to PDFs since saving a library is anyway a good practice that should be imbibed in users. Secondly, I thought why only specifically warn in case of pdfs? There too the warning could be completely removed, the entries anyway get imported, and the pdf is accessible by clicking the pdf icon. So I thought either don't warn at all (like other fetchers) or warn whenever importing entries (and change the dialog to "Import entries" instead of "Download file") to maintain uniformity.

It is only about the first save ever! Not subsequent saves.

The PDF is accessible by the URL, not by the file system.

The PDF is accessible by the URL, not by the file system.

Hmm...I see now. In that case there should be two different warning texts. Or I'll look later if I can restrict this only to PDFs.

No additional warning text.

One could make it even more user friendly if there was no warning text at all - and all fetchers just put the link in case there is no file directory.

Should be enough for this edge case. - And I keep saying edge case, because it only happens at new files before the first save ever. When working with existing files (e.g., Chocolate.bib or the example bib of a latex template) it does not happen.

Okay there is a simple way to do that - declaring a static boolean dialogShown and using it inside the execute() method of DownloadLinkedFileAction.java. Good thing - now only PDFs will trigger this. The tradeoff - now it will only be shown once for each session, and not if next time another set of entries are imported.
Which fix would you prefer?

One could make it even more user friendly if there was no warning text at all - and all fetchers just put the link in case there is no file directory.

Oh so it'd be better if we removed the warning itself? That too is doable. Let me see.
Edit: Done.

koppor · 2024-03-30T12:02:07Z

CHANGELOG.md

@@ -87,6 +87,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - We fixed an issue where the "Import by ID" function would throw an error when a DOI that contains URL-encoded characters was entered. [#10648](https://github.com/JabRef/jabref/issues/10648)
 - We fixed an issue with handling of an "overflow" of authors at `[authIniN]`. [#11087](https://github.com/JabRef/jabref/issues/11087)
 - We fixed an issue where an exception occurred when selecting entries in the web search results. [#11081](https://github.com/JabRef/jabref/issues/11081)
+- We fixed a bug where the "Directory not set" warning would come up multiple times when importing multiple entries without saving library, and would not come up at all for some fetchers if there was no pdf associated. [#11075](https://github.com/JabRef/jabref/issues/11075)


Uff, too long text

When a new library is unsaved, there is now a warning when fetching PDFs.

Changed 😅

But Oliver, wasn't the warning already there? The fix addresses the case of multiple warnings (in case of pdfs) and no warning (in case of no pdfs).

Then use "improved warning"

Not needed to describe the details since all of this is an edge case!

subhramit · 2024-03-30T13:35:01Z

PR description and code updated as per the case of best UX.

koppor

Good way forward, but I think there should still be a kind of information. LOGGER is good.

Maybe you need to adapt the language file, too for the removed message

koppor · 2024-03-30T13:40:10Z

src/main/java/org/jabref/gui/linkedfile/DownloadLinkedFileAction.java

@@ -100,7 +100,6 @@ public void execute() {

        Optional<Path> targetDirectory = databaseContext.getFirstExistingFileDir(filePreferences);
        if (targetDirectory.isEmpty()) {
-            dialogService.showErrorDialogAndWait(Localization.lang("Download file"), Localization.lang("File directory needs to be set or does not exist.\nPlease save your library and check the preferences."));


Add a LOGGER.warn here. Non- localized.

LOGGER.warn("File directory not available while downloading {}. Storing as URL in file field.", url)

subhramit · 2024-03-30T13:42:34Z

Maybe you need to adapt the language file, too for the removed message

Yes, already did that (check files changed).

…utlSaving * upstream/main: Bump com.puppycrawl.tools:checkstyle from 10.14.2 to 10.15.0 (#11121) Bump com.dlsc.gemsfx:gemsfx from 2.4.0 to 2.6.0 (#11120) improve error handling for bibdesk files (#11118) Update CSL styles (#11119) New translations jabref_en.properties (German) (#11116) New Crowdin updates (#11115) Remove obsolete sentence (#11114) add json file Remove warnings shown when importing search entries in unsaved libraries (#11111) New Crowdin updates (#11112)

subhramit added 2 commits March 30, 2024 16:02

Fix multiple and non uniform popup of directory not set warning

5a701e6

Changelog addition

93ac293

subhramit changed the title ~~Fix for issue 11075~~ Change the way warnings are shown when importing in unsaved libraries Mar 30, 2024

Changelog error fix

17edf6a

subhramit changed the title ~~Change the way warnings are shown when importing in unsaved libraries~~ Change the way warnings are shown when importing search entries in unsaved libraries Mar 30, 2024

koppor requested changes Mar 30, 2024

View reviewed changes

Shorten changelog line

888a86f

subhramit requested a review from koppor March 30, 2024 12:10

Siedlerchr previously approved these changes Mar 30, 2024

View reviewed changes

Changelog modification

a30c68c

subhramit dismissed Siedlerchr’s stale review via a30c68c March 30, 2024 12:28

subhramit requested a review from Siedlerchr March 30, 2024 12:29

UX-enhanced fix - no warnings

12316e0

subhramit changed the title ~~Change the way warnings are shown when importing search entries in unsaved libraries~~ Remove warnings shown when importing search entries in unsaved libraries Mar 30, 2024

koppor requested changes Mar 30, 2024

View reviewed changes

Log in case of no directory set while downloading

5efad2d

koppor approved these changes Mar 30, 2024

View reviewed changes

koppor enabled auto-merge March 30, 2024 13:47

koppor added this pull request to the merge queue Mar 30, 2024

Merged via the queue into JabRef:main with commit af6d14f Mar 30, 2024

subhramit changed the title ~~Remove warnings shown when importing search entries in unsaved libraries~~ Remove warnings when importing search entries in unsaved libraries Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove warnings when importing search entries in unsaved libraries #11111

Remove warnings when importing search entries in unsaved libraries #11111

subhramit commented Mar 30, 2024 •

edited

Loading

koppor Mar 30, 2024

subhramit Mar 30, 2024 •

edited

Loading

koppor Mar 30, 2024

koppor Mar 30, 2024

subhramit Mar 30, 2024

koppor Mar 30, 2024

subhramit Mar 30, 2024 •

edited

Loading

subhramit Mar 30, 2024 •

edited

Loading

koppor Mar 30, 2024

subhramit Mar 30, 2024

subhramit Mar 30, 2024

koppor Mar 30, 2024

subhramit commented Mar 30, 2024

koppor left a comment

koppor Mar 30, 2024

subhramit Mar 30, 2024

subhramit commented Mar 30, 2024

Remove warnings when importing search entries in unsaved libraries #11111

Remove warnings when importing search entries in unsaved libraries #11111

Conversation

subhramit commented Mar 30, 2024 • edited Loading

Description

Changes

Mandatory checks

Choose a reason for hiding this comment

subhramit Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhramit Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

subhramit Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhramit commented Mar 30, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhramit commented Mar 30, 2024

subhramit commented Mar 30, 2024 •

edited

Loading

subhramit Mar 30, 2024 •

edited

Loading

subhramit Mar 30, 2024 •

edited

Loading

subhramit Mar 30, 2024 •

edited

Loading