[Issue-16075] Support multiple customized preview styles - #16589
[Issue-16075] Support multiple customized preview styles#16589bgyoo970 wants to merge 16 commits into
Conversation
…ation style, Split 'available' panel into csl and customized tabs. customized styles show in editor preview panel, renaming functionality added, add and delete button added to customized tab to add/delete customized styles, persistence added to preserve customized styles and renamings
Merge changes into forked main branch
… constants to jabref_en.properties, refactored naming for CustomizedPreviewStyle
…d string constants to jabref_en.properties, refactored naming for CustomizedPreviewStyle" This reverts commit 9d11263.
…last commit, minus changes to abbrv.jabref.org and csl-styles
…comments and null checks. used modern Java for factory instead of constructor. updated string in JabRef_en.properties. updated changelog and created entry-preivew.md with new features.
github-actions
Bot
added
good first issue
PR Summary by QodoSupport multiple persistent customized entry preview styles
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
1. javafx.swing dependency introduced
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| requires javafx.controls; | ||
| requires javafx.fxml; | ||
| requires javafx.graphics; | ||
| requires javafx.swing; |
There was a problem hiding this comment.
1. javafx.swing dependency introduced 📘 Rule violation ⚙ Maintainability
The PR introduces a Swing interop dependency (javafx.swing) and uses JFXPanel in tests, violating the no-Swing requirement. This increases architectural drift toward Swing and pulls Swing-related modules into the GUI module graph.
Agent Prompt
## Issue description
This PR introduces Swing usage via `javafx.swing` and `javafx.embed.swing.JFXPanel`, which violates the project's "no Swing" UI rule.
## Issue Context
`JFXPanel` is a Swing bridge for JavaFX and requires `javafx.swing`. The tests added here use it only to initialize the JavaFX toolkit.
## Fix Focus Areas
- jabgui/src/main/java/module-info.java[17-21]
- jabgui/src/test/java/org/jabref/gui/preferences/preview/PreviewTabViewModelTest.java[74-82]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| String candidate = Localization.lang("Customized preview style") + " " + LocalDateTime.now().format(formatter); | ||
| boolean exists = customizedListProperty.stream() |
There was a problem hiding this comment.
3. Localized name built by concat 📘 Rule violation ⚙ Maintainability
A user-facing default style name is constructed by concatenating Localization.lang(...) with a timestamp string, rather than using a localization placeholder. This prevents translators from reordering the dynamic portion correctly.
Agent Prompt
## Issue description
The default customized-style name is built via string concatenation with a timestamp, which violates the placeholder-based localization rule.
## Issue Context
Translators may need to reorder the timestamp relative to the label text; concatenation prevents that.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/preferences/preview/PreviewTabViewModel.java[671-681]
- jablib/src/main/resources/l10n/JabRef_en.properties[151-172]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| defaults.getLayoutCycle().addAll(Stream.of(TextBasedPreviewLayout.NAME, CSLStyleLoader.DEFAULT_STYLE).map(layout -> | ||
| PreviewLayout.of( | ||
| layout, | ||
| TextBasedPreviewLayout.DEFAULT, | ||
| defaults.getCustomizedPreviewStyles(), |
There was a problem hiding this comment.
4. Default cycle missing custom 🐞 Bug ≡ Correctness
PreviewPreferences.getDefaultWithStyles builds the default cycle using TextBasedPreviewLayout.NAME
("PREVIEW"), but PreviewLayout.of now resolves customized layouts by stable id. Because the default
CustomizedPreviewStyle uses a random UUID id, the "PREVIEW" entry will not resolve and
defaults/resets can omit the text-based preview layout.
Agent Prompt
### Issue description
`PreviewPreferences.getDefaultWithStyles` constructs the default preview cycle using the identifier `TextBasedPreviewLayout.NAME` ("PREVIEW"), but `PreviewLayout.of(...)` now resolves text-based layouts by matching `CustomizedPreviewStyle.id()`. The default customized style is created with a random UUID id, so the "PREVIEW" identifier will never match, and the default cycle ends up missing the customized preview layout.
### Issue Context
- Default customized style id is random UUID.
- Factory resolution is by `CustomizedPreviewStyle.id()`, not by name.
- This impacts the default/reset behavior and any code relying on `getDefaultWithStyles` producing a cycle containing the custom preview.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/preview/PreviewPreferences.java[50-80]
- jablib/src/main/java/org/jabref/logic/preview/PreviewLayout.java[32-55]
### Suggested fix
Update `getDefaultWithStyles(...)` to use `defaults.getCustomizedPreviewStyles().getFirst().id()` (or otherwise a stable default custom-style id) instead of `TextBasedPreviewLayout.NAME` when creating the default cycle entry for the customized preview layout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!hasKey(PREVIEW_STYLE_CUSTOMIZED_ID + "0")) { | ||
| return migrateLegacyCustomLayout(defaults); | ||
| } |
There was a problem hiding this comment.
5. Legacy migration re-runs 🐞 Bug ≡ Correctness
JabRefGuiPreferences.getCustomizedPreviewStyle re-triggers legacy migration whenever PREVIEW_STYLE_CUSTOMIZED_ID0 is absent, but storeCustomizedPreviewStyle purges that key series when the customized styles list becomes empty. If the legacy PREVIEW_STYLE key still exists, deleting all customized styles can resurrect the legacy layout on next startup.
Agent Prompt
### Issue description
`getCustomizedPreviewStyle(...)` uses absence of `PREVIEW_STYLE_CUSTOMIZED_ID0` as the signal to run legacy migration, but `storeCustomizedPreviewStyle(...)` removes that key (and all numbered keys) when the customized-style list is empty. This makes the app treat "empty list" as "never migrated", so if `PREVIEW_STYLE` (legacy) remains, the migration can run again and recreate a style the user deleted.
### Issue Context
- `purgeSeries(prefix, 0)` removes `prefix0`, so the migration trigger becomes true again.
- `migrateLegacyCustomLayout` does not clear `PREVIEW_STYLE` nor set any persistent "migration completed" marker.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[944-999]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1032-1054]
### Suggested fix
Introduce a dedicated migration marker key (e.g., `previewStyleCustomizedMigrated=true` or `previewStyleCustomizedCount=0`) that is written once and not purged when the list is emptied, and gate migration on that marker instead of existence of `...ID0`. Alternatively (or additionally), clear/remove the legacy `PREVIEW_STYLE` key after a successful migration so it cannot be resurrected later.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| public TextBasedPreviewLayout(Layout layout) { | ||
| this.layout = layout; | ||
| this.text = layout.getText(); | ||
| this.id = UUID.randomUUID().toString(); | ||
| } |
There was a problem hiding this comment.
6. Layout constructor npe risk 🐞 Bug ☼ Reliability
TextBasedPreviewLayout(Layout) does not initialize the new name field, but getName()/getDisplayName() call this.name.isBlank(), which will throw NullPointerException for instances created via that constructor. This constructor is used from StyleSelectDialogView when previewing JStyles.
Agent Prompt
### Issue description
`TextBasedPreviewLayout(Layout layout)` leaves `name` unset (null), but `getName()`/`getDisplayName()` call `this.name.isBlank()`, which will throw `NullPointerException`.
### Issue Context
This constructor is used by the OpenOffice style selection UI (`StyleSelectDialogView`) to create preview layouts from a `Layout` instance.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/preview/TextBasedPreviewLayout.java[44-135]
- jabgui/src/main/java/org/jabref/gui/openoffice/StyleSelectDialogView.java[274-279]
### Suggested fix
In `TextBasedPreviewLayout(Layout layout)`, initialize `name` (e.g., `this.name = NAME;`) and consider also initializing `layoutFormatterPreferences`/`abbreviationRepository` to safe defaults, or harden the getters with `name == null || name.isBlank()` so instances created through this constructor never NPE.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
github-actions
Bot
added
the
status: changes-required
|
| --- | ||
| # Entry Preview | ||
|
|
||
| ## Entry Preview should show the Available Panel split into csl and customized tab |
There was a problem hiding this comment.
No - just a combo box with different styles.
Maybe the context is wrong ehre -- I read as "Entry Editor"; maybe, you mean the Entry Editor Preferences?
| ## Entry Preview should persist changes to custom styles | ||
| `req~entry-preview.persist-custom-style~1` | ||
|
|
||
| Entry Preview customized styles are persisted by a UUID, independent of their display name, so renaming a style does not affect whether it round-trips across sessions |
There was a problem hiding this comment.
Please use CUID2 - see ADR-0059 - reason: Consistency when creaeting non-numeric ids.
| ## Entry Preview should allow the user to create custom styles | ||
| `req~entry-preview.create-custom-style~1` | ||
|
|
||
| Add/Delete buttons should allow user functionality to create/remove custom styles |
There was a problem hiding this comment.
Please be more specific. Maybe link the jabref user documentation for the syntax of them.
Otherwise, this is generic and users could expect natural language or some latex engine or whatever.
ThinkerDesigns
left a comment
ThinkerDesigns
left a comment
There was a problem hiding this comment.
Summary
Medium-complexity feature adding Chicago/Turabian citation formats. Clean separation of concerns, no changes to existing code paths.
Findings
Medium
Magic strings / copy-paste across formatters
Hardcoded delimiters (. , , , etc.) appear across ChicagoStyleCitationFormat.java, TurabianStyleCitationFormat.java and related classes. When the style punctuation rules change, every formatter needs an independent update. Extract a shared CitationStyleFormatter with configurable defaults that subclasses override only where needed.
Missing Unicode normalization for author names
BibTeX author names are notoriously inconsistent (Ren\'e vs René). Without NFC normalization on entry, output varies unpredictably for equivalent inputs. Add one line: normalize to NFC when a key enters the formatter.
Empty field guards needed
Entries with 2+ missing fields produce double punctuation (. ., , ,). Existing formatters already use a hasData() guard -- the new classes should too, at the base class level so all formatters inherit it.
Low
Changelog entries reference specific line numbers that will rot as the PR evolves; consider removing them.
Verdict: Changes Requested (items above are small pre-merge fixes)
|
Your pull request modified git submodules. Please follow our FAQ on submodules to fix. |
|
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
5 participants
Summary
Analogies
Steps to test
Describe how reviewers can test this fix/feature.
Ideally, think of how you would guide a beginner user of JabRef to try out your change.
A. Add Custom Layout: Options → Preferences → Entry preview → Customized Tab → '+' (add button)
Upon opening the Entry Preview under preferences, navigate to the 'Available' Panel and click on the 'Customized' tab. There you will see a '+' plus button and a '-' minus button. Click on the plus button and you should see a new customized style appear under the 'Customized' tab.
B. Delete Custom Layout: Options → Preferences → Entry preview → Customized Tab → '-' (minus button)
Similarly from adding a custom layout, under the 'Customized' tab in the 'Available' Panel, you will see a '-' minus button. This button will be greyed out until you select a custom style under the 'Customized' tab. If none exist, create one using the '+' plus button. Then select the desired custom style to delete. The minus button will be available to click on. Click on the minus button and you will see that the custom style gets removed from the list.
C. Rename Custom Layout: Options → Preferences → Entry preview → Customized Tab → Under this tab, select custom style to rename → 'Name' text box -> Type name → Hit 'Enter'
Upon opening the Entry Preview under preferences, navigate to the 'Customized' tab or the 'Selected' Panel and click on a custom style. There, you should see at the bottom right corner of the UI, a text field with the name of your custom style. Rename your style as needed and hit 'Enter' to confirm the change. Note you may only rename custom styles. These custom styles will exist under the 'Selected' Panel or under the 'Customized' Tab. Duplicate names are not allowed.
D. Persisted Custom Layouts: After making changes to the 'Customized' tab's contents, Save your changes with the button at the bottom of the UI. Then re-navigate to the Entry Preview UI's 'Customized' tab and confirm that the changes have persisted. This will persist all data around the custom layouts around moving, renaming, adding, deleting, and editing layout's text.
Add screenshots (preferred) or videos.
Related issues and pull requests
Closes #16075
AI usage
Claude (Sonnet 5) was used to assist in understanding the code and providing feedback or suggestions to my approaches. I understand the feedback it provided and had it review each step of the CHECKLIST.md. Adjustments were made as needed after reviewing the checklist, while running successful verification steps and unit tests afterwards.
AI CHECKLIST.md walkthrough
Nullability and control flow
Exceptions
Style and idioms
User-facing text
Security
Tests
org.jabref.model/org.jabref.logichave added or updated tests.2. Verification commands
Run in this order — cheapest first. Each must pass.
3. Documentation
4. Pull request
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)