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.

Sign up for GitHub

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

Jump to bottom

fix-for-issue-4489 #4538

Merged
merged 7 commits into from
Dec 17, 2018
Merged

fix-for-issue-4489 #4538

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cde9b4e
Removed DownloadExternalFile call, replaced with LinkedFileViewModel
nrmancuso Dec 14, 2018
0bd59b3
Added some notes
nrmancuso Dec 14, 2018
3f67c97
Added addLinkedFileFromURL method to handle linking and display dialogs
nrmancuso Dec 15, 2018
69cc478
Fixed issues per codacity review
nrmancuso Dec 15, 2018
fd9448f
Fixed issues per gradlew check, added new Localization.lang entry,
nrmancuso Dec 15, 2018
2e3c902
Added extends SimpleCommand to FindFullTextAction
nrmancuso Dec 16, 2018
af710b9
Added entry to changelog
nrmancuso Dec 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 57 additions & 35 deletions src/main/java/org/jabref/gui/externalfiles/FindFullTextAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.jabref.gui.externalfiles;

import java.io.IOException;
import java.net.URL;
import java.nio.file.Path;
import java.util.ArrayList;
Expand All @@ -11,18 +10,23 @@

import javax.swing.SwingUtilities;

import javafx.beans.property.BooleanProperty;
import javafx.beans.property.ListProperty;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.beans.property.SimpleListProperty;
import javafx.collections.FXCollections;

import org.jabref.Globals;
import org.jabref.gui.BasePanel;
import org.jabref.gui.DialogService;
import org.jabref.gui.actions.BaseAction;
import org.jabref.gui.undo.UndoableFieldChange;
import org.jabref.gui.fieldeditors.LinkedFileViewModel;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.DefaultTaskExecutor;
import org.jabref.logic.importer.FulltextFetchers;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.FieldChange;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.FieldName;
import org.jabref.model.entry.LinkedFile;
import org.jabref.preferences.JabRefPreferences;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -39,6 +43,10 @@ public class FindFullTextAction implements BaseAction {
private final BasePanel basePanel;
private final DialogService dialogService;

//Stuff for downloading full texts
private final FulltextFetchers fetcher = new FulltextFetchers(JabRefPreferences.getInstance().getImportFormatPreferences());
Copy link
Member

Choose a reason for hiding this comment

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

We try to get rid of explicit calling JabRefPreferences., the ideal solution would be pass the ImportFormatPrefernces as parameter in the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

And while you are onto this class, you could refactor it to extend the SimpleCommand class (see for example WriteXMP Action)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Siedlerchr So, it should not implement the BaseAction interface anymore then, correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, replace the base action with SimpleCommand, instead of an action method you have an execute method, it's technically just a different name, the behavior is the same and you can just rename it

Copy link
Contributor Author

@nrmancuso nrmancuso Dec 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@Siedlerchr BasePanel.java doesn't like that because now the second argument when putting the actions in the map that actions are stored in is no longer a "BaseAction". How should I proceed?

Copy link
Member

@Siedlerchr Siedlerchr Dec 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

you can get rid of that action entry in the action map, look at the setup of the actions in the menus, There should be the SimpleCommand variant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help! I think I've got everything fixed up.

private final ListProperty<LinkedFileViewModel> files = new SimpleListProperty<>(FXCollections.observableArrayList(LinkedFileViewModel::getObservables));
Copy link
Member

Choose a reason for hiding this comment

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

It appears to me that you only add elements to this list, but never query its contents. In this case, you can probably remove this variable.


public FindFullTextAction(DialogService dialogService, BasePanel basePanel) {
this.basePanel = basePanel;
this.dialogService = dialogService;
Expand All @@ -47,8 +55,9 @@ public FindFullTextAction(DialogService dialogService, BasePanel basePanel) {
@Override
public void action() {
BackgroundTask.wrap(this::findFullTexts)
.onSuccess(downloads -> SwingUtilities.invokeLater(() -> downloadFullTexts(downloads)))
.executeWith(Globals.TASK_EXECUTOR);
.onSuccess(downloads -> SwingUtilities.invokeLater(() -> downloadFullTexts(downloads)))
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 you no longer need the swing utitilies call here because you effectively removed the swing code which needed this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! Lambdas (and programming in general) are all new to me, I'm still getting used to them. I have replaced this with a simple method reference to downLoadFullTexts. Thanks for the suggestion. I was also able to remove the now unnecessary import of swing also.

.executeWith(Globals.TASK_EXECUTOR);

}

private Map<Optional<URL>, BibEntry> findFullTexts() {
Expand Down Expand Up @@ -101,40 +110,53 @@ private void downloadFullTexts(Map<Optional<URL>, BibEntry> downloads) {

return;
}
DownloadExternalFile fileDownload = new DownloadExternalFile(dialogService,
basePanel.getBibDatabaseContext(), entry);
try {
fileDownload.download(result.get(), "application/pdf", file -> {
DefaultTaskExecutor.runInJavaFXThread(() -> {
Optional<FieldChange> fieldChange = entry.addFile(file);
if (fieldChange.isPresent()) {
UndoableFieldChange edit = new UndoableFieldChange(entry, FieldName.FILE,
entry.getField(FieldName.FILE).orElse(null), fieldChange.get().getNewValue());
basePanel.getUndoManager().addEdit(edit);
basePanel.markBaseChanged();
}
});

});
} catch (IOException e) {
LOGGER.warn("Problem downloading file", e);
basePanel.output(Localization.lang("Full text document download failed for entry %0",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined"))));
}
basePanel.output(Localization.lang("Finished downloading full text document for entry %0.",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined"))));
} else {
String title = Localization.lang("No full text document found");
String message = Localization.lang("No full text document found for entry %0.",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined")));

basePanel.output(message);
DefaultTaskExecutor.runInJavaFXThread(() -> dialogService.showErrorDialogAndWait(title, message));
//Download full text
BackgroundTask
.wrap(() -> fetcher.findFullTextPDF(entry))
.onSuccess(url -> addLinkedFileFromURL(result.get(), entry))
Copy link
Member

Choose a reason for hiding this comment

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

Since you already have the url in Optional<URL> result there is no need for the additional call to fetcher.findFullTextPDF(entry). Simply invoke addLinkedFileFromURL(result.get(), entry) without any wrapper.

.executeWith(Globals.TASK_EXECUTOR);

} else {
dialogService.notify(Localization.lang("No full text document found for entry %0.",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined"))));
}
finishedTasks.add(result);
}
for (Optional<URL> result : finishedTasks) {
downloads.remove(result);
}
}

/**
* This method attaches a linked file from a URL (if not already linked) to an entry and using the key and value pair
* from the findFullTexts map
* @param url the url "key"
* @param entry the entry "value"
*/
private void addLinkedFileFromURL(URL url, BibEntry entry) {

LinkedFile newLinkedFile = new LinkedFile(url, "");

if (!entry.getFiles().contains(newLinkedFile)) {

LinkedFileViewModel onlineFile = new LinkedFileViewModel(
newLinkedFile,
entry,
basePanel.getBibDatabaseContext(),
Globals.TASK_EXECUTOR,
dialogService,
JabRefPreferences.getInstance());

files.add(onlineFile);
onlineFile.download();

entry.addFile(newLinkedFile);
Copy link
Member

Choose a reason for hiding this comment

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

Here you probably need to add the onlineFile.getFile , because otherwise you add just the url to the entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tobiasdiez thanks for all of the great feedback! I should have everything wrapped up by tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tobiasdiez I think I've gotten everything taken care of. Thanks again for the help!

dialogService.notify(Localization.lang("Finished downloading full text document for entry %0.",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined"))));
} else {
dialogService.notify(Localization.lang("Full text document for entry %0 already linked.",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined"))));
}
}
}
2 changes: 1 addition & 1 deletion src/main/resources/l10n/JabRef_en.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1991,6 +1991,7 @@ However,\ a\ new\ database\ was\ created\ alongside\ the\ pre-3.6\ one.=However,
Opens\ a\ link\ where\ the\ current\ development\ version\ can\ be\ downloaded=Opens a link where the current development version can be downloaded
See\ what\ has\ been\ changed\ in\ the\ JabRef\ versions=See what has been changed in the JabRef versions
Referenced\ BibTeX\ key\ does\ not\ exist=Referenced BibTeX key does not exist
Full\ text\ document\ for\ entry\ %0\ already\ linked.=Full text document for entry %0 already linked.
Finished\ downloading\ full\ text\ document\ for\ entry\ %0.=Finished downloading full text document for entry %0.
Look\ up\ full\ text\ documents=Look up full text documents
You\ are\ about\ to\ look\ up\ full\ text\ documents\ for\ %0\ entries.=You are about to look up full text documents for %0 entries.
Expand Down Expand Up @@ -2132,7 +2133,6 @@ Any\ file=Any file

No\ linked\ files\ found\ for\ export.=No linked files found for export.

Full\ text\ document\ download\ failed\ for\ entry\ %0=Full text document download failed for entry %0
No\ full\ text\ document\ found\ for\ entry\ %0.=No full text document found for entry %0.

Delete\ Entry=Delete Entry
Expand Down