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

Refactor of DOI import failure dialog, import format reader and clipboard manager #8839

Merged
merged 37 commits into from
Aug 6, 2022

Conversation

zkl-ai
Copy link
Contributor

@zkl-ai zkl-ai commented May 22, 2022

Fixes #8743

The modified dialog is as below.
image

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@ThiloteE
Copy link
Member

Will details of the "error" message show under help > event log?

@zkl-ai
Copy link
Contributor Author

zkl-ai commented May 22, 2022

Will details of the "error" message show under help > event log?

There are details in the help -> event log

image

@ThiloteE
Copy link
Member

Will details of the "error" message show under help > event log?

There are details in the help -> event log

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? :)

@zkl-ai zkl-ai marked this pull request as draft May 22, 2022 16:13
@@ -76,7 +76,7 @@ public void fetchAndMerge(BibEntry entry, List<Field> fields) {
})
.onFailure(exception -> {
LOGGER.error("Error while fetching bibliographic information", exception);
String localMessage = exception.getCause().getLocalizedMessage();
String localMessage = exception.getLocalizedMessage();
Copy link
Member

@Siedlerchr Siedlerchr May 22, 2022

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@claell
Copy link
Contributor

claell commented May 22, 2022

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")) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Comment on lines 86 to 88
throw new FetcherException(Localization.lang("Client Error"), e);
} else {
throw new FetcherException(Localization.lang("Server Error"), e);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

I would introduce new exceptions FetcherClientException extends FetcherException and FetcherServerException extends 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.

That would be great! If it is finished, could you inform me?

Copy link
Contributor Author

@zkl-ai zkl-ai left a comment

Choose a reason for hiding this comment

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

final implementation

@zkl-ai zkl-ai requested a review from ThiloteE May 24, 2022 04:33
@zkl-ai zkl-ai marked this pull request as ready for review May 25, 2022 01:53
@ThiloteE ThiloteE changed the title Improve DOI import failure dialog. Do not show exception in the gui Refactor DOI import failure dialog and clipboard manager Jul 16, 2022
@ThiloteE ThiloteE changed the title Refactor DOI import failure dialog and clipboard manager Refactors DOI import failure dialog, import format reader and clipboard manager Jul 16, 2022
@ThiloteE ThiloteE changed the title Refactors DOI import failure dialog, import format reader and clipboard manager Refactor of DOI import failure dialog, import format reader and clipboard manager Jul 16, 2022
@ThiloteE
Copy link
Member

I tested again current version of this pr by entering the non-existing doi: 10.1109/REW.2016.24 into "Import by DOI"
Result:
grafik

This is unexpected, because I know the connection is good, because I can successfully import the DOI 10.1109/rew.2016.039

Worse:

copy&paste 10.1109/REW.2016.24 into the DOI field and pressing the button "Get bibliographic data from DOI" resulted in following exception:

org.jabref.logic.importer.FetcherException: Connection error
	at [email protected]/org.jabref.logic.importer.fetcher.DoiFetcher.performSearchById(DoiFetcher.java:106)
	at [email protected]/org.jabref.gui.mergeentries.FetchAndMergeEntry.lambda$fetchAndMerge$0(FetchAndMergeEntry.java:68)
	at [email protected]/org.jabref.gui.util.BackgroundTask$1.call(BackgroundTask.java:60)
	at [email protected]/org.jabref.gui.util.DefaultTaskExecutor$1.call(DefaultTaskExecutor.java:161)
	at [email protected]/javafx.concurrent.Task$TaskCallable.call(Task.java:1426)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.io.FileNotFoundException: https://doi.org/10.1109/REW.2016.24
	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:67)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:483)
	at java.base/sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:2048)
	at java.base/sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:2043)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:569)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getChainedException(HttpURLConnection.java:2042)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1609)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1589)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:224)
	at [email protected]/org.jabref.logic.net.URLDownload.asString(URLDownload.java:241)
	at [email protected]/org.jabref.logic.net.URLDownload.asString(URLDownload.java:254)
	at [email protected]/org.jabref.logic.importer.fetcher.DoiFetcher.performSearchById(DoiFetcher.java:80)
	... 10 more
Caused by: java.io.FileNotFoundException: https://doi.org/10.1109/REW.2016.24
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1993)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1589)
	at java.base/java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:529)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsURLConnectionImpl.java:308)
	at [email protected]/org.jabref.logic.net.URLDownload.openConnection(URLDownload.java:354)
	... 13 more

Again: I know connection to the DOI server is not the problem. The problem is that this DOI cannot be found (probably because it does not exist, but there also could be a typo by the user or it simply is not in the system serverside)

@Siedlerchr
Copy link
Member

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)
@ThiloteE

This comment was marked as outdated.

@ThiloteE
Copy link
Member

ThiloteE commented Jul 18, 2022

Optional notification messages:

  • Bibliographic data not found. Cause is likely the client side. Please check connection and identifier for correctness
  • Bibliographic data not found. Cause is likely the client side. Please check connection and input.
  • Bibliographic data not found. Cause is likely the client side: Http error code XXX.
  • Bibliographic data not found. Please check the connection and if the identifier is correct.
  • Bibliographic data not found. Please check user input and connection.
  • Bibliographic data not found. Please check correctness of identifier and connection. The server responded with HTTP status code XXX, which means, the error is likely caused at the client side.

@Siedlerchr
Copy link
Member

grafik

same also now for the new entry types dialog

…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
@ThiloteE
Copy link
Member

ThiloteE commented Jul 21, 2022

Meh. Found another ugly error message with current build:

How to reproduce:

  1. Turn off your internet connection
  2. Go to General Tab of entry editor. Enter blabla into the DOI field.
  3. Press Search DOI button at the right side.

Result:

Following error message:

org.jabref.logic.importer.FetcherException: An I/O exception occurred
	at [email protected]/org.jabref.logic.importer.IdParserFetcher.findIdentifier(IdParserFetcher.java:95)
	at [email protected]/org.jabref.gui.fieldeditors.IdentifierEditorViewModel.lambda$lookupIdentifier$5(IdentifierEditorViewModel.java:119)
	at [email protected]/org.jabref.gui.util.BackgroundTask$1.call(BackgroundTask.java:60)
	at [email protected]/org.jabref.gui.util.DefaultTaskExecutor$1.call(DefaultTaskExecutor.java:161)
	at [email protected]/javafx.concurrent.Task$TaskCallable.call(Task.java:1426)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.net.SocketException: Das Netzwerk ist nicht erreichbar
	at java.base/sun.nio.ch.Net.connect0(Native Method)
	at java.base/sun.nio.ch.Net.connect(Net.java:579)
	at java.base/sun.nio.ch.Net.connect(Net.java:568)
	at java.base/sun.nio.ch.NioSocketImpl.connect(NioSocketImpl.java:585)
	at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327)
	at java.base/java.net.Socket.connect(Socket.java:633)
	at java.base/sun.security.ssl.SSLSocketImpl.connect(SSLSocketImpl.java:299)
	at java.base/sun.security.ssl.BaseSSLSocketImpl.connect(BaseSSLSocketImpl.java:174)
	at java.base/sun.net.NetworkClient.doConnect(NetworkClient.java:183)
	at java.base/sun.net.www.http.HttpClient.openServer(HttpClient.java:498)
	at java.base/sun.net.www.http.HttpClient.openServer(HttpClient.java:603)
	at java.base/sun.net.www.protocol.https.HttpsClient.<init>(HttpsClient.java:264)
	at java.base/sun.net.www.protocol.https.HttpsClient.New(HttpsClient.java:378)
	at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.getNewHttpClient(AbstractDelegateHttpsURLConnection.java:189)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1242)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:1128)
	at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:175)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1665)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1589)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:224)
	at java.base/java.net.URL.openStream(URL.java:1161)
	at [email protected]/org.jabref.logic.importer.IdParserFetcher.findIdentifier(IdParserFetcher.java:74)
	... 10 more

Expected result:

Nice looking notification. E.g Something like this:

Network not available. (i think this one should already suffice 👍 )

Alternatives:
Network not available. Please check your connection
Network not available. Please check your (internet-) connection
DOI not found, because the network is not available. Please check your connection

@ThiloteE
Copy link
Member

ThiloteE commented Jul 21, 2022

Similarly:

How to reproduce:

  1. Turn off your internet connection
  2. Go to General Tab of entry editor. Enter blabla into the DOI field.
  3. Press Get bibliographic data from DOI button at the right side.

Result:

Following error message:

org.jabref.logic.importer.FetcherException: Connection error
	at [email protected]/org.jabref.logic.importer.fetcher.DoiFetcher.performSearchById(DoiFetcher.java:106)
	at [email protected]/org.jabref.gui.mergeentries.FetchAndMergeEntry.lambda$fetchAndMerge$0(FetchAndMergeEntry.java:68)
	at [email protected]/org.jabref.gui.util.BackgroundTask$1.call(BackgroundTask.java:60)
	at [email protected]/org.jabref.gui.util.DefaultTaskExecutor$1.call(DefaultTaskExecutor.java:161)
	at [email protected]/javafx.concurrent.Task$TaskCallable.call(Task.java:1426)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.net.UnknownHostException: doi.org
	at java.base/sun.nio.ch.NioSocketImpl.connect(NioSocketImpl.java:564)
	at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327)
	at java.base/java.net.Socket.connect(Socket.java:633)
	at java.base/sun.security.ssl.SSLSocketImpl.connect(SSLSocketImpl.java:299)
	at java.base/sun.net.NetworkClient.doConnect(NetworkClient.java:178)
	at java.base/sun.net.www.http.HttpClient.openServer(HttpClient.java:498)
	at java.base/sun.net.www.http.HttpClient.openServer(HttpClient.java:603)
	at java.base/sun.net.www.protocol.https.HttpsClient.<init>(HttpsClient.java:264)
	at java.base/sun.net.www.protocol.https.HttpsClient.New(HttpsClient.java:378)
	at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.getNewHttpClient(AbstractDelegateHttpsURLConnection.java:189)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1242)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:1128)
	at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:175)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1665)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1589)
	at java.base/java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:529)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsURLConnectionImpl.java:308)
	at [email protected]/org.jabref.logic.net.URLDownload.openConnection(URLDownload.java:354)
	at [email protected]/org.jabref.logic.net.URLDownload.asString(URLDownload.java:241)
	at [email protected]/org.jabref.logic.net.URLDownload.asString(URLDownload.java:254)
	at [email protected]/org.jabref.logic.importer.fetcher.DoiFetcher.getAgency(DoiFetcher.java:138)
	at [email protected]/org.jabref.logic.importer.fetcher.DoiFetcher.performSearchById(DoiFetcher.java:71)

Expectations are roughly similar as described in last comment above, but of course adapted to include "Bibliographic data not found".

@Siedlerchr
Copy link
Member

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)
  ...
@calixtus calixtus merged commit 16de272 into JabRef:main Aug 6, 2022
Siedlerchr added a commit that referenced this pull request Aug 6, 2022
…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)
  ...
Siedlerchr added a commit to JabRef/jabref-koppor that referenced this pull request Aug 14, 2022
* 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
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetcher import Logging status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: performance ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve DOI import failure dialog. Do not show exception in the gui
6 participants