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

Hide redundant information on fetcher exception #6569

Closed
wants to merge 7 commits into from

Conversation

newtypes9
Copy link
Contributor

@newtypes9 newtypes9 commented May 31, 2020

I change the dialogService::showErrorDialogAndWait to custom exception handler exceptionHandler. The previous one will print all the stack trace information in dialog box. The new one will print the reason and hide the redundant and unnecessary information of exception.
trying to fix #6376
screenshot2

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Thank you for your enhancement, @shiqingliu . Most of it looks good to me, just a question about the if expression.


ImportEntriesDialog dialog = new ImportEntriesDialog(frame.getCurrentBasePanel().getBibDatabaseContext(), task);
dialog.setTitle(activeFetcher.getName());
dialog.showAndWait();
}

private void exceptionHandler(Exception exception) {
if (exception.getClass().getName().equals("org.jabref.logic.importer.FetcherException")) {
Copy link
Member

Choose a reason for hiding this comment

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

How about exception instanceof FetcherException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great!

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Hi @shiqingliu , there is still one import error. It would be great, if you vould fix the import order.
Codewise looks good to me

@@ -19,6 +19,7 @@
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.importer.SearchBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
import org.jabref.logic.importer.FetcherException;
Copy link
Member

Choose a reason for hiding this comment

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

This should be reordered in alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Thank your for your review.

@newtypes9 newtypes9 requested a review from calixtus June 3, 2020 16:15
@Siedlerchr Siedlerchr changed the title fixes #6376 Hide redundant information on fetcher exception Jun 3, 2020
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Codewise looks good to me. Question is now, does it satisfy @MootezSaaD and @ilippert ? Let's ask them for a review. 😉


private void exceptionHandler(Exception exception) {
if (exception instanceof FetcherException) {
if (exception.getMessage().equals("A network error occurred")) {
Copy link
Member

Choose a reason for hiding this comment

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

Of course, please add the strings to the english language resource file.

@MootezSaaD
Copy link
Contributor

MootezSaaD commented Jun 3, 2020

Thanks @shiqingliu ! , there's also another one related to the Springer fetcher if you are interested :)

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

I agree the stacktrace is not very user friendly, and we should improve the error messages. However, the best place for this is when the exceptions are created, e.g. here:

// TODO: Catch HTTP Response 401/403 errors and report that user has no rights to access resource
throw new FetcherException("A network error occurred", e);

There you have the necessary context to construct a useful exception message by e.g. appending the URL that is incorrect and the reason for the failed request. See
https://stackoverflow.com/questions/37302615/extract-http-status-code-from-java-io-ioexception

Please improve the code in this regard. Thanks!

}
} else {
dialogService.showWarningDialogAndWait(Localization.lang("An error occurred"),
Localization.lang(exception.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

Localization.lang only works for static messages so sadly cannot be used here.

Copy link
Member

Choose a reason for hiding this comment

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

@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label Jun 5, 2020
@koppor
Copy link
Member

koppor commented Jul 7, 2020

This is a good start. Since there was no activity since a few weeks, we would like to ask whether there is some interest in continuing in this.

Ideas:

  • Distinguish between known and unknown errors (e.g., no rights vs. something weird happened)
  • Show details somewhere. Similar to <details> at GitHub.

@ilippert
Copy link
Contributor

ilippert commented Jul 7, 2020

Ideas:

* Distinguish between known and unknown errors (e.g., no rights vs. something weird happened)

* Show details somewhere. Similar to `<details>` at GitHub.

I like both ideas very much. My original idea at #6343 was that in case jabref can know what's wrong, tell the user what the error is.
IF this is an error caused by an outside element (blame it on google...), rather than giving the user the feeling that jabref does not work. if possible, then, a message like "[Source] does not allow you to retrieve information from their server" (where [Source] might be google, etc).

My proposal is to think this from the perspective of the user: i try to retrieve information. i get an error. now, is this my error, because I mishandled jabref?, is this an error caused by jabref? or is everything all right, except for the provider (say google) who does not allow me an answer to my request.

@calixtus
Copy link
Member

Decision in Devcall: We think a more appropriate solution would be to throw exceptions in every single fetcher instead of a generic one.

@calixtus calixtus closed this Aug 18, 2020
koppor pushed a commit that referenced this pull request Jun 15, 2023
4b4e8f442d Create silva-fennica.csl (#6568)
dd8760bb2b Update american-journal-of-botany.csl (#6569)
8b0e505363 Update haute-ecole-de-gestion-de-geneve-iso-690.csl (#6560)
016050c4b7 Update geographie-et-cultures.csl (#6563)
e8b62f1c80 Update and rename dependent/retina.csl to retina.csl (#6565)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 4b4e8f442d2de454c638393fbb9dd911c8b7aca7
koppor pushed a commit that referenced this pull request Jul 1, 2023
80b3861bce Update al-jamiah-journal-of-islamic-studies.csl (#6581)
b6fb00e415 Create arachnologische-mitteilungen.csl (#6375)
2dbc8edf8e Update universitat-basel-iberoromanistik.csl (#6580)
fd230a7073 Create veterinary-clinical-pathology.csl (#6372)
ac0afa3cae Create dedicated Basque APA file (#6370)
fee0677a88 Update chicago-author-date.csl (#6289)
ca1bf2db6e Create van-yuzuncu-yil-universitesi-fen-bilimleri-enstitusu.csl (#6230)
f4116db325 Create Turcica.csl (#6240)
0e4311a802 Create jurnal-teknik-mesin-indonesia.csl (#6211)
e73bf46674 Update vancouver.csl (#6156)
b73c3ef193 Create independent TF AIP Style
befa82e7ef Create harvard-xi-an-jiaotong-liverpool-univeisity.csl (#6181)
048c9bddbc Add csl for SDMI (#6129)
1c2aedd088 Update and rename dependent/energy-research-and-social-science.csl to energy-research-and-social-science.csl (#6567)
b77084255f Update the-quarterly-journal-of-economics.csl (#6572)
cf66f60f25 Update publicatiewijzer-voor-de-archeologie.csl (#6577)
9e9e08c219 Update isara-iso-690.csl (#6578)
4b4e8f442d Create silva-fennica.csl (#6568)
dd8760bb2b Update american-journal-of-botany.csl (#6569)
8b0e505363 Update haute-ecole-de-gestion-de-geneve-iso-690.csl (#6560)
016050c4b7 Update geographie-et-cultures.csl (#6563)
e8b62f1c80 Update and rename dependent/retina.csl to retina.csl (#6565)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 80b3861bce121a64d43ef167581f8d100a4f70aa
calixtus pushed a commit that referenced this pull request Jul 1, 2023
…ce (#10048)

80b3861bce Update al-jamiah-journal-of-islamic-studies.csl (#6581)
b6fb00e415 Create arachnologische-mitteilungen.csl (#6375)
2dbc8edf8e Update universitat-basel-iberoromanistik.csl (#6580)
fd230a7073 Create veterinary-clinical-pathology.csl (#6372)
ac0afa3cae Create dedicated Basque APA file (#6370)
fee0677a88 Update chicago-author-date.csl (#6289)
ca1bf2db6e Create van-yuzuncu-yil-universitesi-fen-bilimleri-enstitusu.csl (#6230)
f4116db325 Create Turcica.csl (#6240)
0e4311a802 Create jurnal-teknik-mesin-indonesia.csl (#6211)
e73bf46674 Update vancouver.csl (#6156)
b73c3ef193 Create independent TF AIP Style
befa82e7ef Create harvard-xi-an-jiaotong-liverpool-univeisity.csl (#6181)
048c9bddbc Add csl for SDMI (#6129)
1c2aedd088 Update and rename dependent/energy-research-and-social-science.csl to energy-research-and-social-science.csl (#6567)
b77084255f Update the-quarterly-journal-of-economics.csl (#6572)
cf66f60f25 Update publicatiewijzer-voor-de-archeologie.csl (#6577)
9e9e08c219 Update isara-iso-690.csl (#6578)
4b4e8f442d Create silva-fennica.csl (#6568)
dd8760bb2b Update american-journal-of-botany.csl (#6569)
8b0e505363 Update haute-ecole-de-gestion-de-geneve-iso-690.csl (#6560)
016050c4b7 Update geographie-et-cultures.csl (#6563)
e8b62f1c80 Update and rename dependent/retina.csl to retina.csl (#6565)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 80b3861bce121a64d43ef167581f8d100a4f70aa

Co-authored-by: github actions <jabrefmail+webfeedback@gmail.com>
koppor pushed a commit that referenced this pull request Jul 15, 2023
a97dbb32c5 Update studii-teologice.csl (#6591)
e19e08780e Update acm-sig-proceedings-long-author-list.csl (#6594)
c8abbcdd88 Update acm-sig-proceedings.csl (#6595)
725ace4a40 Create uppsala-university-library-harvard.csl (#6574)
a973041e0e update bluebook-law-review.csl (#6583)
0891cfc49a Update masarykova-univerzita-pravnicka-fakulta.csl (#6589)
80b3861bce Update al-jamiah-journal-of-islamic-studies.csl (#6581)
b6fb00e415 Create arachnologische-mitteilungen.csl (#6375)
2dbc8edf8e Update universitat-basel-iberoromanistik.csl (#6580)
fd230a7073 Create veterinary-clinical-pathology.csl (#6372)
ac0afa3cae Create dedicated Basque APA file (#6370)
fee0677a88 Update chicago-author-date.csl (#6289)
ca1bf2db6e Create van-yuzuncu-yil-universitesi-fen-bilimleri-enstitusu.csl (#6230)
f4116db325 Create Turcica.csl (#6240)
0e4311a802 Create jurnal-teknik-mesin-indonesia.csl (#6211)
e73bf46674 Update vancouver.csl (#6156)
b73c3ef193 Create independent TF AIP Style
befa82e7ef Create harvard-xi-an-jiaotong-liverpool-univeisity.csl (#6181)
048c9bddbc Add csl for SDMI (#6129)
1c2aedd088 Update and rename dependent/energy-research-and-social-science.csl to energy-research-and-social-science.csl (#6567)
b77084255f Update the-quarterly-journal-of-economics.csl (#6572)
cf66f60f25 Update publicatiewijzer-voor-de-archeologie.csl (#6577)
9e9e08c219 Update isara-iso-690.csl (#6578)
4b4e8f442d Create silva-fennica.csl (#6568)
dd8760bb2b Update american-journal-of-botany.csl (#6569)
8b0e505363 Update haute-ecole-de-gestion-de-geneve-iso-690.csl (#6560)
016050c4b7 Update geographie-et-cultures.csl (#6563)
e8b62f1c80 Update and rename dependent/retina.csl to retina.csl (#6565)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: a97dbb32c57c8c00e47422dae4ba4f480e753da5
koppor pushed a commit that referenced this pull request Aug 1, 2023
0749a19b83 Update journal-of-experimental-botany.csl (#6604)
b1768724fe Update modern-language-association.csl (#6606)
dd364c1815 Create modern-language-association-for-JEI.csl (#6593)
6cb436997b Partial update of APA styles for 1.0.2, including software, legal, most localizations (#6270)
d7c4ebec85 fix film/video authors for american-sociological-association.csl (#6602)
a97dbb32c5 Update studii-teologice.csl (#6591)
e19e08780e Update acm-sig-proceedings-long-author-list.csl (#6594)
c8abbcdd88 Update acm-sig-proceedings.csl (#6595)
725ace4a40 Create uppsala-university-library-harvard.csl (#6574)
a973041e0e update bluebook-law-review.csl (#6583)
0891cfc49a Update masarykova-univerzita-pravnicka-fakulta.csl (#6589)
80b3861bce Update al-jamiah-journal-of-islamic-studies.csl (#6581)
b6fb00e415 Create arachnologische-mitteilungen.csl (#6375)
2dbc8edf8e Update universitat-basel-iberoromanistik.csl (#6580)
fd230a7073 Create veterinary-clinical-pathology.csl (#6372)
ac0afa3cae Create dedicated Basque APA file (#6370)
fee0677a88 Update chicago-author-date.csl (#6289)
ca1bf2db6e Create van-yuzuncu-yil-universitesi-fen-bilimleri-enstitusu.csl (#6230)
f4116db325 Create Turcica.csl (#6240)
0e4311a802 Create jurnal-teknik-mesin-indonesia.csl (#6211)
e73bf46674 Update vancouver.csl (#6156)
b73c3ef193 Create independent TF AIP Style
befa82e7ef Create harvard-xi-an-jiaotong-liverpool-univeisity.csl (#6181)
048c9bddbc Add csl for SDMI (#6129)
1c2aedd088 Update and rename dependent/energy-research-and-social-science.csl to energy-research-and-social-science.csl (#6567)
b77084255f Update the-quarterly-journal-of-economics.csl (#6572)
cf66f60f25 Update publicatiewijzer-voor-de-archeologie.csl (#6577)
9e9e08c219 Update isara-iso-690.csl (#6578)
4b4e8f442d Create silva-fennica.csl (#6568)
dd8760bb2b Update american-journal-of-botany.csl (#6569)
8b0e505363 Update haute-ecole-de-gestion-de-geneve-iso-690.csl (#6560)
016050c4b7 Update geographie-et-cultures.csl (#6563)
e8b62f1c80 Update and rename dependent/retina.csl to retina.csl (#6565)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 0749a19b8306f2e8dcb9bf1a2e3a6992666030ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete status: stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IEEE fetcher produces FetcherException when rate is exceeded
6 participants