Implement reset for Name Display Preferences#15136
Conversation
|
Hey @RakockiW! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
Review Summary by QodoImplement reset functionality for Name Display Preferences
WalkthroughsDescription• Add reset functionality for Name Display Preferences • Implement getDefault() and setAll() methods in NameDisplayPreferences • Remove hardcoded preference defaults from JabRefGuiPreferences • Integrate name display preferences into reset and import workflows Diagramflowchart LR
NDP["NameDisplayPreferences"]
NDP -->|"add getDefault()"| NDP2["Default instance"]
NDP -->|"add setAll()"| NDP3["Update preferences"]
JRP["JabRefGuiPreferences"]
JRP -->|"remove hardcoded defaults"| JRP2["Cleaner config"]
JRP -->|"add getNameDisplayPreferencesFromBackingStore()"| JRP3["Load from storage"]
JRP2 -->|"integrate into clear()"| RESET["Reset workflow"]
JRP3 -->|"integrate into importPreferences()"| IMPORT["Import workflow"]
File Changes1. jabgui/src/main/java/org/jabref/gui/maintable/NameDisplayPreferences.java
|
Code Review by Qodo
1. Null-unsafe setAll
|
| private NameDisplayPreferences getNameDisplayPreferencesFromBackingStore(NameDisplayPreferences defaults) { | ||
| return new NameDisplayPreferences( | ||
| defaults.getDisplayStyle(), | ||
| defaults.getAbbreviationStyle()); | ||
| } |
There was a problem hiding this comment.
1. getnamedisplaypreferencesfrombackingstore ignores store 📘 Rule violation ✓ Correctness
getNameDisplayPreferencesFromBackingStore is named as if it reads persisted values, but it only copies fields from defaults, so stored/imported preferences can be silently ignored. This violates the requirement for self-documenting naming because the identifier does not match actual behavior.
Agent Prompt
## Issue description
`getNameDisplayPreferencesFromBackingStore` is misleading and currently does not read from the backing store. It only returns values from the `defaults` argument, which can cause persisted/imported name display preferences to be ignored.
## Issue Context
Other `*FromBackingStore` helpers in this class read values via `getBoolean/getInt/...` with a fallback to `defaults`. Name display preferences should follow the same pattern (or the method should be renamed if it is intentionally default-only).
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[930-930]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[1228-1232]
ⓘ 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.
Review Summary by QodoImplement reset functionality for Name Display Preferences
WalkthroughsDescription• Add reset functionality for Name Display Preferences • Implement getDefault() and setAll() methods in NameDisplayPreferences • Refactor preference loading to use backing store with proper defaults • Remove hardcoded preference defaults from JabRefGuiPreferences • Integrate reset into preferences clear operation Diagramflowchart LR
A["NameDisplayPreferences"] -->|"add getDefault()"| B["Default preferences"]
A -->|"add setAll()"| C["Reset mechanism"]
D["JabRefGuiPreferences"] -->|"refactor loading"| E["getNameDisplayPreferencesFromBackingStore()"]
E -->|"use defaults"| B
F["clear() method"] -->|"call setAll()"| C
File Changes1. jabgui/src/main/java/org/jabref/gui/maintable/NameDisplayPreferences.java
|
|
Hi, I would be grateful if you could give me feedback on whether my approach in the method getNameDisplayPreferencesFromBackingStore is correct. |
|
Persistent review updated to latest commit f9577d1 |
jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java
Outdated
Show resolved
Hide resolved
Review Summary by QodoImplement reset functionality for Name Display Preferences
WalkthroughsDescription• Add reset functionality for Name Display Preferences • Implement getDefault() and setAll() methods in NameDisplayPreferences • Refactor preference loading to use backing store with proper defaults • Remove hardcoded preference defaults from JabRefGuiPreferences • Integrate name display preferences reset into global preferences clear method Diagramflowchart LR
A["NameDisplayPreferences"] -->|"add getDefault()"| B["Default preferences"]
A -->|"add setAll()"| C["Reset mechanism"]
D["JabRefGuiPreferences"] -->|"refactor loading"| E["getNameDisplayPreferencesFromBackingStore()"]
E -->|"use defaults"| B
D -->|"integrate reset"| F["clear() method"]
F -->|"calls setAll()"| C
File Changes1. jabgui/src/main/java/org/jabref/gui/maintable/NameDisplayPreferences.java
|
|
Persistent review updated to latest commit b95cf28 |
Review Summary by QodoImplement reset functionality for Name Display Preferences
WalkthroughsDescription• Add reset functionality for Name Display Preferences • Implement getDefault() and setAll() methods in NameDisplayPreferences • Refactor preference loading to use backing store with proper defaults • Remove hardcoded preference defaults from JabRefGuiPreferences • Integrate name display preferences reset into clear() and importPreferences() methods Diagramflowchart LR
A["NameDisplayPreferences"] -->|"add getDefault()"| B["Default preferences"]
A -->|"add setAll()"| C["Update preferences"]
D["JabRefGuiPreferences"] -->|"remove hardcoded defaults"| E["Cleaner initialization"]
D -->|"add getNameDisplayPreferencesFromBackingStore()"| F["Load from backing store"]
G["clear() method"] -->|"call setAll(getDefault())"| H["Reset to defaults"]
I["importPreferences() method"] -->|"call setAll(getFromBackingStore())"| J["Restore from storage"]
File Changes1. jabgui/src/main/java/org/jabref/gui/maintable/NameDisplayPreferences.java
|
373ebd4 to
0352567
Compare
This comment has been minimized.
This comment has been minimized.
0352567 to
ad6c30c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Sorry, thanks for poining that out. |
| System.out.println(NAMES_NATBIB + ": " + newValue); | ||
| putBoolean(NAMES_AS_IS, newValue == NameDisplayPreferences.DisplayStyle.AS_IS); | ||
| System.out.println(NAMES_AS_IS + ": " + newValue); | ||
| putBoolean(NAMES_FIRST_LAST, newValue == NameDisplayPreferences.DisplayStyle.FIRSTNAME_LASTNAME); | ||
| System.out.println(NAMES_FIRST_LAST + ": " + newValue); | ||
| }); | ||
| EasyBind.listen(nameDisplayPreferences.abbreviationStyleProperty(), (obs, oldValue, newValue) -> { | ||
| putBoolean(ABBR_AUTHOR_NAMES, newValue == NameDisplayPreferences.AbbreviationStyle.FULL); | ||
| System.out.println(ABBR_AUTHOR_NAMES + ": " + newValue); | ||
| putBoolean(NAMES_LAST_ONLY, newValue == NameDisplayPreferences.AbbreviationStyle.LASTNAME_ONLY); | ||
| System.out.println(NAMES_LAST_ONLY + ": " + newValue); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅🏷️ Commit: 943707d Learn more about TestLens at testlens.app. |
* Implement reset for NameDisplayPreferences * Fixed implemention of getNameDisplayPreferencesFromBackingStore method * refactor * deleted sout statements
…rg.openrewrite.recipe-rewrite-recipe-bom-3.25.0 * upstream/main: (35 commits) Chore: add dependency-management.md (#15278) Chore(deps): Bump dev.langchain4j:langchain4j-bom in /versions (#15277) New Crowdin updates (#15274) Chore(deps): Bump actions/upload-artifact from 6 to 7 (#15271) Chore(deps): Bump actions/download-artifact from 7 to 8 (#15270) Chore(deps): Bump docker/login-action from 3 to 4 (#15268) Fix threading issues in citations relations tab (#15233) Fix: Citavi XML importer now preserves citation keys (#14658) (#15257) Preserve no break spaces in Latex to Unicode conversion (#15174) Fix: open javafx.scene.control.skin to controlsfx (#15260) Reduce complexity in dependencies setup (restore) (#15194) New translations jabref_en.properties (French) (#15256) Fix: exception dialog shows up when moving sidepanel down/up (#15248) Implement reset for Name Display Preferences (#15136) Chore(deps): Bump net.bytebuddy:byte-buddy in /versions (#15252) Chore(deps): Bump io.zonky.test.postgres:embedded-postgres-binaries-bom (#15253) Chore(deps): Bump io.zonky.test:embedded-postgres in /versions (#15254) Chore(deps): Bump net.ltgt.errorprone from 5.0.0 to 5.1.0 in /jablib (#15251) New Crowdin updates (#15247) Refined the "Select files to import" page in "Search for unlinked local files" dialog (#15110) ...
Related issues and pull requests
Closes #14412
PR Description
Hello! I followed the steps in #14400 and implemented resetting for Name Display Preferences.
Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)