Fix CSL files are not removed properly if already exist#15463
Fix CSL files are not removed properly if already exist#15463Siedlerchr merged 10 commits intoJabRef:mainfrom
Conversation
Review Summary by QodoFix CSL style duplicate detection and immediate removal
WalkthroughsDescription• Add duplicate detection for citation styles before insertion • Refactor CSL style addition logic for improved readability • Separate validation and duplicate checking from style addition • Add user-friendly error messages for duplicate styles Diagramflowchart LR
A["User selects CSL file"] --> B["Create citation style"]
B --> C["Validate style"]
C -->|Invalid| D["Show error dialog"]
C -->|Valid| E["Check for duplicates"]
E -->|Duplicate found| F["Show info dialog"]
E -->|No duplicate| G["Add style to list"]
G --> H["Update UI and preferences"]
File Changes1. jabgui/src/main/java/org/jabref/gui/openoffice/StyleSelectDialogViewModel.java
|
Code Review by Qodo
|
github-actions
bot
added
good first issue
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| // Create a citation style | ||
| Optional<CitationStyle> citationStyleToAddOptional = CSLStyleUtils.createCitationStyleFromFile(stylePath); | ||
|
|
||
| if (newStyleOptional.isPresent()) { | ||
| CitationStyle newStyle = newStyleOptional.get(); | ||
| // Check if citation style is valid | ||
| if (citationStyleToAddOptional.isEmpty()) { | ||
| dialogService.showErrorDialogAndWait( | ||
| Localization.lang("Invalid style selected"), | ||
| Localization.lang("You must select a valid CSL style file.") | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| List<CitationStyle> allStyles = CSLStyleLoader.getStyles(); | ||
| List<CitationStylePreviewLayout> updatedLayouts = allStyles.stream() | ||
| .map(style -> new CitationStylePreviewLayout(style, bibEntryTypesManager)) | ||
| .toList(); | ||
| CitationStyle citationStyleToAdd = citationStyleToAddOptional.get(); | ||
|
|
||
| availableCslLayouts.setAll(updatedLayouts); | ||
| // Check if citation style is duplicate in the list | ||
| Optional<CitationStylePreviewLayout> existingLayout = findDuplicate(citationStyleToAdd); | ||
| if (existingLayout.isPresent()) { | ||
| dialogService.showInformationDialogAndWait( | ||
| Localization.lang("Style already available"), | ||
| Localization.lang("The selected CSL style is already available in the list.") | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| Optional<CitationStylePreviewLayout> newLayoutOptional = updatedLayouts.stream() | ||
| .filter(layout -> layout.getFilePath().equals(stylePath)) | ||
| .findFirst(); | ||
| // Citation style is good to add | ||
| cslStyleLoader.addStyle(citationStyleToAdd); | ||
|
|
||
| if (newLayoutOptional.isPresent()) { | ||
| CitationStylePreviewLayout newLayout = newLayoutOptional.get(); | ||
| selectedCslLayoutProperty.set(newLayout); | ||
| List<CitationStyle> allStyles = CSLStyleLoader.getStyles(); | ||
| List<CitationStylePreviewLayout> updatedLayouts = allStyles.stream() | ||
| .map(style -> new CitationStylePreviewLayout(style, bibEntryTypesManager)) | ||
| .toList(); | ||
|
|
||
| openOfficePreferences.setCurrentStyle(newStyle); | ||
| availableCslLayouts.setAll(updatedLayouts); | ||
|
|
||
| dialogService.showInformationDialogAndWait( | ||
| Localization.lang("Style added"), | ||
| Localization.lang("The CSL style has been added successfully.") | ||
| ); | ||
| } else { | ||
| dialogService.showErrorDialogAndWait( | ||
| Localization.lang("Style not found"), | ||
| Localization.lang("The CSL style was added but could not be found in the list.") | ||
| ); | ||
| } | ||
| Optional<CitationStylePreviewLayout> newLayoutOptional = updatedLayouts.stream() | ||
| .filter(layout -> layout.getFilePath().equals(stylePath)) | ||
| .findFirst(); | ||
|
|
||
| if (newLayoutOptional.isPresent()) { | ||
| CitationStylePreviewLayout newLayout = newLayoutOptional.get(); | ||
| selectedCslLayoutProperty.set(newLayout); | ||
|
|
||
| openOfficePreferences.setCurrentStyle(citationStyleToAdd); | ||
|
|
||
| dialogService.showInformationDialogAndWait( | ||
| Localization.lang("Style added"), | ||
| Localization.lang("The CSL style has been added successfully.") | ||
| ); | ||
| } else { | ||
| dialogService.showErrorDialogAndWait( | ||
| Localization.lang("Invalid style selected"), | ||
| Localization.lang("You must select a valid CSL style file.") | ||
| Localization.lang("Style not found"), | ||
| Localization.lang("The CSL style was added but could not be found in the list.") | ||
| ); | ||
| } |
There was a problem hiding this comment.
1. No tests for csl logic 📘 Rule violation ☼ Reliability
This PR changes CSL style import behavior (new validation/duplicate detection and a new CSLStyleLoader.addStyle API), but no corresponding tests are added/updated in the diff. This risks regressions and violates the requirement to update tests when behavior changes.
Agent Prompt
## Issue description
CSL style importing behavior was changed (duplicate detection + new loader API), but the PR does not add/update tests to prevent regressions.
## Issue Context
At minimum, tests should cover: (1) rejecting invalid CSL files, (2) detecting duplicates by title/short title, and (3) persisting newly added external styles.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/openoffice/StyleSelectDialogViewModel.java[197-250]
- jablib/src/main/java/org/jabref/logic/citationstyle/CSLStyleLoader.java[117-121]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This comment has been minimized.
This comment has been minimized.
|
Thanks for tackling this issue, maybe another point you can think about: When adding a new CSL style it would be great to select/focus it in the list. I think this is what I missed |
This comment has been minimized.
This comment has been minimized.
github-actions
bot
added
status: changes-required
✅ All tests passed ✅🏷️ Commit: ed76532 Learn more about TestLens at testlens.app. |
Thank you for the suggestion. I have updated the code. |
github-actions
bot
added
status: no-bot-comments
and removed
status: changes-required
| Optional<Path> path = dialogService.showFileOpenDialog(fileDialogConfiguration); | ||
|
|
||
| path.map(Path::toAbsolutePath).map(Path::toString).ifPresent(stylePath -> { | ||
| Optional<CitationStyle> newStyleOptional = cslStyleLoader.addStyleIfValid(stylePath); |
There was a problem hiding this comment.
Can't the duplicate checking logic be incorporated into the existing addStyleIfValid method?
There was a problem hiding this comment.
Can't the duplicate checking logic be incorporated into the existing
addStyleIfValidmethod?
Yeah I have tried it before, but then both "Invalid style" and "Duplicate style" return Optional.empty(), so viewmodel cannot distinguish between them.
I have a new workaround:
put CSLStyleUtils.createCitationStyleFromFile in viewmodel and let viewmodel check style validity. Then pass valid citation style to addStyleIfValid, which incorporats duplication check. What do you think?
There was a problem hiding this comment.
Hi, I am slightly busy till the weekend. I'll get back to this soon.
There was a problem hiding this comment.
Now that I think, the design of addStyleIfValid is a bit misleading.
It suggests a void action, but is also returning the style optional for further processing by the viewModel.
Since CSLStyleLoader and JStyleLoader (currently) don't implement a common interface, it is okay to remove that method from CSLStyleLoader and do it the way it is done in this PR now.
That being said, do check the requirements for consistency - the EXTERNAL_STYLES list (which was a private member of CSLStyleLoader) is no longer being updated on addition now, which was in turn being used in storeExternalStyles() to update the list of externally added styles in openOfficePreferences. So now the loader may no longer be "aware" of the addition of a new style. The action is symmetric in case of removal - so one way to check would be to add a valid, non-duplicate external style now, restart JabRef to see if it still persists, and try removing it. Compare the logic carefully and see why or why not it'll work.
There was a problem hiding this comment.
P.S. During the upcoming refactoring phases, we can think on how both of the style loaders can implement an interface to share these methods - as both have internal, external styles addition, removal and preference storage.
Maybe that was the idea I had when I named this method - I see an analogue in JStyleLoader which returns a boolean instead. We can later on look how duplicates are handled there.
There was a problem hiding this comment.
Thank you for the detailed comment, they are very insightful. But I have a question here:
the EXTERNAL_STYLES list (which was a private member of CSLStyleLoader) is no longer being updated on addition now
Could you please explain about this? Because in my view, EXTERNAL_STYLES.add() and storeExternalStyles() are still kept in addStyle in this PR.
I have checked the consistency following the steps above. External style can persist when JabRef restarts and can be removed properly.
During the upcoming refactoring phases, we can think on how both of the style loaders can implement an interface to share these methods
We then maybe need to refactor addStyleIfValid() and other methods in JStyleLoader as well, since they currently have different method signature.
There was a problem hiding this comment.
Could you please explain about this? Because in my view,
EXTERNAL_STYLES.add()andstoreExternalStyles()are still kept inaddStylein this PR.
Ah that was a miss from my side - when I was looking at the viewmodel changes in this PR while checking CSLStyleLoader in my IDE where I did not have this PR checked out. This should work. Thanks!
We then maybe need to refactor
addStyleIfValid()and other methods inJStyleLoaderas well, since they currently have different method signature.
Correct - we will deal with that later.
| } | ||
| } | ||
|
|
||
| private boolean findDuplicate(CitationStyle style) { |
There was a problem hiding this comment.
Better name would be isDuplicate(...) so that it reads as
if (isDuplicate(citationStyleToAdd)) {
...There was a problem hiding this comment.
Good idea!
Code updated.
github-actions
bot
added
status: changes-required
github-actions
bot
added
status: no-bot-comments
and removed
status: changes-required
|
|
||
| return Optional.empty(); | ||
| /// Adds a new external CSL style. | ||
| public void addStyle(@NonNull CitationStyle citationStyle) { |
|
Did one small modification: |
github-actions
bot
added
status: changes-required
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
github-actions
bot
added
status: no-bot-comments
and removed
status: changes-required
github-actions
bot
added
the
status: to-be-merged
3 participants
Related issues and pull requests
Closes #15438
PR Description
When users remove a citation style, if the style already exists in the list, it will not be removed immediately unless users reopen the dialog.
This PR added a duplication check to find styles with the same title (or short title) before insersion. In addition, this PR did some refactor to the related logic (mainly refactored some
ifPresentlogic to avoid nestedif) for more readability.Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)