Issue 13234 automatic lookup of DOI at citation relations #13539
Conversation
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
| Button refreshButton, | ||
| CitationFetcher.SearchType searchType, | ||
| Button importButton, | ||
| ProgressIndicator progress) { |
There was a problem hiding this comment.
UI component (ProgressIndicator) should not be directly included in a data structure. This violates separation of concerns and should be managed in a dedicated UI controller.
There was a problem hiding this comment.
This is simple parameter object - instead passing 7 parameters, use dedicated object.
| ObservableList<CitationRelationItem> observableList = FXCollections.observableArrayList(); | ||
| citationComponents.listView().setItems(observableList); | ||
|
|
||
| // TODO: It should not be possible to cancel a search task that is already running for same tab |
There was a problem hiding this comment.
Comment does not add new information and is just stating what should be done. TODO comments should be tracked in issue tracking system instead of code.
There was a problem hiding this comment.
All TODO: comments were in this class before refactoring - I'm happy to add all 3 to issue tracking system but I think I don't have sufficient priviledges.
There was a problem hiding this comment.
Why did you move things around? Makes things hard to review.
There was a problem hiding this comment.
I grouped methods that collaborate. It was slightly difficult for me to navigate this class in the previous form.
| @@ -2791,7 +2791,8 @@ Miscellaneous=Diversos | |||
| File-related=Arquivo relacionado | |||
| Add\ selected\ entry(s)\ to\ library=Adicionar as referências selecionadas à biblioteca | |||
There was a problem hiding this comment.
You have to go through crowdin web site to update translations....
There was a problem hiding this comment.
OK, let me have a look.
There was a problem hiding this comment.
When the PR is merged, the new translations will be synced with crowdin and you can translat them there https://crowdin.com/project/jabref
| hideNodes(citationComponents.abortButton(), citationComponents.progress(), citationComponents.importButton()); | ||
| showNodes(citationComponents.refreshButton()); | ||
| task.cancel(); | ||
| dialogService.notify(Localization.lang("Search aborted!")); |
There was a problem hiding this comment.
Use of exclamation mark in UI text notification violates the guideline for avoiding exclamation marks at sentence endings in UI messages.
There was a problem hiding this comment.
This message was here before I refactored this class. Looks like a valid notification to me.
There was a problem hiding this comment.
You could change the localization string to Search aborted..
|
@koppor @Siedlerchr This PR is ready for a review. Please have a look. |
There was a problem hiding this comment.
Minor comments
I need to start RefactoringMiner to be able really review the moved around methods.
|
|
||
| HBox hBox = new HBox(); | ||
| Label label = new Label(Localization.lang("The selected entry doesn't have a DOI linked to it.")); | ||
| Hyperlink link = new Hyperlink(Localization.lang("Look Up a DOI and try again.")); |
There was a problem hiding this comment.
We use sentence case in JabRef. Change Up to up. -- Also at the lines below.
| ObservableList<CitationRelationItem> observableList = FXCollections.observableArrayList(); | ||
| citationComponents.listView().setItems(observableList); | ||
|
|
||
| // TODO: It should not be possible to cancel a search task that is already running for same tab |
There was a problem hiding this comment.
Why did you move things around? Makes things hard to review.
|
@trag-bot didn't find any issues in the code! ✅✨ |
ankamde
changed the title
|
@koppor I've addressed all of the comments. Please have a look. |
Closes #13234
In the "Citation relations", if there is no DOI, offer a "link" to determine the DOI.
Steps to test
@Article{,
author = {Oliver Kopp and Carl Christian Snethlage and Christoph Schwentker},
journal = {TUGboat},
title = {JabRef: BibTeX-based literature management software},
year = {2023},
issn = {0896-3207},
number = {3},
pages = {441--447},
volume = {44},
issue = {138},
ranking = {rank4},
}
Negative tests
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)