-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Added a download checkbox to the import dialog #6381
Merged
+58
−6
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
526099e
Added a download checkbox to the import dialog
btut f09a0ff
Added languages
btut 1854e8d
removed translations
btut 18ac794
Fixed style problems
btut b8a4b1b
Added preference in import tab
btut eb743f0
Fixed style
btut 50d2731
Removed singleton-access to preferences
btut 6e00bf0
Downloaded file is inserted before link
btut 091d4a2
Fix style
btut d2bfb89
Consider FileDirPattern when downloading fulltext
btut 01ca6e4
Added changes
btut d2f3c69
Merge branch 'master' of https://github.com/JabRef/jabref into featur…
btut File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
import javafx.scene.Node; | ||
import javafx.scene.control.Button; | ||
import javafx.scene.control.ButtonType; | ||
import javafx.scene.control.CheckBox; | ||
import javafx.scene.control.Label; | ||
import javafx.scene.control.ToggleButton; | ||
import javafx.scene.control.Tooltip; | ||
|
@@ -50,6 +51,7 @@ public class ImportEntriesDialog extends BaseDialog<Boolean> { | |
public ButtonType importButton; | ||
public Label totalItems; | ||
public Label selectedItems; | ||
public CheckBox downloadLinkedOnlineFiles; | ||
private final BackgroundTask<ParserResult> task; | ||
private ImportEntriesViewModel viewModel; | ||
@Inject private TaskExecutor taskExecutor; | ||
|
@@ -77,9 +79,11 @@ public ImportEntriesDialog(BibDatabaseContext database, BackgroundTask<ParserRes | |
Button btn = (Button) this.getDialogPane().lookupButton(importButton); | ||
btn.disableProperty().bind(booleanBind); | ||
|
||
downloadLinkedOnlineFiles.setSelected(preferences.getFilePreferences().getDownloadLinkedFiles()); | ||
|
||
setResultConverter(button -> { | ||
if (button == importButton) { | ||
viewModel.importEntries(entriesListView.getCheckModel().getCheckedItems()); | ||
viewModel.importEntries(entriesListView.getCheckModel().getCheckedItems(), downloadLinkedOnlineFiles.isSelected()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} else { | ||
dialogService.notify(Localization.lang("Import canceled")); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@btut Have a look at the method
addLinkedFileFromURL
in DownloadFullTextAction, that solves a similar caseIn the importer you would create a progressbar/dialog and bind it to the prepareDownloadTask.progressProperty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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!
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, go ahead with a PR, makes it easier to help for specific questions