Fix bad web search error messages #2034

chriba · 2016-09-22T13:36:27Z

I rewrote the error messages when the fetchers fail (or added them).

boceckts

Some minor changes are needed otherwise it looks good to me 👍

boceckts · 2016-09-23T09:38:19Z

CHANGELOG.md

@@ -43,6 +43,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - [#2012](https://github.com/JabRef/jabref/issues/2012) Implemented integrity check for `month` field: Should be an integer or normalized (BibLaTeX), Should be normalized (BibTeX)

 ### Fixed
+- Improved error messages when using fetcher


I would put it in the changed section, also please add the issue number and url to your description.

That's up for debate. Before the error messages didn't work as intended (or didn't work at all).

boceckts · 2016-09-23T09:44:29Z

src/main/java/net/sf/jabref/gui/importer/fetcher/ACMPortalFetcher.java

-            status.showMessage(e.getMessage(),
+            LOGGER.error("Error while fetching from " + getTitle(), e);
+            status.showMessage(Localization.lang("Error while fetching from %0", getTitle()) +"\n"+
+                            Localization.lang("Please try again later and/or check your network connection."),


You use this code fragment in all the fetchers, which is why I would extract it to a (dialog) utility class, I'm not sure if there already exists one.

I exported them into the two main Dialog classes.

boceckts · 2016-09-23T09:47:36Z

src/main/java/net/sf/jabref/gui/importer/fetcher/GoogleScholarFetcher.java

@@ -82,14 +83,22 @@ public boolean processQueryGetPreview(String query, FetcherPreviewDialog preview
                hasRunConfig = true;
            }
            Map<String, JLabel> citations = getCitations(query);
+            if (citations.size() == 0) {


Replace with citations.isEmpty().

boceckts · 2016-09-23T09:59:01Z

src/main/java/net/sf/jabref/gui/importer/fetcher/GoogleScholarFetcher.java

            for (Map.Entry<String, JLabel> linkEntry : citations.entrySet()) {
                preview.addEntry(linkEntry.getKey(), linkEntry.getValue());
            }
-
            return true;


Put this block in the else statement and set one return false at the end of the method instead of the two.

boceckts · 2016-09-23T10:02:15Z

src/main/java/net/sf/jabref/gui/importer/fetcher/MedlineFetcher.java

-                        return false;
+                int numberToFetch = result.count;
+                if (numberToFetch > MedlineFetcher.PACING) {
+                    while (true) {


Please use a variable and just set it to false instead of using break.

boceckts · 2016-09-23T10:10:53Z

src/main/java/net/sf/jabref/gui/importer/fetcher/OAI2Fetcher.java

-            status.setStatus(Localization.lang("Error while fetching from %0", "OAI2"));
-            LOGGER.error("Error while fetching from OAI2", e);
+        } catch (IOException | SAXException | InterruptedException e) {
+            LOGGER.error("Error while fetching from " + getTitle(), e);


For me this this too much generalization, use the same error message for all IOExceptions and maybe InterruptedExceptions for the fetchers, but I think the error message doesn't make sense when an SAXException occurs!?

Does it have any additional value for the user if they know that is was a SAX exception?
They can't do anything about it.

In my opinion yes, a SAXException won't be resolved by retrying or checking the internet connection.

Now the SAXException will be shown to the user.

boceckts · 2016-09-23T13:07:52Z

src/main/java/net/sf/jabref/gui/importer/FetcherPreviewDialog.java

@@ -302,4 +303,10 @@ public void showMessage(String message, String title, int msgType) {
    public void showMessage(String message) {
        JOptionPane.showMessageDialog(this, message);
    }
+
+    public void showErrorMessage(EntryFetcher fetcher) {


I would change the interface to public void showFetchingErrorMessage(String fetcherName) since you don't need the whole fetcher object and the name is more specific for the fetchers. When creating public utility methods it's also good to have a javadoc comment describing it's functionality as it should also be used by others in the future. Other than that I'm fine with your changes 👍

…rMessages

tobiasdiez · 2016-09-26T09:56:22Z

src/main/java/net/sf/jabref/gui/importer/fetcher/IdBasedEntryFetcher.java

-            status.setStatus(e.getLocalizedMessage());
-            LOGGER.error("Error while fetching from" + fetcher.getName(), e);
+            LOGGER.error("Error while fetching from " + getTitle(), e);
+            ((ImportInspectionDialog)inspector).showErrorMessage(this.getTitle());


You completely ignore the exception details when you call showErrorMessage, thus the user gets just a standard message although the FetcherException contains all the necessary details about what went wrong (e.getLocalizedMessage)

I don't know how much additional value the end user has from : java.net.UnknownHostException: doaj.org, but I appended the localized message to the dialog nonetheless.

tobiasdiez · 2016-09-26T09:57:29Z

src/main/java/net/sf/jabref/gui/importer/fetcher/SearchBasedEntryFetcher.java

-            status.setStatus(e.getLocalizedMessage());
-            LOGGER.error("Error while fetching from" + fetcher.getName(), e);
+            LOGGER.error("Error while fetching from " + getTitle(), e);
+            ((ImportInspectionDialog)inspector).showErrorMessage(this.getTitle());


Same as above: exception information is ignored

tobiasdiez · 2016-09-26T09:58:11Z

src/test/java/net/sf/jabref/gui/importer/fetcher/OAI2HandlerFetcherTest.java

+                Assert.assertEquals(Optional.of("Javier López Peña and Gabriel Navarro"), be.getField("author"));
+                Assert.assertEquals(Optional.of("2007"), be.getField("year"));
+            } catch (IOException | SAXException e) {
+                LOGGER.error("Test probably failed due internet problems");


Please don't catch exceptions in tests.

Moved them to the method signature.

Siedlerchr

In general looks good. Apart from the UTF8 thing, rest is minor. 👍

Siedlerchr · 2016-09-27T21:17:46Z

src/main/java/net/sf/jabref/gui/importer/fetcher/ACMPortalFetcher.java

-            LOGGER.warn("Problem with ACM Portal", e);
+            LOGGER.error("Error while fetching from " + getTitle(), e);
+            preview.showErrorMessage(this.getTitle(), e.getLocalizedMessage());
+            return false;
        }


Why is the return false moved inside the catch?

I like to return immediately in general so I forget that branch.
If someone now breaks out of the try-catch (which shouldn't be done) he is forced to deal with it later down.

Siedlerchr · 2016-09-27T21:19:15Z

src/main/java/net/sf/jabref/gui/importer/fetcher/DOAJFetcher.java

+                if (numberToFetch > MAX_PER_PAGE) {
+                    boolean numberEntered = false;
+                    do {
+                        String strCount = JOptionPane.showInputDialog(Localization.lang("References found") +": " + numberToFetch + " " +


String.Format would look cleaner here, etc, use %1s, %2s for the arguments

It would be best to bind the number to the translation, but then it has to be re-translated b/c the number isn't always at the same spot in every language.

In correspondence with @koppor I combined it to a single translation.
For the user it will change nothing.

Siedlerchr · 2016-09-27T21:21:19Z

src/main/java/net/sf/jabref/gui/importer/fetcher/INSPIREFetcher.java


-                ParserResult pr = BibtexParser.parse(reader, Globals.prefs.getImportFormatPreferences());
+        try (INSPIREBibtexFilterReader reader = new INSPIREBibtexFilterReader(
+                new InputStreamReader(inputStream, Charset.forName("UTF-8")))) {


StandardCharsets.UTF-8

Siedlerchr · 2016-09-27T21:22:42Z

src/main/java/net/sf/jabref/gui/importer/fetcher/MedlineFetcher.java


-            String searchTerm = toSearchTerm(query);
+            String cleanQuery = query.trim().replace(';', ',');
+            if (cleanQuery.matches("\\d+[,\\d+]*")) {


Could you please extract that to a Pattern, too?

Siedlerchr · 2016-09-27T21:23:16Z

src/main/java/net/sf/jabref/gui/importer/fetcher/MedlineFetcher.java

+                if (numberToFetch > MedlineFetcher.PACING) {
+                    boolean numberEntered = false;
+                    do {
+                        String strCount = JOptionPane.showInputDialog(Localization.lang("References found") + ": " + numberToFetch + " " +


Same as above, String.format

Siedlerchr · 2016-09-27T21:25:23Z

src/main/java/net/sf/jabref/gui/importer/fetcher/MedlineFetcher.java

-                    iIDialog.addEntry(entry);
+                    List<BibEntry> bibs = fetchMedline(result.ids, status);
+                    for (BibEntry entry : bibs) {
+                        dialog.addEntry(entry);


I'm just curious, does the dialog object has a method which accepts a Collection/list of BibEntries?
(Not scope of this PR to add one, but would be nice to have such a method

No, it does not.

Siedlerchr · 2016-09-27T21:26:35Z

src/main/java/net/sf/jabref/gui/importer/fetcher/SpringerFetcher.java

+                if (numberToFetch > MAX_PER_PAGE) {
+                    boolean numberEntered = false;
+                    do {
+                        String strCount = JOptionPane.showInputDialog(Localization.lang("References found") +": "+ numberToFetch + " " +


same as earlier

lenhard

I disabled my internet connection and tried out all fetcher. Overall, the error messages are streamlined now, good job!

I was able to find one remaining inconsistency. When you use medline, first a message box pops up (see screenshot). When you close that message box, the error message as for the other messages appears. That box should not be shown, but apart from that the PR is fine.

tobiasdiez · 2016-10-03T14:47:35Z

@zellerdev currently rewrites the medline fetcher #2066.
Probably the strange error message automatically disappears when the medline fetcher is ported to the new interface.
Thus I think it is ok to merge this PR (and double check before merging #2066).

matthiasgeiger · 2016-10-04T14:16:51Z

Uhm... the change in GeneralFetcher.java causes a quite weird exception:

Exception in thread "JabRef CachedThreadPool" java.lang.IndexOutOfBoundsException: Index: 3, Size: 3
    at java.util.ArrayList.rangeCheck(ArrayList.java:653)
    at java.util.ArrayList.get(ArrayList.java:429)
    at java.awt.Container.createHierarchyEvents(Container.java:1441)
    at java.awt.Container.createHierarchyEvents(Container.java:1441)
    at java.awt.Container.createHierarchyEvents(Container.java:1441)
    at java.awt.Container.createHierarchyEvents(Container.java:1441)
    at java.awt.Container.createHierarchyEvents(Container.java:1441)
    at java.awt.Dialog.conditionalShow(Dialog.java:953)
    at java.awt.Dialog.show(Dialog.java:1037)
    at java.awt.Component.show(Component.java:1673)
    at java.awt.Component.setVisible(Component.java:1625)
    at java.awt.Window.setVisible(Window.java:1014)
    at java.awt.Dialog.setVisible(Dialog.java:1005)
    at net.sf.jabref.gui.importer.fetcher.GeneralFetcher.lambda$actionPerformed$171(GeneralFetcher.java:220)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

Occurs on (successful) searches with an internet connection in my branch for #2101

Has anyone else this problem?

matthiasgeiger · 2016-10-04T14:40:42Z

... and there is another problem caused by the same change:

The whole UI freezes after some consecutive searches, e.g., using the IEEE fetcher.

@bartsch-dev Can you investigate this issues?

chriba · 2016-10-04T17:01:42Z

I'll look into it tomorrow evening.

matthiasgeiger · 2016-10-05T11:53:13Z

To get the fetchers back working I just reverted your change in GeneralFetcher.java (see 6daa5b5)

Why have you changed it in your PR?

chriba · 2016-10-05T16:14:15Z

To not show the fetcher dialog if the fetching was not successful.
Now fetcher dialog opens, the error message is on top of that and closes when the error message gets closed.

* upstream/master: (102 commits) Removed unused test code (#2140) Fix main table bug when creating a duplicate (#2135) Remove explicit author and add SPDX-License-Identifier Remove GPL from README.md and CONTRIBUTING.md fix preview update (#2125) Remove some UnicodeToLatex uses (#2132) Fix mixup in french/farsi localization FetcherException should extend JabRefException Fix exception when opening preference dialog (#2127) Unify ParserException and ParseException (#2124) Small refactoring in Importer package (#2053) Implement Datepicker "none"-button (#2122) revert change from 816d30c Change title/tooltip of source panel in biblatex mode (#2120) Refactoring: completey typed metadata and add detailed travis output (#2112) RTFchars fix (#2097) Fix NPE in Medline fetcher on missing ISSN (#2113) Ctrl-s parsing error message (#2114) Fix bad web search error messages (#2034) parse error freeze (#2106) ... # Conflicts: # src/main/java/net/sf/jabref/collab/FileUpdateMonitor.java # src/main/java/net/sf/jabref/gui/externalfiles/DownloadExternalFile.java # src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java # src/main/java/net/sf/jabref/gui/externalfiles/MoveFileAction.java # src/main/java/net/sf/jabref/logic/cleanup/RenamePdfCleanup.java # src/main/java/net/sf/jabref/logic/exporter/FileSaveSession.java # src/main/java/net/sf/jabref/logic/util/io/FileUtil.java # src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

* fix bad web search error messages * add changelog * fix localization * fix issues * add JavaDoc, change parameter type of showErrorMessage * show sax error to user * print localized message * remove unused variable * reformat * fix issues

chriba added 3 commits September 22, 2016 15:25

fix bad web search error messages

51279c0

add changelog

705750d

fix localization

269fda5

boceckts added stupro labels Sep 22, 2016

boceckts reviewed Sep 23, 2016

View reviewed changes

boceckts removed the stupro-ready-for-internal-review label Sep 23, 2016

fix issues

1382737

boceckts reviewed Sep 23, 2016

View reviewed changes

chriba added 4 commits September 24, 2016 19:17

add JavaDoc, change parameter type of showErrorMessage

0d4a8f6

Merge remote-tracking branch 'jabref/master' into fixBadWebSearchErro…

a623dae

…rMessages

show sax error to user

80d067f

Merge remote-tracking branch 'jabref/master' into fixBadWebSearchErro…

6a68413

…rMessages

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 26, 2016

boceckts changed the title ~~[WIP] Fix bad web search error messages~~ Fix bad web search error messages Sep 26, 2016

tobiasdiez reviewed Sep 26, 2016

View reviewed changes

chriba added 3 commits September 26, 2016 12:35

print localized message

8d5e2af

remove unused variable

f62c48f

reformat

0018fad

Siedlerchr requested changes Sep 27, 2016

View reviewed changes

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

fix issues

87eced9

Siedlerchr approved these changes Sep 28, 2016

View reviewed changes

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 29, 2016

lenhard suggested changes Oct 3, 2016

View reviewed changes

tobiasdiez merged commit 816d30c into JabRef:master Oct 3, 2016

tobiasdiez mentioned this pull request Oct 3, 2016

Medline Fetcher using new infrastructure #2066

Merged

2 tasks

Siedlerchr mentioned this pull request Oct 4, 2016

Medline search throws Exception and hangs (Jabref 3.6) #2110

Closed

chriba deleted the fixBadWebSearchErrorMessages branch October 4, 2016 18:13

Fix bad web search error messages #2034

Fix bad web search error messages #2034

Conversation

chriba commented Sep 22, 2016

boceckts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boceckts Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boceckts Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard left a comment

Choose a reason for hiding this comment

tobiasdiez commented Oct 3, 2016

matthiasgeiger commented Oct 4, 2016

matthiasgeiger commented Oct 4, 2016 • edited Loading

chriba commented Oct 4, 2016

matthiasgeiger commented Oct 5, 2016

chriba commented Oct 5, 2016

boceckts Sep 23, 2016 •

edited

Loading

boceckts Sep 23, 2016 •

edited

Loading

Siedlerchr Sep 27, 2016 •

edited

Loading

matthiasgeiger commented Oct 4, 2016 •

edited

Loading