Refactor of DOI import failure dialog, import format reader and clipboard manager#8839
Conversation
|
Will details of the "error" message show under |
Perfect! Then I guess it is fine if the "show details" button is missing :) @claell what do you say? :) I kinda hijacked your issue there haha :D Is this pr something you would like? :) |
…o "No DOI data was found"
…g' into improve-DOI-import-failure-dialog
| .onFailure(exception -> { | ||
| LOGGER.error("Error while fetching bibliographic information", exception); | ||
| String localMessage = exception.getCause().getLocalizedMessage(); | ||
| String localMessage = exception.getLocalizedMessage(); |
There was a problem hiding this comment.
Why do you use localized messge for checking here? wouldn't that faill in different languages?
There was a problem hiding this comment.
I think I need to change the dialog title and content according to the localized message.
There was a problem hiding this comment.
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.
|
From the general approach, this looks good to me and pretty much as detailed in the original issue :) Nice that it worked out that way. Regarding the implementation, I was wondering whether the logic of dealing with the exceptions can be improved (see comment on the code that I will post soon). |
| dialogService.showErrorDialogAndWait(exception); | ||
| String localMessage = exception.getLocalizedMessage(); | ||
| // client error | ||
| if (localMessage.startsWith("Client")) { |
There was a problem hiding this comment.
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.
| throw new FetcherException(Localization.lang("Client Error"), e); | ||
| } else { | ||
| throw new FetcherException(Localization.lang("Server Error"), e); |
There was a problem hiding this comment.
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.
I would introduce new exceptions FetcherClientException extends FetcherException and FetcherServerException extends FetcherException
There was a problem hiding this comment.
That would be great! If it is finished, could you inform me?
…ption and DOIServerNotAvailableException
…g' into improve-DOI-import-failure-dialog
…ption and DOIServerNotAvailableException and improved
zkl-ai
left a comment
There was a problem hiding this comment.
final implementation
Fix some fetcher tests
|
Yeah, this comes from my latest changes. The problem is that the fetcher tests expect empty entries on 404. I will come up with a better solution. |
throw exception on 404 todo fix tests
…failure-dialog * upstream/main: Update Gradle Wrapper from 7.4.2 to 7.5. (JabRef#8986) New translations JabRef_en.properties (Russian) (JabRef#8985)
This comment was marked as outdated.
This comment was marked as outdated.
|
Optional notification messages:
|
…failure-dialog * upstream/main: Reworded t in pr (JabRef#8989) Fixed checkstyle (JabRef#8990) Fix RfcFetcherTest (JabRef#8988) Fixes JabRef#521 - Allow to drag&drop selected bib entries to other library tabs (JabRef#8982) # Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.java # src/test/java/org/jabref/logic/importer/fetcher/JstorFetcherTest.java
|
Meh. Found another ugly error message with current build: How to reproduce:
Result: Following error message: Expected result: Nice looking notification. E.g Something like this:
Alternatives: |
|
Similarly: How to reproduce:
Result: Following error message: Expectations are roughly similar as described in last comment above, but of course adapted to include "Bibliographic data not found". |
|
TODO: Check exception handling in Composite fetchers |
…failure-dialog * upstream/main: (31 commits) Snapcraft and issue template Show development information\n\n+semver: minor Release v5.7 New Crowdin updates (JabRef#9030) New Crowdin updates (JabRef#9029) [Bot] Update CSL styles (JabRef#9027) Add missing translations for AutomaticFieldEditor (JabRef#9028) [Bot] Update Journal abbrev list (JabRef#9026) Rating in main table (JabRef#9023) New Crowdin updates (JabRef#9024) New Crowdin updates (JabRef#9016) New Crowdin updates (JabRef#9013) try to gather more output from LO exception (JabRef#9002) Improve Automatic Field Editor Dialog (JabRef#8973) Update BST VM to Antlr4 (JabRef#8934) Support biblatex apa citation for legal entry types (JabRef#8966) Bump junit-jupiter from 5.8.2 to 5.9.0 (JabRef#9012) Bump lucene-core from 9.2.0 to 9.3.0 (JabRef#9009) Bump checkstyle from 10.3.1 to 10.3.2 (JabRef#9006) Bump appleboy/ssh-action from 0.1.4 to 0.1.5 (JabRef#9005) ...
…rg.jsoup-jsoup-1.15.2 * upstream/main: (34 commits) Refactor of DOI import failure dialog, import format reader and clipboard manager (#8839) Snapcraft and issue template Show development information\n\n+semver: minor Release v5.7 New Crowdin updates (#9030) New Crowdin updates (#9029) [Bot] Update CSL styles (#9027) Add missing translations for AutomaticFieldEditor (#9028) [Bot] Update Journal abbrev list (#9026) Rating in main table (#9023) New Crowdin updates (#9024) New Crowdin updates (#9016) New Crowdin updates (#9013) try to gather more output from LO exception (#9002) Improve Automatic Field Editor Dialog (#8973) Update BST VM to Antlr4 (#8934) Support biblatex apa citation for legal entry types (#8966) Bump junit-jupiter from 5.8.2 to 5.9.0 (#9012) Bump lucene-core from 9.2.0 to 9.3.0 (#9009) Bump checkstyle from 10.3.1 to 10.3.2 (#9006) ...
* upstream/main: (31 commits) Citavi Importer - Import all knowledge items (JabRef#9043) Fixed table update in eft preferences (JabRef#9051) Keep EOL setting at backups (JabRef#9048) ExternalFileTypes singleton refactor (JabRef#9044) Fix dead link (JabRef#9047) Fix performance regresssion (JabRef#9045) Update javafx to 18.02 import citavi knowledge items (JabRef#9033) Fix .gitattributes for CHANGELOG.md [GSOC22] - B - Implement merging fields in the three way merge UI (JabRef#9022) [GSOC22] - A - Implement a fully functional three way merge UI (JabRef#8945) Change button label from "Return to JabRef" to "Return to library" (JabRef#9039) Bump postgresql from 42.4.0 to 42.4.1 (JabRef#9036) Bump org.javamodularity.moduleplugin from 1.8.11 to 1.8.12 (JabRef#9037) Bump slf4j-api from 2.0.0-alpha7 to 2.0.0-beta1 (JabRef#9035) Bump slf4j-api from 2.0.0-alpha7 to 2.0.0-beta1 in /buildSrc (JabRef#9038) Update Gradle Wrapper from 7.5 to 7.5.1. (JabRef#9034) Refactor of DOI import failure dialog, import format reader and clipboard manager (JabRef#8839) Snapcraft and issue template Show development information\n\n+semver: minor ...



Fixes #8743
The modified dialog is as below.

CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)