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

Add "Look up DOIs" to Quality menu #2442

Merged
merged 5 commits into from
Mar 18, 2017
Merged

Add "Look up DOIs" to Quality menu #2442

merged 5 commits into from
Mar 18, 2017

Conversation

koppor
Copy link
Member

@koppor koppor commented Dec 31, 2016

This adds "Lookup DOIs" to the quality menu. Additionally: Rename lookup to look up (see http://notaverb.com/lookup)

With an updating status bar:

grabbed_20161231-184622

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

Additionally: Rename lookup to look up (see http://notaverb.com/lookup)
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 31, 2016
NamedCompound namedCompound = new NamedCompound(Localization.lang("Look up DOIs"));
int count = 0;
int foundCount = 0;
for (BibEntry bibEntry : bibEntries) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to directly filter out the bibtex fields which have a DOI here with a stream?
e.g bibentries.stream().filter(x->hasField(FielName.DOI).....

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea how to create a progress bar there.

I found http://stackoverflow.com/a/26814087/873282, but that code looks much more complicated than the current code. Should I change it?

Copy link
Member

Choose a reason for hiding this comment

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

Na then go for it now.,

frame.output(Localization.lang("Looking up DOIs... - entry %0 out of %1 - found %2", Integer.toString(count), totalCount, Integer.toString(foundCount)));
if (!bibEntry.hasField(FieldName.DOI)) {
Optional<DOI> doi = DOI.fromBibEntry(bibEntry);
if (doi.isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

You could chain the optionals with flatmap, avoid the ifPresent cascading...

http://www.nurkiewicz.com/2013/08/optional-in-java-8-cheat-sheet.html

Copy link
Member Author

Choose a reason for hiding this comment

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

No clue how I can do that with progress...

Copy link
Member

Choose a reason for hiding this comment

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

Okay, was just an idea. If it does not make it easier forget about it

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Some minor changes and its okay from my side


private static final Log LOGGER = LogFactory.getLog(LookupDOIsWorker.class);

public LookupDOIsWorker(JabRefFrame frame) {
Copy link
Member

Choose a reason for hiding this comment

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

For maximal reusability, could you please make this class independent on DOI. There are many other identifier for which such an automatic look-up makes sense.

Copy link
Member Author

@koppor koppor Jan 20, 2017

Choose a reason for hiding this comment

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

  • Language: Either ID or let the caller set the DOI name
  • lines 43 to 46 somehow extract. Interface: CanCompleteEntry

Copy link
Member

Choose a reason for hiding this comment

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

@koppor can you please rebase and I will implement then the rest. Deal?

@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 9, 2017
@koppor koppor added on-hold and removed on-hold labels Jan 20, 2017
koppor and others added 3 commits March 17, 2017 09:08
# Conflicts:
#	CHANGELOG.md
#	src/main/java/org/jabref/gui/BasePanel.java
#	src/main/java/org/jabref/gui/JabRefFrame.java
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 17, 2017
@tobiasdiez
Copy link
Member

tobiasdiez commented Mar 17, 2017

@koppor thanks for rebased. I generalized it now to also work for other identifiers so this PR is now ready for review.

@@ -0,0 +1,8 @@
package org.jabref.model.entry.identifier;

public interface Identifier {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we call this class HasIdentifier?

@koppor
Copy link
Member Author

koppor commented Mar 17, 2017

LGTM 😇

I get multiple errors when using it. I don't get the information which entry is affected. But this is not an issue of this PR. Just wanted to note it down somewhere.

2017-03-17 18:14:36,528 Spin-0 ERROR Recursive call to appender GuiLogger

Caused by: org.jabref.logic.importer.ParseException: CrossRef API JSON format has changed
        at org.jabref.logic.importer.fetcher.CrossRef.jsonItemToBibEntry(CrossRef.java:131) ~[main/:?]
        at org.jabref.logic.importer.fetcher.CrossRef.lambda$getParser$3(CrossRef.java:88) ~[main/:?]
        at org.jabref.logic.importer.IdParserFetcher.findIdentifier(IdParserFetcher.java:75) ~[main/:?]
        ... 8 more
Caused by: org.json.JSONException: JSONObject["given"] not found.
        at org.json.JSONObject.get(JSONObject.java:471) ~[json-20160212.jar:?]
        at org.json.JSONObject.getString(JSONObject.java:717) ~[json-20160212.jar:?]
        at org.jabref.logic.importer.fetcher.CrossRef.toAuthors(CrossRef.java:144) ~[main/:?]
        at org.jabref.logic.importer.fetcher.CrossRef.jsonItemToBibEntry(CrossRef.java:117) ~[main/:?]
        at org.jabref.logic.importer.fetcher.CrossRef.lambda$getParser$3(CrossRef.java:88) ~[main/:?]
        at org.jabref.logic.importer.IdParserFetcher.findIdentifier(IdParserFetcher.java:75) ~[main/:?]

Caused by: java.io.IOException: Server returned HTTP response code: 500 for URL: http://api.crossref.org/works?query.title=A+%28sub%29graph+isomorphism+algorithm+for+matching+large+graphs&query.author=L.P.+Cordella+and+P.+Foggia+and+C.+Sansone+and+M.+Vento&filter=from-pub-date%3A2004&rows=20&offset=0

@koppor koppor merged commit 63620ff into master Mar 18, 2017
@koppor koppor deleted the lookupdoi branch March 18, 2017 08:44
Siedlerchr added a commit that referenced this pull request Mar 25, 2017
* upstream/master:
  Localization: General: French: Translation of new entries
  Localization: Menu: French: Translation of an entry (#2685)
  Fix #2680 and fix #2667: Swing errors are catched properly and without freezing (#2681)
  Do not log AND throw
  Replace misleading error message for fetcher connection error
  Document CrossRef test
  Fix subtitle detection for CrossRef fetcher
  Revert "Invoke LogMessages.add in JavaFX thread"
  Use global user agent
  Update mockito from 2.7.17 to 2.7.18
  Move GuiAppender to GUI package
  Invoke LogMessages.add in JavaFX thread
  [WIP] Put the PDFAnnotationImporter under Test, enhance FileAnnotationTab (#2640)
  Fix for "Paying off technical debt: almost all utility classes have a private constructor now." (#2672)
  Revert "Paying off technical debt: almost all utility classes have a private constructor now. (#2649)" (#2670)
  Paying off technical debt: almost all utility classes have a private constructor now. (#2649)
  Changed codeformatting for better fxml annotation (#2668)
  Disalbe Google Scholar tests on all CI environments (#2654)
  Fix JSONException in Crossref fetcher as mentioned in #2442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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