DOI Fetcher using the new fetcher infrastructure #1885

zesaro · 2016-08-29T15:38:24Z

(Please do not squash at merge to keep author ships - @koppor)

Implement a new DOI Fetcher using the new fetcher infrastructure #1594
In this case, the two classes DOItoBibTeX & DOItoBibTeXFetcher has been merged into the new Fetcher.

Tests created for changes
Manually tested changed features in running JabRef

Siedlerchr · 2016-08-29T15:47:34Z

It would be really nice if you could address #1799 here as well. (I think the fetcher just has to be called in background thread somehow)

oscargus · 2016-09-01T12:55:40Z

The SwingWorker commit looks really interesting (and not so hard to use...)! 👍

Siedlerchr · 2016-09-01T15:29:18Z

src/main/java/net/sf/jabref/logic/importer/fetcher/DoiFetcher.java

+                return Optional.empty();
+            }
+        } catch (IOException e) {
+            throw new FetcherException("Bad URL when fetching ISBN info", e);


Seem like a copy & paste error ;)

tschechlovdev · 2016-09-05T10:52:44Z

src/main/java/net/sf/jabref/logic/importer/fetcher/DoiFetcher.java

+        this.prefs = preferences;
+    }
+
+    public static Optional<BibEntry> getEntryFromDOI(String doiStr, ParserResult parserResult, ImportFormatPreferences importFormatPreferences){


Why not just doi instead of doiStr ?
And would be nice if you would stay consistent and write everywhere preferences for example instead of importFormatPreferences or prefs.

chriba · 2016-09-05T13:52:07Z

Fixed a Bug when editing the Doi field and using the get BibTex button right beside it.
When saving the changed doi field to the BibEntry it rewrote it to the textfield which disabled the fetcher and enabled it again which aborted the useres click.

tschechlovdev · 2016-09-08T18:04:11Z

src/main/java/net/sf/jabref/gui/mergeentries/MergeFetchedEntryDialog.java

}
+

delete empty line

tobiasdiez · 2016-09-12T10:37:37Z

src/main/java/net/sf/jabref/gui/mergeentries/FetchAndMergeWorker.java

+        IdBasedFetcher fetcher;
+        if (FieldName.DOI.equals(field)) {
+            fetcher = new DoiFetcher(Globals.prefs.getImportFormatPreferences());
+        } else if (FieldName.ISBN.equals(field)) {


Use switch here

tobiasdiez · 2016-09-12T10:53:30Z

src/main/java/net/sf/jabref/logic/importer/fetcher/DoiFetcher.java

+                // BibTeX entry
+                return BibtexParser.singleFromString(cleanupEncoding(bibtexString), preferences);
+            } else {
+                LOGGER.warn(Localization.lang("Invalid DOI: '%0'.", identifier));


throw FetcherException instead (so that the caller gets an better error description instead of just an empty result)

# Conflicts: # src/main/java/net/sf/jabref/logic/importer/fileformat/PdfContentImporter.java

tobiasdiez · 2016-09-12T18:41:27Z

Could please add an additional test for an article (instead of book), for example doi: 10.1109/ICWS.2007.59. I think afterwards we can merge it.

koppor · 2016-09-12T21:10:48Z

src/main/java/net/sf/jabref/logic/importer/fetcher/DoiFetcher.java

+                Optional<BibEntry> result = BibtexParser.singleFromString(cleanupEncoding(bibtexString), preferences);
+                return result;
+            } else {
+                throw new FetcherException(String.format("Invalid DOI: %s", identifier));


Is the text of the fetcher exception presented in the UI at another place in the UI?

Then, please do not remove the translation and add Localization.lang here.

I think, at least b85c379 has to be reverted.

Still not fixed. Maybe exception.getLocalizedMessage() is a hint how to code that.

# Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java

koppor · 2016-09-13T18:13:01Z

Before this is ready to go:

Fix localization issue.

tobiasdiez · 2016-09-15T18:15:29Z

src/test/java/net/sf/jabref/logic/importer/fetcher/DoiFetcherTest.java

+
+    @Before
+    public void setUp() {
+        fetcher = new DoiFetcher(Globals.prefs.getImportFormatPreferences());


Could you pls fix the failing test by using mock(ImportFormatPreferences.class) here (if possible, if not JabRefPreferences.getInstance or so does the trick)

tobiasdiez · 2016-09-15T19:41:20Z

No idea why the integration tests don't work here. @JabRef/developers should we merge nonetheless?

koppor · 2016-09-15T19:45:39Z

Sure. Maybe, we should disable the integration tests (refs #1524).

zesaro added 3 commits August 29, 2016 17:28

merge DOItoBibTeX and DOItoBibTeXFetcher into DoiFetcher

b3c7169

add doifetcher to entryfetchers and update method calls

b8f38a8

add test for doifetcher

71c9d82

remove obsolete keys

66f316d

boceckts added the stupro label Sep 1, 2016

chriba force-pushed the doifetcher branch from 76daa4d to 8458378 Compare September 1, 2016 11:37

1799 move fetcher to own SwingWorker

e75f6da

chriba force-pushed the doifetcher branch from 8458378 to e75f6da Compare September 1, 2016 14:49

Siedlerchr reviewed Sep 1, 2016
View reviewed changes

refactor MergeFetchedEntryDialog and fix Typo

0ab9f17

tschechlovdev reviewed Sep 5, 2016
View reviewed changes

zesaro force-pushed the doifetcher branch from cb1ba1a to 49b7a09 Compare September 5, 2016 12:09

include feedback

c3f380d

zesaro force-pushed the doifetcher branch from 49b7a09 to c3f380d Compare September 5, 2016 12:10

fixed clicking on the get bibtex button beside the doi field

83ca5ba

boceckts added the stupro-ready-for-internal-review label Sep 5, 2016

zesaro mentioned this pull request Sep 5, 2016

Add ID Fetcher in Entrytypedialog #1925

Merged

6 tasks

tschechlovdev reviewed Sep 8, 2016
View reviewed changes

src/main/java/net/sf/jabref/gui/mergeentries/MergeFetchedEntryDialog.java

}

Copy link

Contributor

tschechlovdev Sep 8, 2016

Choose a reason for hiding this comment

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

delete empty line

delete empty line

a9e30ac

zesaro force-pushed the doifetcher branch from 4ca8030 to a9e30ac Compare September 12, 2016 09:12

tschechlovdev added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Sep 12, 2016

tobiasdiez reviewed Sep 12, 2016
View reviewed changes

zesaro added 3 commits September 12, 2016 19:56

include feedback

bddb30b

Merge remote-tracking branch 'upstream/master' into doifetcher

1ef372f

# Conflicts: # src/main/java/net/sf/jabref/logic/importer/fileformat/PdfContentImporter.java

fix ci

0cd71a4

add one more test

edfa2fa

koppor reviewed Sep 12, 2016
View reviewed changes

zesaro added 2 commits September 12, 2016 23:42

include feedback

2c5e62f

Merge remote-tracking branch 'upstream/master' into doifetcher

91da69b

# Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java

zesaro force-pushed the doifetcher branch from 63b7707 to 91da69b Compare September 12, 2016 21:46

stefan-kolb changed the title ~~[WIP] DOI Fetcher using the new fetcher infrastructure~~ DOI Fetcher using the new fetcher infrastructure Sep 13, 2016

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 13, 2016

zesaro added 2 commits September 15, 2016 19:15

always use fetcherexception's message for output

737de9e

simplify code

1936dfb

tobiasdiez requested changes Sep 15, 2016

View reviewed changes

zesaro force-pushed the doifetcher branch from d214936 to 22440f0 Compare September 15, 2016 18:35

use JabRefPreferences.getInstance() to fix ci

9a8f52e

zesaro force-pushed the doifetcher branch from 22440f0 to 9a8f52e Compare September 15, 2016 18:41

tobiasdiez approved these changes Sep 15, 2016

View reviewed changes

tobiasdiez closed this Sep 15, 2016

tobiasdiez reopened this Sep 15, 2016

koppor merged commit e298257 into JabRef:master Sep 15, 2016

zesaro deleted the doifetcher branch September 15, 2016 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOI Fetcher using the new fetcher infrastructure #1885

DOI Fetcher using the new fetcher infrastructure #1885

zesaro commented Aug 29, 2016 •

edited

Loading

Siedlerchr commented Aug 29, 2016

oscargus commented Sep 1, 2016

Siedlerchr Sep 1, 2016

chriba Sep 2, 2016

tschechlovdev Sep 5, 2016 •

edited

Loading

chriba commented Sep 5, 2016 •

edited

Loading

tschechlovdev Sep 8, 2016

tobiasdiez Sep 12, 2016

tobiasdiez Sep 12, 2016

tobiasdiez commented Sep 12, 2016

koppor Sep 12, 2016

koppor Sep 13, 2016

koppor commented Sep 13, 2016

tobiasdiez Sep 15, 2016

tobiasdiez commented Sep 15, 2016 •

edited

Loading

koppor commented Sep 15, 2016

DOI Fetcher using the new fetcher infrastructure #1885

DOI Fetcher using the new fetcher infrastructure #1885

Conversation

zesaro commented Aug 29, 2016 • edited Loading

Siedlerchr commented Aug 29, 2016

oscargus commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

chriba commented Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Sep 13, 2016

Choose a reason for hiding this comment

tobiasdiez commented Sep 15, 2016 • edited Loading

koppor commented Sep 15, 2016

zesaro commented Aug 29, 2016 •

edited

Loading

tschechlovdev Sep 5, 2016 •

edited

Loading

chriba commented Sep 5, 2016 •

edited

Loading

tobiasdiez commented Sep 15, 2016 •

edited

Loading