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?
There was a problem hiding this comment.
I am referring to "Entry Preview" and not "Entry Editor".
To my understanding from the CHECKLIST.md, The work I did for this PR should qualify as a new feature; the points I added to this file are just the overall features I implemented. I'm not sure if I added the correct content for this file, or if I even need to create this file.
Also, to double check, do you think this file is needed?
I'm creating this new docs/requirements/.md file per CHECKLIST.md:
Requirement added to docs/requirements/.md if the change is a new feature or significant bug fix (skip for refactors, minor fixes, and internal changes).
This file is referencing the "entry preview" under preferences, instead of "entry editor"
I'm guessing you're referring to the combo box under "entry editor" -> "citation count fetcher"? Which shouldn't be related here
| ## 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.
There was a problem hiding this comment.
Sure, I'll address this in my next push and I'll update the description here
| ## 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.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
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. |
|
Your pull request conflicts with the target branch. Please merge |
|
Can I get a review on the other PR related to this? I created a PR from a non-main branch from my fork as requested. I also addressed the comments here and the qodo comments. |
| requires javafx.controls; | ||
| requires javafx.fxml; | ||
| requires javafx.graphics; | ||
| requires javafx.swing; |
There was a problem hiding this comment.
Swing is strictly forbidden. Remove!
| /* | ||
| constructing a ListView just to borrow its selection model needs the JavaFX toolkit initialized. | ||
| If not init, tests throw IllegalStateException: Toolkit not initialized | ||
| */ | ||
| @BeforeAll | ||
| static void initToolkit() { | ||
| new JFXPanel(); | ||
| } |
There was a problem hiding this comment.
Use JavaFX toolkit. NO SWING.
Look at other GUI tests.
6 participants
Summary
The available panel was split into the 'CSL' and 'Customized' tabs to display preset or customized preview layouts respectively. Buttons were added to support the addition and deletion of specifically custom styles under the 'customized' tab only, along with a text box to rename customized styles. Changes were made throughout related files to support more than just the one default "PREVIEW" custom layout and persist multiple custom layouts between user sessions.
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)