-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Show DOI lookup link in citation relations tab #13285
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
51c81ee
12db851
374ff6a
c58cea5
260c76d
4bcccfa
ac8de81
55797cc
1bc1201
d080769
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,7 @@ | |
- We improved JabRef's internal document viewer. It now allows text section, searching and highlighting of search terms and page rotation [#13193](https://github.com/JabRef/jabref/pull/13193). | ||
- When importing a PDF, there is no empty entry column shown in the multi merge dialog. [#13132](https://github.com/JabRef/jabref/issues/13132) | ||
- We added a progress dialog to the "Check consistency" action and progress output to the corresponding cli command. [#12487](https://github.com/JabRef/jabref/issues/12487) | ||
- We added Enhanced "Citation Relations" feature: "Look up a DOI and try again." is now a clickable hyperlink that triggers a DOI lookup. The link shows result states ("Looking up DOI...", "No DOI found", "List of citations") as appropriate. | ||
|
||
### Fixed | ||
|
||
|
@@ -1575,6 +1576,7 @@ | |
- We added the ability to use negation in export filter layouts. [#5138](https://github.com/JabRef/jabref/pull/5138) | ||
- Focus on Name Area instead of 'OK' button whenever user presses 'Add subgroup'. [#6307](https://github.com/JabRef/jabref/issues/6307) | ||
- We changed the behavior of merging that the entry which has "smaller" bibkey will be selected. [#7395](https://github.com/JabRef/jabref/issues/7395) | ||
- we added a new | ||
Check failure on line 1579 in CHANGELOG.md
|
||
|
||
### Fixed | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
import javafx.scene.control.Button; | ||
import javafx.scene.control.ButtonType; | ||
import javafx.scene.control.DialogPane; | ||
import javafx.scene.control.Hyperlink; | ||
import javafx.scene.control.Label; | ||
import javafx.scene.control.ProgressIndicator; | ||
import javafx.scene.control.ScrollPane; | ||
|
@@ -30,6 +31,8 @@ | |
import javafx.scene.layout.HBox; | ||
import javafx.scene.layout.Priority; | ||
import javafx.scene.layout.VBox; | ||
import javafx.scene.text.Text; | ||
import javafx.scene.text.TextFlow; | ||
|
||
import org.jabref.gui.DialogService; | ||
import org.jabref.gui.LibraryTab; | ||
|
@@ -52,6 +55,9 @@ | |
import org.jabref.logic.citation.SearchCitationsRelationsService; | ||
import org.jabref.logic.database.DuplicateCheck; | ||
import org.jabref.logic.exporter.BibWriter; | ||
import org.jabref.logic.importer.FetcherClientException; | ||
import org.jabref.logic.importer.FetcherServerException; | ||
import org.jabref.logic.importer.fetcher.CrossRef; | ||
import org.jabref.logic.importer.fetcher.citation.CitationFetcher; | ||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.logic.os.OS; | ||
|
@@ -201,46 +207,50 @@ private SplitPane getPaneAndStartSearch(BibEntry entry) { | |
|
||
refreshCitingButton.setOnMouseClicked(_ -> { | ||
searchForRelations( | ||
entry, | ||
citingListView, | ||
entry, | ||
citingListView, | ||
abortCitingButton, | ||
refreshCitingButton, | ||
CitationFetcher.SearchType.CITES, | ||
importCitingButton, | ||
citingProgress); | ||
refreshCitingButton, | ||
CitationFetcher.SearchType.CITES, | ||
importCitingButton, | ||
citingProgress, | ||
true); | ||
}); | ||
|
||
refreshCitedByButton.setOnMouseClicked(_ -> searchForRelations( | ||
entry, | ||
citedByListView, | ||
entry, | ||
citedByListView, | ||
abortCitedButton, | ||
refreshCitedByButton, | ||
CitationFetcher.SearchType.CITED_BY, | ||
importCitedByButton, | ||
citedByProgress)); | ||
refreshCitedByButton, | ||
CitationFetcher.SearchType.CITED_BY, | ||
importCitedByButton, | ||
citedByProgress, | ||
true)); | ||
|
||
// Create SplitPane to hold all nodes above | ||
SplitPane container = new SplitPane(citingVBox, citedByVBox); | ||
styleFetchedListView(citedByListView); | ||
styleFetchedListView(citingListView); | ||
|
||
searchForRelations( | ||
entry, | ||
citingListView, | ||
abortCitingButton, | ||
entry, | ||
citingListView, | ||
abortCitingButton, | ||
refreshCitingButton, | ||
CitationFetcher.SearchType.CITES, | ||
importCitingButton, | ||
citingProgress); | ||
CitationFetcher.SearchType.CITES, | ||
importCitingButton, | ||
citingProgress, | ||
false); | ||
|
||
searchForRelations( | ||
entry, | ||
citedByListView, | ||
abortCitedButton, | ||
entry, | ||
citedByListView, | ||
abortCitedButton, | ||
refreshCitedByButton, | ||
CitationFetcher.SearchType.CITED_BY, | ||
importCitedByButton, | ||
citedByProgress); | ||
CitationFetcher.SearchType.CITED_BY, | ||
importCitedByButton, | ||
citedByProgress, | ||
false); | ||
|
||
return container; | ||
} | ||
|
@@ -434,13 +444,29 @@ protected void bindToEntry(BibEntry entry) { | |
*/ | ||
private void searchForRelations(BibEntry entry, CheckListView<CitationRelationItem> listView, Button abortButton, | ||
Button refreshButton, CitationFetcher.SearchType searchType, Button importButton, | ||
ProgressIndicator progress) { | ||
ProgressIndicator progress, boolean shouldRefresh) { | ||
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. avoid magic booleans. if someone calls searchForRelations(...,...,true), nobody knows what true means. this may be done for simple methods, but for a recursive method self-documenting code is most important. |
||
if (entry.getDOI().isEmpty()) { | ||
hideNodes(abortButton, progress); | ||
showNodes(refreshButton); | ||
listView.getItems().clear(); | ||
listView.setPlaceholder( | ||
new Label(Localization.lang("The selected entry doesn't have a DOI linked to it. Lookup a DOI and try again."))); | ||
Text doiLookUpText = new Text(Localization.lang("The selected entry doesn't have a DOI linked to it.")); | ||
Hyperlink doiLookUpHyperLink = new Hyperlink(Localization.lang("Lookup a DOI and try again.")); | ||
TextFlow doiLookUpTextFlow = new TextFlow(doiLookUpText, doiLookUpHyperLink); | ||
Label placeHolder = new Label("", doiLookUpTextFlow); | ||
doiLookUpHyperLink.setOnAction(e -> { | ||
CrossRef doiFetcher = new CrossRef(); | ||
BackgroundTask.wrap(() -> doiFetcher.findIdentifier(entry)) | ||
.onRunning(() -> listView.setPlaceholder(new Label("Looking up DOI..."))) | ||
.onSuccess(doiIdentifier -> { | ||
if (doiIdentifier.isPresent()) { | ||
entry.setField(StandardField.DOI, doiIdentifier.get().asString()); | ||
searchForRelations(entry, listView, abortButton, refreshButton, searchType, importButton, progress, shouldRefresh); | ||
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. you are introducing a recursive cascade here with endless creation of UI elements. Extract the background task. put the result in a property. update the view if the property is modified. |
||
} else { | ||
dialogService.notify("No DOI found"); | ||
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}).onFailure((exception) -> handleIdentifierFetchingError(exception, doiFetcher)).executeWith(taskExecutor); | ||
}); | ||
listView.setPlaceholder(placeHolder); | ||
return; | ||
} | ||
|
||
|
@@ -481,6 +507,19 @@ private void searchForRelations(BibEntry entry, CheckListView<CitationRelationIt | |
.executeWith(taskExecutor); | ||
} | ||
|
||
private void handleIdentifierFetchingError(Exception exception, CrossRef fetcher) { | ||
LOGGER.error("Error while fetching identifier", exception); | ||
if (exception instanceof FetcherClientException) { | ||
dialogService.showInformationDialogAndWait(Localization.lang("Look up %0", fetcher.getName()), Localization.lang("No data was found for the identifier")); | ||
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else if (exception instanceof FetcherServerException) { | ||
dialogService.showInformationDialogAndWait(Localization.lang("Look up %0", fetcher.getName()), Localization.lang("Server not available")); | ||
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else if (exception.getCause() != null) { | ||
dialogService.showWarningDialogAndWait(Localization.lang("Look up %0", fetcher.getName()), Localization.lang("Error occurred %0", exception.getCause().getMessage())); | ||
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
dialogService.showWarningDialogAndWait(Localization.lang("Look up %0", fetcher.getName()), Localization.lang("Error occurred %0", exception.getCause().getMessage())); | ||
} | ||
} | ||
|
||
/** | ||
* TODO: Make the method return a callable and let the calling method create the background task. | ||
*/ | ||
|
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.
artifact?