Added a download checkbox to the import dialog #6381

btut · 2020-04-30T16:37:49Z

When importing entries, users now have a checkbox available to download
files linked via url.

I am not a GUI expert and am wondering whether my fix is appropriate and is considered clean in the MVC-concept. Since the functionality was already implemented in the LinkedFileViewModel, I just created an instance and let it handle the download. Is that ok? If not, copying the code over (and resolving some dependencies) would be the easiest, but also the dirtiest solution to this problem. Is there an appropriate place the code in LinkedFileViewModel could be moved, so it is callable in ImportEntriesViewModel?

Fixes #5662

Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

When importing entries, users now have a checkbox available to download files linked via url.

Siedlerchr · 2020-04-30T17:10:39Z

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

@@ -79,7 +81,7 @@ public ImportEntriesDialog(BibDatabaseContext database, BackgroundTask<ParserRes

        setResultConverter(button -> {
            if (button == importButton) {
-                viewModel.importEntries(entriesListView.getCheckModel().getCheckedItems());
+                viewModel.importEntries(entriesListView.getCheckModel().getCheckedItems(), downloadLinkedOnlineFiles.isSelected());


As JabRef uses the MVVM approach, the ideal solution would be to add a property for the checkbox in the viewmodel and bind that to the checkbox's selected item property. That should have ideally also be already the case for the checkedItems.. Don't know why it's not done.

https://devdocs.jabref.org/readings-on-coding/javafx

Siedlerchr · 2020-04-30T17:16:09Z

Thanks for your contribution! Looks good already! I think your approach is the best solution, there are multiple pieces in the code which create the LinkedFileViewModel, so it's totally fine.

If you want to learn more about the GUI principle MVVM https://devdocs.jabref.org/readings-on-coding/javafx
If you want to make it persistent I think an option in the Preferences is fine. Maybe add to the File tab? Then you could add it as part of the FilePreferences.

Siedlerchr · 2020-04-30T17:27:00Z

Please do not add the translations here directly, they are managed via https://crowdin.com/project/jabref and will be overwritten if you modify them here,

btut · 2020-04-30T17:27:35Z

Hi. Thanks for the fast reply. So I should add the english file and leave the others be?

btut · 2020-04-30T17:36:47Z

Thanks for your contribution! Looks good already! I think your approach is the best solution, there are multiple pieces in the code which create the LinkedFileViewModel, so it's totally fine.

If you want to learn more about the GUI principle MVVM https://devdocs.jabref.org/readings-on-coding/javafx
If you want to make it persistent I think an option in the Preferences is fine. Maybe add to the File tab? Then you could add it as part of the FilePreferences.

I think the import tab would be more appropriate, don't you think?

Siedlerchr · 2020-04-30T18:18:03Z

Just add it where it suits best ;)

btut · 2020-05-01T08:45:28Z

I went with FilePreferences after all as the other settings of the ImportTab are stored there as well.
One last thing that bugs me: after downloading the file, it is put after the online link, so when I click on the entry, it still opens the online link. I think it would make sense to reverse that order, don't you think?
I plan on reversing it in any case and giving the user the option (in the preferences only, not on each import) to remove the online link in case a file was successfully downloaded.

Siedlerchr · 2020-05-01T09:21:35Z

@btut Yes that sounds like a good idea, but I am not sure if removing the online link at all is a good idea. There might be cases where it's useful to keep it. Maybe when you share the bib file and the other user does not have downloaded the PDF yet. Just put the PDF in front of the link

btut · 2020-05-01T09:43:01Z

@Siedlerchr exactly. Thats why I would have that functionality configurable in preferences (and off by default)

Siedlerchr · 2020-05-01T09:44:27Z

Okay, that sounds reasonable.

calixtus

LGTM, just a minor suggestion, but should already be mergable.

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

btut · 2020-05-04T09:24:25Z

I noticed that JabRef was opening the online link by default not because the file was second in the list, but because it was never actually linked to the entry. I did that and now when clicking on the file JabRef asks me if I want to open the link or the file.
Now, it makes no more sense to me to have the option to delete the link and I will not implement it.

…e/downloadFilesOnImport

btut · 2020-05-04T10:42:14Z

Ok, I think I am satisfied now.

To sum up:
The import dialog now offers a check-box. If the user selects the check-box and the imported entry contains an online link, JabRef will download the file. It is stored in the directory set up by 'File directory pattern' and named according to the 'Filename format pattern' in the import-tab of the preferences dialog. The file entry is prepended to the imported bib-entry.
The user can choose to have that check-box selected by default in the import-tab of the preferences dialog.

Siedlerchr

Thanks, codewise looks good to me!
Also good that you now take the fileDirPattern and FileNamePattern into account. I think this was also one issue when downloading from the entry editor

btut · 2020-05-04T11:50:42Z

Thanks, codewise looks good to me!

Great!

Also good that you now take the fileDirPattern and FileNamePattern into account. I think this was also one issue when downloading from the entry editor

Indeed, this bugged me even before I coded this feature. I tried looking for an issue to reference here but could not find any thoug, so maybe this was just me.

calixtus

Again, looks good to me. Great work, thank you.

Siedlerchr · 2020-05-04T14:12:53Z

@btut Thanks again for your good work! Looking forward seeing more from you! :)

tobiasdiez · 2020-05-04T15:59:32Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

@@ -415,6 +416,7 @@ public void download() {
                LinkedFile newLinkedFile = LinkedFilesEditorViewModel.fromFile(destination, databaseContext.getFileDirectoriesAsPaths(filePreferences), externalFileTypes);
                linkedFile.setLink(newLinkedFile.getLink());
                linkedFile.setFileType(newLinkedFile.getFileType());
+                entry.addFile(0, newLinkedFile);


@btut Thanks for your implementation. I fear this addition here now duplicates the file if you right-click an online link and select "Download". Can you please double check this.
(In case this is really an issue, one option would be to use prepareDownloadTask below in the importer. Then you can also display a nice progress window for the downloads.)

Good point @tobiasdiez. I checked, and this does indeed duplicate the entry.
the progress-bar next to the link entry does look very nice indeed and would probably be good to have when working with large files or slow connections. I can imagine people thinking it didn't work because the file doesn't show up immediately.

I don't understand your suggestion of using prepareDownloadTask, though. Isn't that implicitly used by the download() method anyway?

As far as I can tell the right-click -> download menu item just calls the download-function of the LinkedFileViewModel. In that case, the LInkedFileViewModel is the one that is actually displayed. Therefore the progress bar can be shown. However; I have no access to that object, correct? I have the LinkedFileViewModel that I use for the download, but I cannot add it to any LinkedFilesEditorViewModel to be displayed.

One quick fix I could think of would be to keep the online-link even for right-click->download actions or delete the link for both methods. There would still be no progress bar for the imported files, but at least it stops duplication.

The best fix (IMO) would be to gain somehow access to the actual LinkedFileViewModel of the online link after the import and call download from there, basically imitating a user right-clicking the item and selecting download. I would need some pointers on how to get that access though.

I guess it would work if I moved the downloadProgressProperty from LinkedFileViewModel to LinkedFile. That object lives in the BibEntry and (AFAICT) and would be the same both for the importer and the LinkedFileViewModel.
Would moving that property be an acceptable option?

@btut Have a look at the method addLinkedFileFromURL in DownloadFullTextAction, that solves a similar case
In the importer you would create a progressbar/dialog and bind it to the prepareDownloadTask.progressProperty

That method just notifies the user after a successful download. I see no progress bar. My issue is not in getting the progress of the download, but where to display the progress bar. Just popping up a dialogue with a bar is no viable solution in my opinion. When I import an entry I don't want to watch a progress bar while most of the entry is already there, I want to start working with the entry.

As I said, a progress bar would be very helpful, but I don't think popping a dialogue (or remaining in the import dialogue as long as the files are not ready, if that is what you mean) would be a good idea. I like how the right-click -> download action puts a small progress bar in the file entry itself. I can work with the bib entry, and if I try to open up the file, I see the progress bar.

However, since there is no access to the linked files editor from the importer, I cannot add the progress bar there. I think moving the download-related properties to LinkedFile would work quite well; I just don't know if it is compliant with how JabRef is developed.

One more thought on that addLinkedFileFromURL method you mentioned. It seems to share a lot of code with the download method of LinkedFileViewModel. If we move the properties I was talking about to the LinkedFile class; the LinkedFilesEditorViewModel would always correctly display the progress no matter where the download() method of LinkedFileViewModel would be called from. In that case, I would suggest calling the download Method from addLinkedFileFromURL to reduce code duplication and have a unique way of handling file downloads and showing it's progress.

Ok, you convinced me, let's continue trying to do it in the background. If users experience problems, then we can investigate of how to best solve them and maybe go back to the blocking-ui strategy if nothing else works.

I noticed in the code (never while running JabRef and I cannot find any piece of code where it is actually used and even setting it's visibility to true does not show anything, maybe it's just dead code) that there is a progress bar in the right end of the status line

The old JabRef 3.x contained a status bar, which was removed since it wasn't used that much. So if there is still code related to this in the code base, then it is definitely dead code and can safely be removed. May I ask you to open a PR removing it? Thanks!

If that progress bar is dead code, I guess there was one at some point which was then removed, so you probably didn't like it. In that case, what do you think about having a small loading circle or a download icon in the top right, next to the menu bar? It could be turning/greyed out while downloading and a tooltip could display the progress of single files.

I like it! Very good idea. Might also be helpful to show progress for other actions as well (e.g. cleanup). One possibility to implement this would be adding an ObservableList of open tasks (including their progress) in StatusManager.

I guess the most pressing thing would be to fix the download right-click, and then afterwards worry about a nice progress display.

I like it! Very good idea. Might also be helpful to show progress for other actions as well (e.g. cleanup). One possibility to implement this would be adding an ObservableList of open tasks (including their progress) in StatusManager.

That was the basic idea. BackgroundTask already has a workDonePercentageProperty, so all thats left to do is build a list of them and display it.
I have a couple of questions though:

The list of tasks would consist of a text label and a progress bar. BackgroundTask has a StringProperty message, I would use that one for the label. Unfortunately, the message does not seem to be set by the file downloaders. It is only set by the UnlinkedFilesCrawler. Did I miss something? If not, I would add a message (something like "Downloading file from "). For other tasks that use BackgroundTask we could either add a message parameter to the Background-Task constructor to force users to provide a sensible message, or we could set the default message to " in progress", so at the very least it says "BackgroundTask" in progress, for child-classes it would take the child's name and produce a somewhat usable label.

I can't seem to find the fxml file for the main JabRef frame where I would add the status wheel. Could you please help me out here?

@btut The StringProperty is as the name implies a property that you can set for the task.In the File Download task you can simply set it to (calling setValue) on it with a text. e.g. "Downloading files".
As this property is exposed you can bind a label to it.
For a quick understanding what those properties are and how they work:
https://www.dummies.com/programming/java/javafx-binding-properties/

JabRefFrame has no fxml. Have a look at the init/ initLayout Method().
You can always add

Thanks for the input. I already started drafting some code and came by the properties and did some reading.
I implemented a simple dialog using the TaskProgressView from javafx control. That looks pretty nice IMO. Unfortunately (and quite obviously) it works on a list of javafx tasks, not JabRef BackgroundTasks.
I noticed that BackgroundTasks are converted to javafx tasks anyways, so when that is done I enlist them in an ObservableList I created in StateManager. The tasks show up, but the bindings do not seem to work, I still have to figure out why. So right now, one can see a list of tasks, but the tasks have no title, no message and an infinite progress bar.

If you don't mind I would like to open a WIP PR and if I need some help I would start a discussion there. Is that ok?

Yes, go ahead with a PR, makes it easier to help for specific questions

calixtus · 2020-05-06T17:09:25Z

I think the status item is enough work for another PR. Best thing would probably to finish the logic of this PR first with the most basic progress indicator possible, merge this PR and then create another PR for the status indicator, as it would probably affect way more parts of the code.

calixtus · 2020-05-06T17:11:38Z

Don't misunderstand me, I do like the idea of a generic progress for JabRef, too.
Just do one step after another.

btut · 2020-05-07T05:22:35Z

Ok, to conclude this PR I suggest the following:

PR to fix the entry duplication for the right-click download action
PR to remove the dead progress-bar code
PR to implement the list of background tasks and it's graphical representation in the GUI

I can probably get to the first two today but will need some time for the third. I will create a PR after only a couple of commits so we can start discussing there. I hope you don't mind.

calixtus · 2020-05-07T06:30:35Z

Great, thank you. Go ahead.
Splitting this up in multiple PR's makes it just easier to review and later to debug, if there are unknown sideeffects.

…ptyLib * upstream/master: (23 commits) Make wrap fields also visible in entry editor (#6315) Add launcher to fix extension import in snap (#6439) add concise message when SaveException happen (#6444) Squashed 'src/main/resources/csl-locales/' changes from 79845b087b..cbb45961b8 Squashed 'src/main/resources/csl-styles/' changes from 906cd6d..270cd32 Addition to the early pull #6438 (#6441) Fix the bug #6421 (#6438) Cleanup dead code (#6436) Fixed entry duplication on file download (#6437) Add workaround for buildSrc issue (#6433) Remove dash from illegal characters. (#6300) Docs: For developers: New architectural decision added to list (#6397) Added a download checkbox to the import dialog (#6381) Bump jaxb-xjc from 2.3.2 to 2.3.3 (#6410) Bump flexmark-ext-gfm-strikethrough from 0.61.20 to 0.61.24 (#6413) Bump byte-buddy-parent from 1.10.9 to 1.10.10 (#6408) Bump flexmark-ext-gfm-tasklist from 0.61.20 to 0.61.24 (#6412) Bump org.beryx.jlink from 2.17.8 to 2.18.0 (#6411) Bump tika-core from 1.24 to 1.24.1 (#6409) Bump flexmark from 0.61.20 to 0.61.24 (#6416) ...

* upstream/master: Added a download checkbox to the import dialog (#6381) Bump jaxb-xjc from 2.3.2 to 2.3.3 (#6410) Bump flexmark-ext-gfm-strikethrough from 0.61.20 to 0.61.24 (#6413) Bump byte-buddy-parent from 1.10.9 to 1.10.10 (#6408) Bump flexmark-ext-gfm-tasklist from 0.61.20 to 0.61.24 (#6412) Bump org.beryx.jlink from 2.17.8 to 2.18.0 (#6411) Bump tika-core from 1.24 to 1.24.1 (#6409) Bump flexmark from 0.61.20 to 0.61.24 (#6416) Bump classgraph from 4.8.77 to 4.8.78 (#6414) Restore some missing keyboard shortcuts (#6406) Fix StartupWMClass (#6398)

Added a download checkbox to the import dialog

526099e

When importing entries, users now have a checkbox available to download files linked via url.

Siedlerchr reviewed Apr 30, 2020

View reviewed changes

Added languages

f09a0ff

btut added 2 commits April 30, 2020 19:31

removed translations

1854e8d

Fixed style problems

18ac794

btut added 2 commits May 1, 2020 10:22

Added preference in import tab

b8a4b1b

Fixed style

eb743f0

calixtus reviewed May 1, 2020

View reviewed changes

src/main/java/org/jabref/gui/importer/ImportEntriesDialog.java Outdated Show resolved Hide resolved

btut added 2 commits May 4, 2020 10:31

Removed singleton-access to preferences

50d2731

Downloaded file is inserted before link

6e00bf0

btut added 4 commits May 4, 2020 11:51

Fix style

091d4a2

Consider FileDirPattern when downloading fulltext

d2bfb89

Added changes

01ca6e4

Merge branch 'master' of https://github.com/JabRef/jabref into featur…

d2f3c69

…e/downloadFilesOnImport

Siedlerchr approved these changes May 4, 2020

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 4, 2020

calixtus approved these changes May 4, 2020

View reviewed changes

Siedlerchr merged commit 7cb7ea6 into JabRef:master May 4, 2020

btut deleted the feature/downloadFilesOnImport branch May 4, 2020 14:55

tobiasdiez reviewed May 4, 2020

View reviewed changes

Xuanxuan-Zou mentioned this pull request May 6, 2020

Context menu of linked file not updated after downloading #5777

Closed

1 task

This was referenced May 7, 2020

Cleanup dead code #6436

Merged

Fixed entry duplication on file download #6437

Merged

btut mentioned this pull request May 7, 2020

Implement task progress indicator (and dialog) in the toolbar #6443

Merged

5 tasks

tobiasdiez mentioned this pull request Jun 7, 2020

Download of files put them in subfolders #6589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a download checkbox to the import dialog #6381

Added a download checkbox to the import dialog #6381

btut commented Apr 30, 2020 •

edited

Loading

Siedlerchr Apr 30, 2020

Siedlerchr commented Apr 30, 2020

Siedlerchr commented Apr 30, 2020 •

edited

Loading

btut commented Apr 30, 2020

btut commented Apr 30, 2020

Siedlerchr commented Apr 30, 2020

btut commented May 1, 2020

Siedlerchr commented May 1, 2020

btut commented May 1, 2020

Siedlerchr commented May 1, 2020

calixtus left a comment

btut commented May 4, 2020

btut commented May 4, 2020

Siedlerchr left a comment •

edited

Loading

btut commented May 4, 2020

calixtus left a comment

Siedlerchr commented May 4, 2020

tobiasdiez May 4, 2020

btut May 5, 2020

btut May 5, 2020

Siedlerchr May 5, 2020

btut May 5, 2020

tobiasdiez May 6, 2020 •

edited

Loading

btut May 7, 2020

Siedlerchr May 7, 2020

btut May 7, 2020

Siedlerchr May 7, 2020

calixtus commented May 6, 2020

calixtus commented May 6, 2020

btut commented May 7, 2020

calixtus commented May 7, 2020

Added a download checkbox to the import dialog #6381

Added a download checkbox to the import dialog #6381

Conversation

btut commented Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Siedlerchr commented Apr 30, 2020

Siedlerchr commented Apr 30, 2020 • edited Loading

btut commented Apr 30, 2020

btut commented Apr 30, 2020

Siedlerchr commented Apr 30, 2020

btut commented May 1, 2020

Siedlerchr commented May 1, 2020

btut commented May 1, 2020

Siedlerchr commented May 1, 2020

calixtus left a comment

Choose a reason for hiding this comment

btut commented May 4, 2020

btut commented May 4, 2020

Siedlerchr left a comment • edited Loading

Choose a reason for hiding this comment

btut commented May 4, 2020

calixtus left a comment

Choose a reason for hiding this comment

Siedlerchr commented May 4, 2020

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

tobiasdiez May 6, 2020 • 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

calixtus commented May 6, 2020

calixtus commented May 6, 2020

btut commented May 7, 2020

calixtus commented May 7, 2020

btut commented Apr 30, 2020 •

edited

Loading

Siedlerchr commented Apr 30, 2020 •

edited

Loading

Siedlerchr left a comment •

edited

Loading

tobiasdiez May 6, 2020 •

edited

Loading