-
-
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
Generate an entry from ID #8129
Conversation
Co-authored-by: Oliver Kopp <[email protected]>
…to generateEntryFromId merge changes in changelog
merge jabref main commits
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.
Thanks for your work. Some suggestions from my side about the general code structure / flow.
src/main/java/org/jabref/logic/importer/CompositeIdFetcher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/importer/CompositeIdFetcher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/importer/GenerateEntryFromIdAction.java
Outdated
Show resolved
Hide resolved
This reverts commit 871e413. revert to trying fetchers because of issue with parser methods
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.
Some small code comments
src/main/java/org/jabref/gui/importer/GenerateEntryFromIdAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/importer/GenerateEntryFromIdAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/importer/CompositeIdFetcher.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jabref/logic/importer/fetcher/CompositeIdFetcherTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jabref/model/entry/identifier/IacrEprintTest.java
Outdated
Show resolved
Hide resolved
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 like that feature very much. Thank you for adressing the remaining issues. Looks good to me!
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.
Small nitpick comments remain 😇
src/main/java/org/jabref/gui/importer/GenerateEntryFromIdAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/importer/GenerateEntryFromIdAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/importer/CompositeIdFetcher.java
Outdated
Show resolved
Hide resolved
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.
Since a few hours, I am working with the feature, and it works great. One micro thing, then it is good to go from my side
src/main/java/org/jabref/logic/importer/CompositeIdFetcher.java
Outdated
Show resolved
Hide resolved
In case an entry already exists ... ... and error is thrown Is it possible to display "Entry already exists"? |
When an error occurs, the error should be included in the error message somehow. Maybe reuse the https://doi.org/10.1109/VISSOFT52517.2021.00013 always leads to the error popup. OK, it is an invalid DOI. Maybe, the error message can be routed to the user? |
I've included the exception messages in the update-messages of the background-task, now it gives more specific feedback. |
src/test/java/org/jabref/logic/importer/fetcher/CompositeIdFetcherTest.java
Outdated
Show resolved
Hide resolved
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.
l10n consistency test
org.opentest4j.AssertionFailedError: Obsolete keys found in language properties file:
Entry could not be created
- CHECK IF THE KEY IS REALLY NOT USED ANYMORE
- REMOVE THESE FROM THE ENGLISH LANGUAGE FILE
==> expected: <[]> but was: <[Entry could not be created]>
Yeah! This looks good now 🥇 Thanks for the patience and I hope we didn't annoy you with our nitpicking :) |
Fixes #4183
Adds new code segment in JabrefFrame to handle a new popover.
Adds multiple new files:
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)