-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Refactor of DOI import failure dialog, import format reader and clipboard manager #8839
Changes from 10 commits
02481fa
07345ea
f786412
f06021a
2348a34
d32eae2
987d5df
a6ec469
b3ac167
6bf38c4
58361cb
8e5b82a
d406d5a
38f3e52
6b7dfa9
2561d32
a02f5d4
5dfdc8e
3c0ee30
9ed8131
890e623
c6deb0c
4b0aeb2
a03e25f
32fd370
a9c4589
0165d14
73a88b7
5060e34
b8a83ac
4c0ebd1
0cc4826
369310e
d26e288
c47ea2d
be99801
7c609f3
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 |
---|---|---|
|
@@ -76,7 +76,17 @@ public void fetchAndMerge(BibEntry entry, List<Field> fields) { | |
}) | ||
.onFailure(exception -> { | ||
LOGGER.error("Error while fetching bibliographic information", exception); | ||
dialogService.showErrorDialogAndWait(exception); | ||
String localMessage = exception.getLocalizedMessage(); | ||
// client error | ||
if (localMessage.startsWith("Client")) { | ||
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. This is the check, I am talking about. Personally, I think this should not be a string comparison, but a different, more robust check. I assume, that @Siedlerchr had something similarly in mind with his remark. |
||
dialogService.showInformationDialogAndWait(Localization.lang("Lookup DOI"), Localization.lang("No DOI data was found")); | ||
// server error | ||
} else if (localMessage.startsWith("Server")) { | ||
dialogService.showInformationDialogAndWait(Localization.lang("Lookup DOI"), Localization.lang("DOI server not available")); | ||
// default error | ||
} else { | ||
dialogService.showErrorDialogAndWait(exception); | ||
} | ||
}) | ||
.executeWith(Globals.TASK_EXECUTOR); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package org.jabref.logic.importer.fetcher; | ||
|
||
import java.io.FileNotFoundException; | ||
import java.io.IOException; | ||
import java.net.URL; | ||
import java.util.Collections; | ||
|
@@ -72,7 +73,6 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc | |
return new Medra().performSearchById(identifier); | ||
} | ||
URL doiURL = new URL(doi.get().getURIAsASCIIString()); | ||
|
||
// BibTeX data | ||
URLDownload download = getUrlDownload(doiURL); | ||
download.addHeader("Accept", MediaTypes.APPLICATION_BIBTEX); | ||
|
@@ -81,7 +81,12 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc | |
bibtexString = download.asString(); | ||
} catch (IOException e) { | ||
// an IOException will be thrown if download is unable to download from the doiURL | ||
throw new FetcherException(Localization.lang("No DOI data exists"), e); | ||
// dispatch the IOException | ||
if (e instanceof FileNotFoundException) { | ||
throw new FetcherException(Localization.lang("Client Error"), e); | ||
} else { | ||
Siedlerchr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw new FetcherException(Localization.lang("Server Error"), e); | ||
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. Does it make sense to give those exceptions more meaningful types? Client and Server error seem a bit too arbitrary to me. Also, are exceptions usually localized? 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. I would introduce new exceptions 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. That would be great! If it is finished, could you inform me? |
||
} | ||
} | ||
|
||
// BibTeX entry | ||
|
@@ -109,6 +114,17 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc | |
} | ||
} | ||
|
||
/** | ||
* Returns client error or server error according to the message | ||
* | ||
* @param exception the IOException | ||
*/ | ||
private String dispatchIOException(IOException exception) { | ||
String classification = "Client Error"; | ||
|
||
return classification; | ||
} | ||
|
||
private void doPostCleanup(BibEntry entry) { | ||
new FieldFormatterCleanup(StandardField.PAGES, new NormalizePagesFormatter()).cleanup(entry); | ||
new FieldFormatterCleanup(StandardField.URL, new ClearFormatter()).cleanup(entry); | ||
|
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.
Why do you use localized messge for checking here? wouldn't that faill in different languages?
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.
I think I need to change the dialog title and content according to the localized message.
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.
But as far as I can see, you only rely on this variable for the check logic. The value is never actually used for the dialog content.