Conversation
PR Summary by QodoFix unlinked-files wizard theming, header graphics, and keyboard toggles
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Missing Grouped by directory key
|
|
|
||
| public enum ExternalFileSorter { | ||
| DEFAULT(Localization.lang("Default")), | ||
| DEFAULT(Localization.lang("Grouped by directory")), |
There was a problem hiding this comment.
1. Missing grouped by directory key 📘 Rule violation ≡ Correctness
ExternalFileSorter now requests the localization key Grouped by directory, but that key is only present in JabRef_en.properties and missing from at least JabRef_de.properties, which can lead to untranslated UI strings in non-English locales. When a translation is missing, the UI may fall back to displaying the raw key text instead of a localized label.
Agent Prompt
## Issue description
A new localization key (`Grouped by directory`) is now referenced from `ExternalFileSorter` via `Localization.lang(...)`, but it was only added to the English resource bundle (`JabRef_en.properties`) and is missing from at least one other locale bundle (e.g., `JabRef_de.properties`). This can result in untranslated UI text where non-English locales display the raw key string.
## Issue Context
Compliance requires localization keys to remain synchronized across all locale bundles and their call sites. JabRef’s localization implementation falls back to returning the key itself when a translation is missing, so any missing entries in `JabRef_*.properties` will surface directly in the UI.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/externalfiles/ExternalFileSorter.java[5-8]
- jablib/src/main/resources/l10n/JabRef_en.properties[177-181]
- jablib/src/main/resources/l10n/JabRef_de.properties[172-181]
- jablib/src/main/java/org/jabref/logic/l10n/Localization.java[130-168]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Dropdown rename was specified in the issue requirement to make the default file tree view clear to end user ,I also added the Grouped\ by\ directory has been added to JabRef_en.properties.
| private void hideHeaderGraphic() { | ||
| if (getScene() == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
2. hideheadergraphic() duplicated across pages 📘 Rule violation ⚙ Maintainability
The same hideHeaderGraphic() implementation is duplicated across multiple wizard pages, increasing maintenance cost and risking inconsistent future fixes. The implementation is also fragile because it hides the header graphic by querying .header-panel and mutating a GridPane child by index, coupling behavior to ControlsFX skin internals.
Agent Prompt
## Issue description
`hideHeaderGraphic()` is copy-pasted across multiple wizard pages and implemented via a fragile scene-graph lookup (`.header-panel`) that assumes a `GridPane` and hides a child by index, coupling behavior to ControlsFX internals.
## Issue Context
Compliance guidance is to avoid duplication when common logic can be shared, especially when the duplicated logic is brittle and risks diverging over time. In this PR, the same `hideHeaderGraphic()` block appears in multiple pages and uses `Platform.runLater(...)` plus `getScene().lookup(".header-panel")` / `grid.getChildren().get(1)` to remove the header graphic, while at least one page already uses the supported API (`setGraphic(null)`) to remove graphics.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/FileSelectionPage.java[205-215]
- jabgui/src/main/java/org/jabref/gui/externalfiles/SearchConfigurationPage.java[252-262]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportResultsPage.java[159-169]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportResultsPage.java[149-169]
- jabgui/src/main/java/org/jabref/gui/externalfiles/FileSelectionPage.java[55-62]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| stage.setWidth(650); | ||
| stage.setHeight(550); | ||
| stage.getIcons().addAll(IconTheme.getLogoSet()); | ||
| themeManager.installCssOnScene(stage.getScene()); |
There was a problem hiding this comment.
3. Theme install wipes stylesheets 🐞 Bug ≡ Correctness
UnlinkedFilesWizard applies theming via ThemeManager.installCssOnScene(stage.getScene()), which replaces the scene’s entire stylesheet list (setAll) and can remove any pre-existing dialog/ControlsFX stylesheets, causing UI regressions in this wizard.
Agent Prompt
### Issue description
`ThemeManager.installCssOnScene(Scene)` calls `scene.getStylesheets().setAll(toAdd)`, which overwrites all previously-registered stylesheets on that scene. Calling it from `UnlinkedFilesWizard.show()` risks removing any wizard/dialog-specific stylesheets that were already present.
### Issue Context
This method is documented as primarily intended for app startup; dialogs/wizards may already have stylesheets registered.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/UnlinkedFilesWizard.java[48-56]
- jabgui/src/main/java/org/jabref/gui/theme/ThemeManager.java[84-99]
### Suggested fix
Add a ThemeManager method that *adds/merges* the JabRef theme stylesheets without clobbering existing ones (e.g., prepend if missing), and use that method here instead of `installCssOnScene`. Alternatively, locally merge in UnlinkedFilesWizard by preserving `scene.getStylesheets()` contents and only ensuring the JabRef theme sheets are present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Violating our ai policy. Also alternating checklist, removing licensing option. This is not acceptable. If you continue to ignore our policies, we will have to ban you from our repository, regardless of the quality of your code. Sorry. See #16590 (comment) |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #16158. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
|
I sincerely apologize to the maintainers and the JabRef team. I accidentally removed the licensing line from the checklist while attempting to fix formatting errors. I completely respect your policies and your decision. Thank you for your time and for reviewing my work |
|
You may continue if you restore the checklist. Again, this is not about the quality of your contribution. But you need to respect the policies about contributing and communicating with us and our community please. |
| - We fixed the "Search for unlinked local files" that was not applying the JabRef's theme. [#16158](https://github.com/JabRef/jabref/issues/16158) | ||
| - We fixed the question-mark icon appearing in the "Search for unlinked local files" wizard pages. [#16158](https://github.com/JabRef/jabref/issues/16158) | ||
| - We fixed the "select files to import" header visibility while loading the required files and still running. [#16158](https://github.com/JabRef/jabref/issues/16158) | ||
| - We fixed the space key not toggling the file checkboxes after we scan the document and we have a tree structure of our files in "Search for unlinked local files". [#16158](https://github.com/JabRef/jabref/issues/16158) |
There was a problem hiding this comment.
1. Changelog wording too technical 📘 Rule violation ⚙ Maintainability
Several new CHANGELOG.md entries are overly detailed and read like implementation notes, reducing clarity for end users. This makes release notes harder to understand and violates the end-user wording requirement.
Agent Prompt
## Issue description
The new `CHANGELOG.md` bullets for the unlinked-files wizard fixes are too detailed/implementation-focused and should be rewritten for average end users.
## Issue Context
Compliance requires end-user wording for changelog entries and avoiding programmer-centric descriptions.
## Fix Focus Areas
- CHANGELOG.md[154-157]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| themeManager.installCssOnScene(stage.getScene()); | ||
| } |
There was a problem hiding this comment.
3. Theme applied after show 🐞 Bug ☼ Reliability
UnlinkedFilesWizard.show() defers themeManager.installCssOnScene(...) via Platform.runLater, but then immediately calls wizard.showAndWait(), so CSS installation can happen after the dialog is already visible. ThemeManager explicitly documents that delayed CSS installation can cause a flash of the default JavaFX (Modena) theme before the intended theme appears.
Agent Prompt
## Issue description
CSS installation is scheduled with `Platform.runLater`, but the wizard is shown immediately with `showAndWait()`. This allows a window-visible interval where the scene still uses default styles.
## Issue Context
`ThemeManager.installCssOnScene` is explicitly documented as the method to avoid theme-application delay/flash; however, calling it in a `runLater` right before `showAndWait()` undermines that guarantee.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/UnlinkedFilesWizard.java[43-59]
- jabgui/src/main/java/org/jabref/gui/theme/ThemeManager.java[84-99]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit dea7533 |
|
The requested changes were not addressed for 3 days. Please follow-up in the next 7 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
|
All checks complete -submodule issue is fixed and branch is synced with main |
| } | ||
| javafx.scene.Node headerPanel = getScene().lookup(".header-panel"); | ||
| if (headerPanel instanceof javafx.scene.layout.GridPane grid && grid.getChildren().size() > 1) { | ||
| javafx.scene.Node graphicContainer = grid.getChildren().get(1); |
There was a problem hiding this comment.
used proper import statement instead of java reference, thanks for flagging it
| } | ||
|
|
||
| private void applyThemeAndStageSettings(javafx.scene.Scene scene) { | ||
| if (cssInstalled || !(scene.getWindow() instanceof javafx.stage.Stage stage)) { |
There was a problem hiding this comment.
addressed it , thankyou
|
The requested changes were not addressed for 3 days. Please follow-up in the next 7 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
|
addressed the changes and added proper import statements and fixed errors |
Summary
Fixed all bugs in the Search for unlinked local files related to css theming of the dialog box and the unwanted question mark changed the sort method name from default to Grouped by directory and using space for toggling the switch , removing the headers from the loading screen ,
this makes the dialog box with original css themed doesnt look odd and maintains consistency in it
Analogies
this fix was necessary as when we see a chocolate or any materail which is not in the same theme as rest of the material it standsout and in here feels uncomfortable for the user to watch the small imperfections on moon surface as well keeps us making uncomfortable and impurities in honey makes it look bad so single theming is necessary and the default java theme is eye-straining
jabref-contrib-policy:4.2:reviewed:ok
i took help of claude AI to understand massive codebase of jabref and guide me to the files after that i personally tested the changes and reviewed the code and flow of it
Steps to test
Screenshots of the fix
Related issues and pull requests
Closes #16158
Checklist
CHANGELOG.mdin a way that can be understood by the average user