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

Fix file not linked after DL from fulltext fetcher #7162

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Dec 5, 2020

Remove redundant code

While debugging for #7152 I found that the file is downloaded, but not linked to the library afterwards.
Fortunately I could just replace it with the specific download method already defined.

  • 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.

Remove redundant code
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 5, 2020
} catch (MalformedURLException exception) {
dialogService.showErrorDialogAndWait(Localization.lang("Invalid URL"), exception);
}
onlineFile.download();
Copy link
Member

Choose a reason for hiding this comment

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

I think with this change, downloading a pdf doesn't show any progress any more since the onlineFile is not added to the entry until the download succeeds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exactly sure, at this point we add the online fie link and then call the download method which also shows the progress

public void download() {
if (!linkedFile.isOnlineLink()) {
throw new UnsupportedOperationException("In order to download the file it has to be an online link");
}
try {
Optional<Path> targetDirectory = databaseContext.getFirstExistingFileDir(filePreferences);
if (targetDirectory.isEmpty()) {
dialogService.showErrorDialogAndWait(Localization.lang("Download file"), Localization.lang("File directory is not set or does not exist!"));
return;
}
URLDownload urlDownload = new URLDownload(linkedFile.getLink());
BackgroundTask<Path> downloadTask = prepareDownloadTask(targetDirectory.get(), urlDownload);
downloadTask.onSuccess(destination -> {
LinkedFile newLinkedFile = LinkedFilesEditorViewModel.fromFile(destination, databaseContext.getFileDirectories(filePreferences), externalFileTypes);
List<LinkedFile> linkedFiles = entry.getFiles();
int oldFileIndex = -1;
int i = 0;
while (i < linkedFiles.size() && oldFileIndex == -1) {
LinkedFile file = linkedFiles.get(i);
// The file type changes as part of download process (see prepareDownloadTask), thus we only compare by link
if (file.getLink().equalsIgnoreCase(linkedFile.getLink())) {
oldFileIndex = i;
}
i++;
}
if (oldFileIndex == -1) {
linkedFiles.add(0, newLinkedFile);
} else {
linkedFiles.set(oldFileIndex, newLinkedFile);
}
entry.setFiles(linkedFiles);
});
downloadProgress.bind(downloadTask.workDonePercentageProperty());
downloadTask.titleProperty().set(Localization.lang("Downloading"));
downloadTask.messageProperty().set(
Localization.lang("Fulltext for") + ": " + entry.getCitationKey().orElse(Localization.lang("New entry")));
downloadTask.showToUser(true);
taskExecutor.execute(downloadTask);

@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label Dec 5, 2020
@koppor
Copy link
Member

koppor commented Dec 7, 2020

Screenshot when using this PR (8 eyes principle)

grafik

Also linked in the entry editor:

grafik

Setting:

grafik

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Is this the first time I really like deleted code? 😇

@koppor koppor merged commit 8674477 into master Dec 7, 2020
@koppor koppor deleted the fixFullTextDownloadLink branch December 7, 2020 20:51
@tobiasdiez
Copy link
Member

Okay, but this progress dialog is really ugly, especially compared to the nice experience you get after "download from a given url" (where you have to paste the url manually).

@Siedlerchr
Copy link
Member Author

I didn't change anything at the progress dialog...That is coming from the previous step, where the fulltext fetcher searches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants