Add tabs to Clean Up Entries dialog #13852

alexsukhin · 2025-09-10T23:11:40Z

This PR adds three tabs to the "Clean up entries" dialog: Single-field, File-related, and Multi-field, representing different types of formatters.

Pressing Apply applies the selected actions of the currently active tab, and Close closes the dialog.

Steps to test

Launch JabRef
Create a new example library
Select an entry
Go to Quality → Clean up entries and select your desired formatters
Apply the formatters or Close the dialog

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
I manually tested my changes in running JabRef (always required)
I added JUnit tests for changes (if applicable)
I added screenshots in the PR description (if change is visible to the user)
I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

- Removed trivial comments - Renamed PDF-related variables - Updated methods to return Optional - Used Optional property for FieldFormatterCleanups

- Removed trivial comments - Fixed CHANGELOG.md

- Removed trivial comments - Improved Optional checks - Perform null check for other parameters

calixtus

First impression after skimming through your changes: looks good, right track. Just set your PR to rfr as soon as you think we should review it.

jablib/src/test/java/org/jabref/logic/cleanup/CleanupPreferencesTest.java

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialog.java

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupFileRelatedPanel.java

calixtus

Hi, thanks for your PR,
the code itself looks fine to me.
However, I think with the refactoring to the tabbed dialog, there is now a logical gap:
You use the strategy pattern to configure the preferences to use in the calling CleanupAction and the currently selected tab as part of the configuration, what action should be called: single / multi ... I dont think that this is a good and intuitive ui pattern.

We discussed this shortly on JabCon. We think the solution would be the following ():

Dismiss returning configuration from the dialog, and pull the logic from the action into a viewmodel of the cleanupDialog.
Move the ok/cleanup button in the ui inside each tab
Leave a close/cancel button at the bottom of the dialog.

Hopefully this will relax the complexity of the code and wont mean too much refactoring.
(@koppor @Siedlerchr wdyt?)

calixtus · 2025-09-12T10:07:51Z

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialog.java

+            if (button.getButtonData() == ButtonBar.ButtonData.OK_DONE) {
+                Tab selectedTab = tabPane.getSelectionModel().getSelectedItem();
+                CleanupPanel panel = (CleanupPanel) selectedTab.getContent();
+                return panel.getCleanupPreferences();


This is weird for this dialog.

Split Class org.jabref.gui.cleanup.CleanupPresetPanel split to [org.jabref.gui.cleanup.CleanupFileRelatedPanel, org.jabref.gui.cleanup.CleanupMultiFieldPanel]

JabRef/jabref#13852

alexsukhin · 2025-09-12T14:31:11Z

Hi, thank you for the review! I understand that moving the cleanup logic into a ViewModel per tab will simplify the UI.

Just to confirm, is the enum refactor okay? I will return a CleanupPreferences from the ViewModel containing the preferences from the currently selected tab, and in CleanupAction I will update the initial preferences by replacing the category with the preferences returned from the tab.

I will also move the Apply button into each tab and leave a global Close button at the bottom.

calixtus · 2025-09-12T14:36:06Z

About View and ViewModel: Every piece of logic should go into the ViewModel. You have a button on each tab, calling a different function in the viewmodel. So there should be no need for the enum anymore. See https://devdocs.jabref.org/code-howtos/javafx.html#architecture-model---view---controller---viewmodel-mvcvm

- Create new ViewModel, pull logic from Action and SingleAction into ViewModel - Move Apply button to each tab - Remove categories from ENUM and keep enums of all jobs in each respective tab to be used for cleanup

* Fix non record comments by carl # Conflicts: # jabgui/src/main/java/org/jabref/gui/edit/automaticfiededitor/MoveFieldValueAction.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/cell/sidebuttons/ToggleMergeUnmergeButton.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/CommentMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/FieldMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/FileMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/GroupMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/KeywordMerger.java # jablib/src/main/java/org/jabref/logic/layout/format/HTMLChars.java # jablib/src/main/java/org/jabref/model/entry/identifier/ArXivIdentifier.java # jablib/src/main/java/org/jabref/model/entry/identifier/EprintIdentifier.java * Add file exceptions * Remove shebang line * Remove shebang line * Remove shebang line * Expand variables & rename class --------- Co-authored-by: Oliver Kopp <[email protected]>

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupMultiFieldPanel.java

trag-bot · 2025-10-02T12:32:24Z

@trag-bot didn't find any issues in the code! ✅✨

alexsukhin · 2025-10-08T17:57:46Z

Hi, this PR is ready for review :)

calixtus · 2025-10-09T21:27:44Z

We will look into it asap, but it could take a few days, as we are currently all very busy with our day jobs. Please excuse our tight schedule.

koppor · 2025-11-05T09:18:59Z

I think RefactoringMiner is needed for review. Reason: Need to check which code is new and which code was just moved.

koppor

Two micro comment; one minor comment, then it should be good to go.

koppor · 2025-11-06T23:37:39Z

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java

+        if (tabSupplier != null) {
+            tabSupplier.get().markBaseChanged();
+        }


This logic is different from the old code

It is strange that tabSupplier is asked for null.

When can it be null?

Shouldn't just simply be checked for modifiedEntriesCount being greater than 0 - as in the old code?

Hi, just to clarify: the tabSupplier != null check was added for the single-action case, where tabSupplier is null. I also added the modifiedEntriesCount > 0 condition to ensure that the base is only marked as changed when an entry is actually modified.

There is a code commetn for that, right?

Hi no, not yet I will add a comment ASAP!

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java

jablib/src/test/java/org/jabref/logic/cleanup/CleanupTabSelectionTest.java

- Remove redundant comments following self-explanatory code - Add modifiedEntriesCount > 0 condition - Use "entry(s)" localization form for clean up message

koppor

OK, let's get this in. In case there are issues, we can fix later 😅

* Add cleanup dialog tabs with individual tab preferences * Fixed indentation and added commenting * Fix Trag-bot review issues - Removed trivial comments - Renamed PDF-related variables - Updated methods to return Optional - Used Optional property for FieldFormatterCleanups * Fix Trag-bot review issues - Removed trivial comments - Fixed CHANGELOG.md * Fix Trag-bot review issues - Removed trivial comments - Improved Optional checks - Perform null check for other parameters * Avoid nested Optionals, returning Optional<CleanupPreferences> directly * Refactor CleanupPreferences by keeping one assertion per test * Converted tests to assertEquals * Maintain consistent naming conventions * Returns CleanupPreferences directly since value is always present * Initial review refactor draft - Create new ViewModel, pull logic from Action and SingleAction into ViewModel - Move Apply button to each tab - Remove categories from ENUM and keep enums of all jobs in each respective tab to be used for cleanup * fix import error! * Reformat codebase (more carefully) (JabRef#13885) * Fix non record comments by carl # Conflicts: # jabgui/src/main/java/org/jabref/gui/edit/automaticfiededitor/MoveFieldValueAction.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/cell/sidebuttons/ToggleMergeUnmergeButton.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/CommentMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/FieldMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/FileMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/GroupMerger.java # jabgui/src/main/java/org/jabref/gui/mergeentries/threewaymerge/fieldsmerger/KeywordMerger.java # jablib/src/main/java/org/jabref/logic/layout/format/HTMLChars.java # jablib/src/main/java/org/jabref/model/entry/identifier/ArXivIdentifier.java # jablib/src/main/java/org/jabref/model/entry/identifier/EprintIdentifier.java * Add file exceptions * Remove shebang line * Remove shebang line * Remove shebang line * Expand variables & rename class --------- Co-authored-by: Oliver Kopp <[email protected]> * fix import error & merge * Apply OpenRewrite Cleanup * Refactor Cleanup Tabs - Moved cleanup panel logic into CleanupDialogViewModel for better separation of UI and logic - Changed tabSupplier and taskExecutor from Optional to nullable parameters - Moved updateWith logic into the record for cleaner preference updates. - General design improvements: more maintainable. * Fix getDisplayName method * Fix formatting * Trag-bot review and fix en properties * fix indentation plssss * format properly and change to observablelist * fix formatting entriestoprocess (please) * Updated names and changed optional dependencies back to nullable * Refactored panels to use separate ViewModels - Introduced ViewModels to encapsulate state and logic for panels. - Replaced direct UI manipulation with bidirectional bindings. - Ensures cleaner UI logic, easier maintenance * Moved ALL_JOBS to respective ViewModels, small naming changes * Replaced requireNotNull to @NotNull following JabRef#13957 * Address review feedback in CleanupDialogViewModel - Remove redundant comments following self-explanatory code - Add modifiedEntriesCount > 0 condition - Use "entry(s)" localization form for clean up message --------- Co-authored-by: Subhramit Basu <[email protected]> Co-authored-by: Oliver Kopp <[email protected]> Co-authored-by: Christoph <[email protected]>

alexsukhin added 2 commits September 10, 2025 23:56

Add cleanup dialog tabs with individual tab preferences

400c6b0

Fixed indentation and added commenting

dd2f74c

alexsukhin marked this pull request as draft September 10, 2025 23:25

alexsukhin added 2 commits September 11, 2025 01:35

Fix Trag-bot review issues

033201c

- Removed trivial comments - Renamed PDF-related variables - Updated methods to return Optional - Used Optional property for FieldFormatterCleanups

Fix Trag-bot review issues

14fde53

- Removed trivial comments - Fixed CHANGELOG.md

alexsukhin force-pushed the 13819-add-dialog-tabs branch from e8093f7 to 14fde53 Compare September 11, 2025 01:17

alexsukhin and others added 3 commits September 11, 2025 02:21

Merge branch 'main' into 13819-add-dialog-tabs

9c44aad

Fix Trag-bot review issues

0c5939e

- Removed trivial comments - Improved Optional checks - Perform null check for other parameters

Avoid nested Optionals, returning Optional<CleanupPreferences> directly

757514e

calixtus reviewed Sep 11, 2025

View reviewed changes

Refactor CleanupPreferences by keeping one assertion per test

a60f696

trag-bot bot reviewed Sep 11, 2025

View reviewed changes

jablib/src/test/java/org/jabref/logic/cleanup/CleanupPreferencesTest.java Outdated Show resolved Hide resolved

Converted tests to assertEquals

43c981b

alexsukhin mentioned this pull request Sep 11, 2025

Update cleanup dialogs page to tab-based implementation JabRef/user-documentation#591

Open

alexsukhin marked this pull request as ready for review September 11, 2025 16:30

trag-bot bot reviewed Sep 11, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialog.java Outdated Show resolved Hide resolved

Maintain consistent naming conventions

56b6164

trag-bot bot reviewed Sep 11, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupFileRelatedPanel.java Outdated Show resolved Hide resolved

Returns CleanupPreferences directly since value is always present

4f2ed12

calixtus requested changes Sep 12, 2025

View reviewed changes

tsantalis added a commit to tsantalis/RefactoringMiner that referenced this pull request Sep 12, 2025

Further improving diff quality for split classes in PR

23720e3

JabRef/jabref#13852

alexsukhin marked this pull request as draft September 12, 2025 22:07

alexsukhin and others added 5 commits September 14, 2025 03:24

Initial review refactor draft

1d2f991

- Create new ViewModel, pull logic from Action and SingleAction into ViewModel - Move Apply button to each tab - Remove categories from ENUM and keep enums of all jobs in each respective tab to be used for cleanup

Merge branch 'main' into 13819-add-dialog-tabs

c8c3d42

fix import error!

7276eb0

fix import error & merge

55c02c2

trag-bot bot reviewed Sep 25, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java Show resolved Hide resolved

trag-bot bot reviewed Sep 25, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java Outdated Show resolved Hide resolved

trag-bot bot reviewed Sep 25, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java Outdated Show resolved Hide resolved

trag-bot bot reviewed Sep 25, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/cleanup/CleanupMultiFieldPanel.java Show resolved Hide resolved

Merge branch 'main' into 13819-add-dialog-tabs

b7f4594

github-actions bot removed the status: changes-required Pull requests that are not yet complete label Oct 1, 2025

Merge branch 'main' into 13819-add-dialog-tabs

9c9f202

calixtus added component: ui component: cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Oct 9, 2025

Merge branch 'main' into 13819-add-dialog-tabs

40f6dbf

Merge branch 'main' into 13819-add-dialog-tabs

25a1151

koppor mentioned this pull request Nov 6, 2025

Add link to blog article #14245

Merged

1 task

koppor reviewed Nov 6, 2025

View reviewed changes

koppor mentioned this pull request Nov 6, 2025

Method call not detected as moved tsantalis/RefactoringMiner#1010

Closed

alexsukhin and others added 3 commits November 9, 2025 20:40

Merge branch 'main' into 13819-add-dialog-tabs

cb94ec5

Address review feedback in CleanupDialogViewModel

5adc301

- Remove redundant comments following self-explanatory code - Add modifiedEntriesCount > 0 condition - Use "entry(s)" localization form for clean up message

Merge branch 'main' into 13819-add-dialog-tabs

576071d

koppor approved these changes Nov 11, 2025

View reviewed changes

koppor enabled auto-merge November 11, 2025 17:38

koppor added this pull request to the merge queue Nov 11, 2025

Merged via the queue into JabRef:main with commit c0642a7 Nov 11, 2025
48 checks passed

github-actions bot mentioned this pull request Nov 11, 2025

Cleanup entries #13819

Closed

koppor mentioned this pull request Nov 20, 2025

Add Integrity Checker for BookTitle #13854

Open

10 tasks

Uh oh!

Add tabs to Clean Up Entries dialog #13852

Add tabs to Clean Up Entries dialog #13852

Uh oh!

Conversation

alexsukhin commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to test

Mandatory checks

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

calixtus Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

alexsukhin commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calixtus commented Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trag-bot bot commented Oct 2, 2025

Uh oh!

alexsukhin commented Oct 8, 2025

Uh oh!

calixtus commented Oct 9, 2025

Uh oh!

koppor commented Nov 5, 2025

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

koppor Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

alexsukhin Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

koppor Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

alexsukhin Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexsukhin commented Sep 10, 2025 •

edited

Loading

alexsukhin commented Sep 12, 2025 •

edited

Loading