Fix undo B - Ids on undo changes, rewordings and split of UndoManager, minor clarifications by calixtus · Pull Request #16680 · JabRef/jabref

calixtus · 2026-08-25T14:16:49Z

Summary

Second PR of the undo/redo series. this one fixes what reviewing that model turned up, and finishes the separation it started.

hasChanged() compared a net edit balance against the balance recorded at save time, so two different histories reached the same number: edit, save, undo, edit again, and the library reported itself unmodified although it held work that was never saved. Since that flag decides whether closing a library asks to save, the library closed silently and the second edit was lost. Positions are now identified rather than counted, so a saved position that history has discarded can never be matched again.
apply renamed in this iteration to applyEdit. performs the change and records it under one acquisition of the journal's monitor, closing the window in which an undo from another thread reverted the previous change while the new one stayed applied but unrecorded.
UndoManager becomes the recording interface that the ~118 classes editing the library depend on, JabRefUndoManager the implementation, and GuiUndoManager / JabRefGuiUndoManager the JavaFX layer above it (cp. CliPreferences / GuiPreferences).
No class type is threaded through the GUI any more: what a class asks for now says what it does with the journal.
Undo step names are now written for whoever will read them, taken from the action the user invoked where one exists, instead of string tokens like CHANGE_SELECTED_FIELD;
Two file operations that undo cannot reverse say so where the user decides, rather than leaving a dangling link to be discovered later.
```
  `jabref-contrib-policy:4.2:reviewed:ok`
```

Steps to test

The close prompt — open a library, edit an entry, save, press Ctrl+Z, then edit any field again. The tab title keeps its asterisk and closing the library asks whether to save. Before this PR it closed without asking and the edit was gone.
Automatic field editor — open it on several entries, use Set, Append, Clear, Rename, Copy, Move and Swap in turn. Each is one undo step, and the "x / y affected entries" notification still reports the same counts as before.
Linked files — delete a linked file from an entry: the dialog now states that undo does not restore files removed from disk. Rename one: the prompt states that undo restores the link, not the file name.
Undo still groups as before — merge two entries, manage keywords, find and replace, generate citation keys, import several files at once, merge external changes: each remains a single undo step, and Undo/Redo enablement in the menu and toolbar follows the stacks as before.
Search field — right-click in the search box and choose Undo: it still undoes typing in that box only, never a change to the library.

Still open issues for follow-up:

One journal for the whole application: undo in one library can undo an edit made in another
key accelerators not working
Saving one library reports another as unmodified, because both share the saved position
Undo Group operations not yet implemented
LibraryTab rolls back a failed Cut with a bare undo(), reverting whatever is on top instead
Keystroke-by-keystroke undo in text fields, and one queued FX refresh per edit
Partially applied change sets are logged, not shown to the user
Field editors offer no Undo item although Ctrl+Z works in them
Shared-database sync changes bypass the journal
No change model outside the GUI for jabkit dry-runs and jabsrv
The undo handle is threaded through sixteen classes that never call it. DI?
FieldChange should be converted to a record
Better names for all classes and records

Related issues and pull requests

Follow-up to #16627

AI usage

AIL3-AIL4

"Claude Code (model claude-opus-5-0)"

AI CHECKLIST.md walkthrough

1. Code self-review

Nullability and control flow

No == null / != null checks — JSpecify annotations used instead. Three Deque.peek() guards were replaced by isEmpty() + getFirst(). Two != null checks on ThreadLocal.get() remain in addEdit/applyEdit; the first is pre-existing and the second mirrors it deliberately, because a thread-local recorder has no annotated absence to express. Flagged rather than hidden.
No Objects.requireNonNull(...)
New classes annotated with @NullMarked — UndoManager, GuiUndoManager, JabRefGuiUndoManager
Optional consumed with ifPresent / map / orElseThrow
StringUtil.isBlank(...) where applicable — no new blank checks
No catch (Exception e) in added code — the one in notifyListeners predates this PR
No throw new RuntimeException(...) / IllegalStateException(...) added
Logged exceptions passed as the last logger argument

Style and idioms

[/] BibEntry withers — no new BibEntry construction
Modern Java — records, sealed types, List.of, switch patterns
[/] Precompiled Pattern — no regexes added
[/] BackgroundTask — no new threading
No commented-out code, no trivial comments, no AI-disclosure comments in source
Markdown Javadoc uses [Type] and backticks, no {@link} / {@code}

User-facing text

All user-facing text localized; the eight developer tokens that were reaching ChangeSet.name are gone
Sentence case, no trailing !, no label colons
Placeholders rather than concatenation — the two new strings take no arguments
[/] Security / XSS — no HTML response touched

Tests

Behaviour changes in logic covered — five tests added to JabRefUndoManagerTest, four of which fail without the corresponding fix (verified by reverting each)
Plain JUnit asserts, no @DisplayName, no swallowed exceptions
[/] Fetcher tests — none touched

2. Verification commands

./gradlew :jabgui:test — 1009 tests, 7 failing, all failing identically on main in this environment (clipboard and TestFX window-focus tests, plus the known KeyBindingViewModelTest)
./gradlew :jablib:test — 11034 tests, 1 failing: RemoteSetupTest.pingReturnsFalseForNoServerListening, which fails because a JabRef instance was running locally
./gradlew checkstyleMain checkstyleTest — clean, all modules
./gradlew modernizer — clean
./gradlew --no-configuration-cache :rewriteDryRun — clean; it caught a logger still named after the renamed type, fixed in its own commit
./gradlew javadoc — clean
[/] markdownlint — no Markdown changed

3. Documentation

CHANGELOG.md — two entries, one sentence each, under twenty words, end-user wording
Searched both issue trackers; no confident match, so TODO placeholders were kept
docs/requirements/ — bug fixes and refactors
Developer documentation under docs/ — architecture changed (the interface/implementation split); worth a look before merge

4. Pull request

Body built from the template, every section filled
All checklist items kept and marked
All HTML comments removed
Created with gh pr create --body-file — for the author to run
TODO placeholders in CHANGELOG.md replaced with the PR number after creation

Checklist

I own the copyright of the code submitted and I license it under the MIT license
If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
[.] 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 added one sentence (max 20 words) to CHANGELOG.md describing the change from the user's point of view (if the change is visible to the user)
[.] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

`hasChanged()` compared a net edit balance against the balance recorded at save time. Two different histories reach the same balance, because pushing a change clears the redo stack: save, undo, then make a different edit, and the balance is back where it started although the library now holds something that was never saved. That is not only a stale modified marker. `hasChanged()` feeds `LibraryTab.markChangedOrUnChanged()`, which sets `changedProperty`, which `requestClose()` consults before offering to save — so the false negative closes a library holding unsaved work without asking. Positions are now identified rather than counted. Every push takes an id from a counter that only ever increments, the id travels with the change across both stacks so redo returns to the position it came from, and `markUnchanged()` stores the id at the top of the undo stack. Because ids are never reused, a saved position discarded by a redo-stack clear or by the LIMIT trim can never be matched again, which is the right answer in both cases: it is no longer reachable. The id lives in a private `UndoJournalEntry` record rather than on `BibChange`. A change is a value describing a modification; a position in this journal is bookkeeping only the manager needs. Putting it on the change would also cost `inverted()` its involution, since it would have to either copy the id — giving two distinct positions one identity — or drop it. The empty stack needs its own id for the same reason: once a trim or a `clear()` has discarded history for good, "undone back to nothing" is no longer the state the library started in. Three tests cover the defect and fail without this change. The two existing trim tests pin the behaviour the id scheme had to preserve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

`apply()` was `change.apply(); addEdit(change);` — two separate acquisitions of this object's monitor, with the model write outside it. Between them the library holds the change and the journal does not, so an undo arriving there reverts the *previous* change while the new one stays applied but unrecorded. The history that ends up on the stack then describes a library state that never existed. The class header already stated the opposite as an invariant: "Applying the change is inside the lock. It has to be: if the stack transition and the model write could interleave, two threads could undo the same change." `undo()` and `redo()` honour it; `apply()` was the one path that did not, and commands do push from background tasks, so the two threads the header describes exist. The stack transition moves into a private `push()` that requires the monitor, and `apply()` takes it once around both the model write and the push. Applying foreign code under the lock is already the accepted cost here — `undo()` does it — so this brings one path in line rather than changing the design. Inside an `addEdit` block there is no window to close: the recorder belongs to one thread and nothing reaches the stacks until the block ends, so that path takes no lock. `CompoundEdit.apply()` keeps its two-step form for the same reason, and its javadoc now says so instead of pointing at a lock invariant that does not apply to it. The regression test asks the applying thread, from inside a `BibEntry` subclass that probes on `setField`, whether it holds the journal's monitor at that moment. Staging a second thread would only have shown that it did not get in within some interval, which makes the assertion a statement about a timeout; whether the lock is held is a fact available on the spot, and no threads, executors or waits are needed to read it. The probe overrides rather than subscribing to the entry's field events, so the test adds no use of Guava's deprecated EventBus. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

P15 stopped `apply()` from delegating to `addEdit()`: it has to hold the monitor across the model write and the stack push, and `addEdit()` ends by notifying listeners, which must never run under the lock. The branch deciding whether a change reaches the stacks at all therefore had to be restated in `apply()`, and the two methods came to read as near duplicates of one another. They are not duplicates. The only difference is who performs the change: `addEdit` records one the caller has already made — the common case, because the model hands the caller a `FieldChange` as a by-product — while the other makes it and records it as one operation. Everything after that is the same journal entry. `apply` said only that it performed the change and left the recording to its javadoc, so the pair is now `addEdit` / `applyEdit`: one verb apart, same noun, and the javadoc's first sentence states the recording outright. `CompoundEdit` carries the same pair, so code inside a recording block reads like code outside one. The empty-step guard the two share is one `isEmptyStep` helper rather than the same condition written twice with two different comments, so each method now reads as the same three-branch dispatch: hand to the enclosing recorder, skip an empty step, or lock and push. Renames only; no behaviour change, and the compiler enumerated all 28 call sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

`AutomaticFieldEditorUndoableEdit` existed to add one `int` to `CompoundEdit`, and `CompoundEdit` gave up `final` to allow it. The subtype added no behaviour: the count is what the "Automatic field editor" notification tells the user, and nothing in the undo model ever read it — a value type carrying a field for the benefit of a dialog. The count now travels as a parameter, `addEdit(CompoundEdit, int)`, to the one method that consumed it. Every call site already had it in a local variable, so the change removes a setter call rather than adding an argument to compute. `AutomaticFieldEditorUndoableEdit` is deleted and `CompoundEdit` is `final` again, leaving the change model with no inheritance at all. Recorded in the plan as the P11 follow-up, and promoted from "no behavioural gain" by the SOLID review: it is the only place where inheritance was standing in for composition in this area. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

Roughly 120 classes hold an undo handle, and almost all of them do one thing with it: edit the library and hand the change over. Undoing, redoing, asking whether the library differs from the last saved position and subscribing to stack changes are the business of five classes. Passing the whole manager to the rest handed every field editor, cleanup and import task the ability to rewrite the user's history, when all any of them does is describe what it just changed. `UndoManager` is now the recording interface — `addEdit(BibChange)`, `addEdit(String, Consumer)`, `applyEdit(BibChange)` — and `JabRefUndoManager` is the implementation, following the shape JabRef already uses for DialogService / JabRefDialogService. The class header argued against a separate recording type on the grounds that it "would mean threading a second handle everywhere the first one already goes"; that holds against a second object and not against an interface on the same one, and the header now says so. Keeping the interface under the old name is what makes this small. The ~118 recording clients keep the type name and the handle name they already had, so the diff is 21 files rather than 133, and afterburner keeps resolving injection sites because the registered key still matches the declared type. `JabRefGUI` registers the one instance under both keys, for the few classes that ask for the implementation. Only five classes call anything beyond recording: GuiUndoManager, UndoAction, RedoAction, EditAction and LibraryTab (SaveDatabaseAction reaches markUnchanged through LibraryTab's getter). Sixteen further classes hold the implementation and call nothing on it at all — they carry it to reach one of those five, which is P12's problem, not an interface gap: what they would need is undo/redo/canUndo/hasChanged, which is the whole class again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

P16 left the implementation type threaded through the GUI: twenty classes named `JabRefUndoManager`, and only five of them called anything on it. The other fifteen carried it to reach one of those five, so narrowing them meant narrowing what they carried it *to*. The layering JabRef already uses for preferences does that in one move: CliPreferences <- JabRefCliPreferences GuiPreferences extends CliPreferences JabRefGuiPreferences extends JabRefCliPreferences implements GuiPreferences UndoManager <- JabRefUndoManager GuiUndoManager extends UndoManager JabRefGuiUndoManager extends JabRefUndoManager implements GuiUndoManager `GuiUndoManager` is now an interface: the recording half it inherits, the stack controls the undo UI needs, and the two JavaFX properties the menus bind to. It declares the controls rather than inheriting them because an interface cannot inherit from a class; `JabRefUndoManager` already implements every one, and `JabRefGuiUndoManager` brings the two together. No class type is threaded anywhere now. In jabgui, `JabRefUndoManager` appears only in that extends clause, and `JabRefGuiUndoManager` only in `JabRefGUI`, which creates the single instance and registers it under both interfaces — as `Launcher` registers `JabRefGuiPreferences` under `GuiPreferences.class`. Extending rather than wrapping also removes the unwrap: `UndoAction` and `RedoAction` already received the facade and did `guiUndoManager.getUndoManager().undo()`. That accessor is gone, and with it the second object the application never wanted — `JabRefFrame` no longer builds a facade around the manager it was passed, and `MainMenu` and `MainToolBar` take one handle where they took two. This is inheritance for a layer specialization, which is why it does not contradict the P11 follow-up two commits ago: that subclass hung a dialog's counter on a value type and the owner never read it, while this adds the GUI's view of a service, the shape this codebase already uses for preferences and dialogs. The cost is that `JabRefUndoManager` is now an extension point: it stays non-final, the subclass shares its monitor, and the refresh runs from a listener the subclass registers on itself, after the monitor is released. Three tests that had to mock the class can now mock an interface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

`CompoundEdit` takes any string and `toChangeSet()` hands it on as `ChangeSet.name`, which that record's javadoc calls "the only user-facing text in the change model". Nothing converts, validates or localizes it in between, so the promotion from developer label to user text happens silently at that one call. Eight of the twenty-six step names had never been written for a reader: CHANGE_SELECTED_FIELD, APPEND_TO_SELECTED_FIELD, CLEAR_SELECTED_FIELD, COPY_FIELD_VALUE, MOVE_EDIT, SWAP_FIELD_VALUES, RENAME_EDIT, and EDIT_FIELDS — the last published as `NAMED_COMPOUND_EDITS`, a leftover of the `NamedCompoundEdit` class workstream A deleted. Each now carries the label of the control the user actually activated: Set, Append, Clear field content, Copy content, Move content, Swap content, Rename field, and the dialog's own title for the step that spans all of them. Every one of those was already a translated key, so no key is added and none retired. Four more names were hand-copies of a StandardActions label — three "Merge entries" and one "Automatically set file links" — and now come from the action, which is where the text the user just clicked already lives and where it will stay correct when the menu wording changes. Nothing renders these yet: the sole reader is the warning ChangeSet logs when part of a set fails to apply. P5 is what will show them, and this is its prerequisite — a name written for a log is a name written for a person, or P5 ships "Could not fully apply CHANGE_SELECTED_FIELD". The javadoc on both types now states that rule instead of leaving it to the field's type. `AutomaticFieldEditorViewModel#cancelChanges` is documented rather than changed: it reverts what the tabs already wrote and records nothing, which is correct because nothing reaches the stack until OK — but it is the one deliberate write outside the journal and read like the defect `applyEdit` exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

Three step names described the command the user had just invoked, in wording that differed from the command's own label: "Autogenerate citation keys" -> GENERATE_CITE_KEYS "Generate citation keys" "Update keywords" -> MANAGE_KEYWORDS "Manage keywords" "Replace string" -> REPLACE_ALL "Find and replace" Each now takes its text from the action, so the undo step says what the menu item said, and stays right when the menu wording changes. That also settles the casing drift, which was the visible symptom: "Replace string" beside a dialog titled "Replace String" beside a menu entry reading "Find and replace", for one operation. A fourth, "duplicate removal", is deliberately left alone, because the rule is not "use the action label" but "name what will be reversed". Find duplicates only searches; the removals and merges come from the user's decisions about each pair afterwards, so the action's label describes something no undo can take back. A comment at the call site says so, since the substitution looks obvious until you read the step. "Update keywords" existed only as a step name, so its key is removed from the English properties file — the same handling A3 gave the 19 presentation-name keys it retired, with the translations left to Crowdin. The other keys stay: KeyBinding still uses them. Nothing rendered these names before this commit and nothing renders them after it, so no user-visible text changes today. What changes is that P5 can surface any of them without first having to ask whether this particular one was written for a person. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

Undo and Redo in `EditorContextAction` act on the text control's own stack. That is correct for the search field, whose text is not a value of the library, and wrong for a field editor, where `Ctrl+Z` is routed to the library's journal by the filter in `FieldEditorFX` (#11420): a menu Undo would revert the control's text, the text listener would see that as a fresh edit, and the undo the user asked for would land on the stack as a new forward change. `getDefaultContextMenuItems` has left both items out since 2019 (d1307a8), so the hazard is already avoided — but the avoidance rested on a parenthesis reading "(except undo/redo)", with nothing to tell the next contributor why the omission is deliberate or what to do instead. Both javadocs now say it. No behaviour change; the two items are still built for the search field, which is the one control they suit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

Undo covers the bibliography, not the file system (decision D7). Two operations left the user to discover that for themselves. Deleting a linked file removes it from disk and journals the entry's `file` field, so undo brings the link back and points it at a file that is no longer there. Renaming one is worse in a quieter way: the rename happens on disk, only the link is journalled, so undoing restores the old link — a name no file has any more. Both already ask before acting, so both now say so at the point where the user decides, rather than notifying afterwards about something already done. Not covered, deliberately: renaming to the suggested name, moving to a directory, downloading, and the file cleanups have no decision point to attach a sentence to, and a toast on every such operation is noise that teaches people to dismiss toasts. The cleanup that renames PDFs already asks its own "does not support undo" question, which is the pattern to follow if the others ever grow one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

One sentence each, under twenty words, starting with "We fixed" and "We changed", with no internals named. The links were pointing at #16627, the already-merged predecessor of this branch; they are TODO placeholders until this pull request has a number, per AGENTS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

`Deque.peek()` answers "empty" with `null`, so reading the top of a stack meant a null check in three places. `isEmpty()` plus `getFirst()` says the same thing without one, and keeps the order that undo needs: the inverse is applied before the entry moves across, so a change that throws stays undoable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

The logger still named `UndoManager`, which is now the interface, so every message from the journal was tagged with a type that has no implementation of its own. Caught by `rewriteDryRun`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

`MainTableColumnModel` asked the injector for an undo journal in its constructor, although only `getDisplayName()` needs one, and only for a special field's label. Preference migrations run in `Launcher` before `JabRefGUI` registers anything, and they build column models. While `UndoManager` was a class, afterburner quietly reflected a throwaway instance into existence; now that it is an interface, the same call throws "Cannot instantiate view" and takes the startup down with it — for the users whose stored preferences still need that migration. Moving the lookup to the point of use fixes the crash and removes a dependency a column-preference value object never had a reason to hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

Adapts DatabaseChangeMonitorTest from main to the UndoManager interface split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFPhW8W2qKVhhhRz7mYwPD

koppor

🤖 Generated with Claude Code

Read the journal, the GUI layer and all call sites in full; ran the undo/edit/collab suites locally on the branch merged with current main.

Required

Semantic conflict with main. The merge with main is textually clean (the "conflicts" flag on this PR is stale), but main gained DatabaseChangeMonitorTest (#16698) after the last merge here, and it does new UndoManager() twice — which no longer compiles once UndoManager is an interface, so :jabgui:compileTestJava fails on the merged tree. I pushed the merge of main to this branch (1e1851a) with that test adapted to JabRefUndoManager (the mock(UndoManager.class) calls stay as they are). With it, JabRefUndoManagerTest 31/31, JabRefGuiUndoManagerTest 4/4, DatabaseChangeMonitorTest 9/9 and all gui.edit.* / gui.collab.* tests pass.

Verified

Saved-position identity: ids are never reused, a redo-stack clear makes a saved id unreachable, the LIMIT trim hands its id to the empty stack, clear() mints a fresh one — all under the monitor. The trim boundary case is covered by undoingBackToASavedPositionTheLimitDroppedReportsUnchanged.
applyEdit under one acquisition: undo/redo already applied under the lock before this PR, so the "no FX wait inside BibChange.apply" constraint is not new. No BibChange implementation touches JavaFX, no @Subscribe handler in jabgui blocks on the FX thread, and there is no lock-order cycle with LibraryTab.markChangedOrUnChanged (no event subscriber reaches it).
Every apply( call site is renamed; removing AutomaticFieldEditorUndoableEdit keeps the "x / y affected entries" counts; CompoundEdit is final again.

Suggestions (non-blocking)

GuiUndoManager restates eight signatures the journal already has (the ADR lists this as a con). A jablib interface — say UndoJournal extends UndoManager with undo/redo/canUndo/canRedo/hasChanged/markUnchanged/clear/addListener, implemented by JabRefUndoManager — lets GuiUndoManager extends UndoJournal add only the two properties, drops the "an interface cannot inherit from a class" workaround, and gives jabkit/jabsrv a typed control surface later.
Two references point at a plan that is not in the repository: "see P21 in the undo plan" (JabRefUndoManager#applyEdit javadoc) and "The defect P14 fixes" (JabRefUndoManagerTest). Either drop them or commit the plan.
JabRefUndoManager#applyEdit while a block is active on the same thread has no test — applyingInsideABlockRecordsIntoThatStep goes through edit.applyEdit, not the manager. pushInsideABlockJoinsIt covers the addEdit twin; one mirror test would close it.
JabRefGUI.undoManager could be typed GuiUndoManager; the ADR says the concrete type is named there only for construction.
Nit: the renamed type in the MainMenu ToDo comment ("mark … the GuiUndoManager as dirty") adds nothing.

Everything else raised by Qodo/Copilot is either already addressed on the branch or explicitly deferred in the description.

koppor · 2026-08-28T02:53:20Z

+* `UndoManager` (jablib) — the recording interface: what about 120 classes depend on, and all they can do.
+* `JabRefUndoManager` (jablib) — the journal: stacks, undo, redo, saved position. Plain Java, no toolkit.


The naming is not good

first one is ...jabref... undomanager

second one is ...jabref... jabrefundomanager

I think, the first one should be renamed to RecordingUndoManager to distinguish the classes by name while using.

The wording in docs/decisions/0070-split-the-undo-manager-into-recording-and-gui-layers.md could be "more Software Engineering: professional, neutral tone".

UndoManager is the interface, JabRefUndoManager the implementation. I followed your design of CliPreferences and JabRefCliPreferences.

Renaming the Interface means changes in almost 2/3 files in JabGui

Two javadoc comments pointed at a planning document that is not in the repository — "see P21 in the undo plan" and "the defect P14 fixes". A citation a reader cannot follow is worse than none, so both now state the point instead of naming it: that reserving a command's writes against undo has to happen above the journal, since only the command knows when its block ends, and that counting edits cannot distinguish two histories that arrive at the same balance. `applyEdit` called on the manager while a block is active on the same thread had no test. The existing pair covers the recorder's own `applyEdit` and the manager's `addEdit`; this is the fourth corner, and it is the branch this series added. `JabRefGUI` declared the implementation type for a field it only constructs, registers and hands on. The interface says what it uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1

calixtus · 2026-08-29T11:01:48Z

About UndoJournal: compiler will catch any mismatches with names for now. Future benefit for JabKit/JabSrv will be done will be done in future. This is not the last PR in this series. But to keep things managable and reviewable, good old german: step after step. Also interface is not yet in final state. Still architectural changes required to implement per-library journal. Therefor postponed.

About comment: UndoManager is now the recording interface and has no dirty state.

@Inject

…abRef#16857) * Drop the two unused dependencies from FieldsUtil.getNameWithType getNameWithType(field, preferences, undoManager) built a SpecialFieldViewModel only to read getLocalization(), which is getAction().getText() — and SpecialFieldViewModel.getAction(SpecialField) is static. Neither the preferences nor the undo manager was ever read for the label. Where they came from, since it explains why this is worth a commit rather than tidying. JabRef#6031 had to translate special field names, the translation already lived on the view model, so the cheapest way to reach it was to construct one — with Globals.undoManager, a static global that cost the call site nothing. JabRef#11342 ("DI Part A") deleted Globals and made the constructor's dependencies explicit, so a string lookup grew two parameters; the translation was mechanical and correct, but nobody re-asked whether the label needed them. JabRef#16273 then added the static getAction(SpecialField) and FieldsUtil.getDisplayName(Field), which made the label reachable with no instance at all, while getNameWithType kept the old route — two ways to compute one label in one file. The label now comes from getDisplayName(Field), so the two cannot drift, and getNameWithType has the signature it had before Globals went. SpecialFieldViewModel is unchanged: it earns those dependencies for getSpecialFieldAction, which writes the field onto entries. The fault was in the caller. This retires a workaround rather than deferring it. MainTableColumnModel resolved UndoManager from the injector only to forward it here, and that lookup crashed startup once UndoManager became an interface in JabRef#16680, because preference migrations build column models before JabRefGUI registers anything. JabRef#16680 fixed the crash by moving the lookup into getDisplayName(), which left a value object asking the service locator once per rendered cell, since nameProperty() builds a fresh wrapper per call. Both lookups are gone now, together with the CliPreferences field that existed only to be forwarded, so the class no longer references Injector at all. SaveOrderConfigPanel loses both @Inject fields and the jakarta.inject.Inject import; XmpPrivacyTab loses its injector field and import. MainTableColumnModelTest no longer needs the @BeforeAll that registered a mock CliPreferences to satisfy the constructor, and gains a test that builds a special-field column with nothing registered at all and asserts the rendered label — the startup crash pinned as a test rather than as a comment. No behaviour change and no CHANGELOG entry: the rendered label is byte-identical, which is also why no existing test needed changing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Make a failed Cut leave the library alone instead of undoing the top of the stack cutEntry copied to the clipboard, deleted the entries whether or not that copy had succeeded, and then compared the two counts: on a mismatch it called a bare undoManager.undo() to put the entries back. That reverts whatever is on top of the journal, which is this deletion only if nothing else pushed in between — an import or a cleanup finishing at that moment was undone instead, and the entries stayed deleted. The deletion also moved to the redo stack rather than being discarded, so the next redo re-applied a cut the user had just been told failed. The copy already ran first, so the fix is to return when it fails: nothing is deleted and there is nothing to roll back. Recording the cut as one invertible step would also have worked, but an early return puts the invariant in the shape of the method rather than in a compensating action that has to be got right. The same branch also called clipBoardManager.setContent(""). The IOException it reacts to comes from serializeEntries, which runs before the clipboard is written (ClipBoardManager:186,189), so the clipboard still held whatever the user had put there earlier; the failure handler discarded content this cut never owned. This is latent rather than live, and no CHANGELOG entry goes with it. The branch cannot currently be reached: entriesCopied is negative only when ClipBoardManager.setContent throws IOException, which comes from BibEntryWriter.write(List, mode) writing into a StringWriter, which does not throw. Nor can the counts differ another way — both calls take the same selection, getSelectedEntries() being mainTable.getSelectedEntries(), and doDeleteEntry returns -1 only in DELETE_ENTRY mode, never for CUT. Observable behaviour is therefore unchanged, including the empty-selection path, which still notifies "Cut 0 entry(s)". What the change buys is that "compensate for a write by undoing whatever is on top" is no longer in the codebase to be copied or made reachable. No test: LibraryTab is not constructible in a test, every test that mentions it mocks it, and the trigger is an IOException a StringWriter cannot produce. undo() was LibraryTab's last direct call on the undo manager beyond hasChanged(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Let Undo and Redo take the journal from the active library Groundwork for one journal per library. Nothing changes yet, because one journal still serves every library; what changes is who decides which journal an undo acts on. UndoAction and RedoAction held a GuiUndoManager handed to them at construction. There are three instances of each — main menu, toolbar, entry editor — all built once during startup, and each of them serves every library the session opens. A held journal is therefore the wrong shape even before there is more than one: which journal an undo belongs to is a property of the moment the user presses the button, not of the moment the button was built. Both actions already receive a Supplier<LibraryTab> for exactly this reason, and LibraryTab already exposes the journal, so the action now asks it when it runs. Enablement follows the same route through StateManager.activeTabProperty(), which JabRefFrame already uses to bind canGoBackProperty and canGoForwardProperty per tab. The bindings are equivalent while a single journal is shared. They differ only if an active database can exist without an active tab, in which case undo is now disabled instead of enabled over a tab that is not there; the two state properties are set together (LibraryTab:381-383, JabRefFrame:428-429), so this is not reachable. UndoRedoActionTest gives each of two mocked tabs its own journal and asserts that undo, redo and enablement follow the active one. Those assertions describe what per-library journals will mean; today they hold because the action reads through the tab rather than because production has two journals. They are the guard for the step that gives each LibraryTab its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Drop EditAction's undo manager and the two branches that used it EditAction took a GuiUndoManager for its UNDO and REDO cases, and nothing ever selects them. All sixteen construction sites of org.jabref.gui.edit.EditAction — MainMenu, MainToolBar, RightClickMenu and MainTable, four each — pass CUT, COPY, PASTE or DELETE_ENTRY. (The single-argument `new EditAction(StandardActions.CUT)` calls in SourceTab and PreviewTab construct those classes' own private nested EditAction, not this one.) So both branches are unreachable, and with them the dependency: the entry branch drove the journal directly, which was a third undo path beside UndoAction and RedoAction, and the text-control branch called textInputControl.undo(), the control's private stack that JabRef#16680 documented as the wrong target for a value of the library. Removing them leaves the switch's `default` to report an unexpected action loudly rather than let a future UNDO silently take either wrong route. The class javadoc described dispatch through Globals.focusListener, which has not existed since JabRef#11342 removed Globals; it now describes the focus dispatch the code actually does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Let callers ask the state manager for the journal of a named library First of three steps towards one undo journal per library. This one adds the seam and changes no behaviour: getUndoManager(context) and getGuiUndoManager(context) return the single journal the application already shares, so every library still resolves to the same one. The state manager is where this belongs. It already holds what is per-library and per-session — the open databases, the active one, the selected groups, the search contexts — and every class that will need to resolve a journal already receives it. Eight of the eleven actions that hold a journal today also already call stateManager.getActiveDatabase() inside execute(), so for them the conversion is the removal of a constructor parameter rather than the addition of one. Resolving by library, rather than by "the active tab", is what makes this correct for the five collectors that gather on a background thread and push when they finish — ImportHandler, BatchEntryMergeTask, AutoLinkFilesAction, GenerateCitationKeyAction and LookupIdentifierAction. The library they record against is the one they started on, which the context names and the active tab does not: the user may have switched tabs while the task ran. The state manager takes the journal rather than creating it, so that it hands out the same one JabRefGUI registers with the injector while the conversion is under way. The no-argument constructor keeps its own, which is what the six tests that build a state manager use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Resolve the journal from the library each action acts on Second of three steps towards one journal per library, and again no behaviour change: every context still resolves to the one shared journal. Nine classes stopped holding a journal handed to them when they were built and now ask the state manager for the journal of the library they are about to change: JabRefFrameViewModel, CleanupAction, GenerateCitationKeyAction, AutoLinkFilesAction, MergeEntriesAction, MergeWithFetchedEntryAction, UpdateWithBibliographicInformationByWebFetchers, BatchEntryMergeWithFetchedDataAction and MergeLibraryAction. Each already determined that library inside execute(), so the constructor parameter and the field go and nothing takes their place. JabRefFrameViewModel.newImportHandler is the clearest case: the context was already its parameter. GenerateCitationKeyAction also stops reading the active library from inside the background task. It reads it where the task is built, on the JavaFX thread, and generates and records against that one. This is a fix, not a refactor: the keys were generated for whichever library was active when the task got around to running, so switching tabs during a long generation wrote keys into the wrong library. It is unreported and hard to hit, and it is why the journal has to be keyed by library rather than by "the active tab" — the five collectors that push when a background task finishes all have this shape. AutoLinkFilesAction likewise records against the library captured at the start of execute() rather than whatever is active when the linking finishes. MainMenu, MainToolBar, RightClickMenu, MainTable and JabRefFrame stop passing a journal at fourteen construction sites. They still hold one for the classes not yet converted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Resolve the journal per library in the remaining menu and side pane actions Continues the second step of journal by library; still no behaviour change, since every context resolves to the one shared journal. AutomaticFieldEditorAction, LookupIdentifierAction, GroupTreeView and OpenOfficePanel's "Cite" now ask the state manager for the journal of the library they are changing, and stop holding one. Each had the library at hand already: the active database for the first two, the drop target's database in GroupTreeView, and the context the citation keys are generated against in OpenOfficePanel. LookupIdentifierAction reads the library where the background task is built rather than where it finishes, as GenerateCitationKeyAction now does, for the same reason: the entries belong to the library the lookup started on. OpenOfficePanel keeps its journal for one more call — it hands it to createLibraryTab, which still takes one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Record a special field edit in the library of the row it was made on A main table column serves rows of one library, but the global search results table mixes entries from every open library into one table and passes a synthetic context that belongs to no library at all. Both were handed a single journal when the column was built. That is invisible while one journal serves everything, and becomes wrong the moment journals are per library: an edit made in the global search results would be recorded against a context no tab owns, and would be undoable from nowhere. SpecialFieldColumn and ContentSelectorColumn now take the state manager and resolve the journal from the row they are writing to, which they already have: BibEntryTableViewModel carries its BibDatabaseContext. The normal table is then correct by construction rather than by holding a single library. Two dependencies turned out not to exist at all: - CellFactory built six SpecialFieldViewModels only to read getIcon(), which resolves through the static SpecialFieldViewModel.getAction. Same shape as the getNameWithType finding: the journal was never read. It takes only preferences now. The column header and the empty-value icon in SpecialFieldColumn come from the same static action. - GlobalSearchBar held a journal only to build GlobalSearchResultDialog, which held it only to build a GlobalSearchBar and the results table. With the table no longer taking one, the whole cycle disappears: the global search path records nothing itself. ContentSelectorColumn also loses a branch that applied the change without recording it when the journal was null. Neither of the two call sites can pass null, and a resolved journal never is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Hand the field editors and the backup restore a journal instead of looking one up Continues the second step of P18. Three classes stop reaching for the journal through the service locator or holding one they were handed. KeywordsEditor and GroupsEditor called Injector.instantiateModelOrService for it, which is the pattern the getNameWithType change removed elsewhere: the journal they need belongs to the library whose entry is being edited, and FieldEditors, which builds them, already receives that library's journal and passes it to every other editor. They now take it like their siblings do. BackupUIManager resolves the journal from the library it is restoring into. It runs before a tab exists for that library, so it cannot take one from a tab, but it loads the context itself and can name it. It already had the state manager. TagsEditorTest failed once in a batch run here and passed on three reruns and in isolation; the same batch on the parent commit fails eight network-dependent tests instead and not that one. IdentifierEditorTest is one of the five known environment failures on this machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Give every library its own undo journal Completes P18. Until now one journal served every open library, which had two live consequences: Ctrl+Z in library B popped whatever was on top of the shared stack, applying an edit made in library A to A's entries; and saving A stamped the shared saved position, so B reported itself unchanged while holding unsaved edits. No position scheme could fix the second, because identifying positions correctly in a stack that mixes two libraries still answers the wrong question. The fix is ownership. JabRefGuiStateManager now keeps one journal per library and hands it out by context. The map is keyed by BibDatabaseContext.getUid() rather than by the context, as selectedGroups already is: a context's hashCode changes whenever an entry is added, so a map keyed by the context itself loses its entries — its own javadoc says as much. It is concurrent because changes are recorded from background tasks as well as from the JavaFX thread. Three further things follow from ownership: - Closing a library discards its journal, and with it the changes and the entries those changes kept alive for the rest of the session. The GuiUndoManager.clear() that nothing could call is not needed: the journal goes when the library does. - The stack limit is now per library. LIMIT was already a constant rather than a preference; the ceiling is libraries x LIMIT. - LibraryTab resolves its journal on each call instead of holding one, because the context a tab shows is replaced when loading finishes. MainTable and RightClickMenu do the same rather than caching what the tab had at construction, which for a tab still loading would have been the dummy context's journal. The journal is no longer registered with the injector and is no longer threaded through the frame: JabRefFrame, MainMenu, MainToolBar, the SidePane chain, WelcomeTab, OpenDatabaseAction, SharedDatabaseUIManager, OpenOfficePanel and the shared-database login dialog all stop carrying one. Sixteen of those classes never called anything on it — the pass-through tax P12 describes, paid off here as a side effect rather than deferred again. Two more classes stopped needing a journal at all, both the same shape as the getNameWithType finding: SpecialFieldMenuItemFactory built view models only to read a menu label and icon, which resolve through the static getAction, and SpecialFieldAction now takes the journal from the tab it acts on, which it already uses for markBaseChanged. ImportEntriesDialog resolves the journal of the library it imports into instead of injecting one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Hand out the recording half of a library's journal, not the whole manager LibraryTab.getUndoManager() returned GuiUndoManager to every caller. Of the twenty-one live call sites, fifteen only record a change they have already made — directly through addEdit or applyEdit, or by passing the journal to ImportHandler, DatabaseChangeMonitor or ChangeEntryTypeMenu, all three of which declare the narrow UndoManager. Handing those the full manager gives every field editor, cleanup and import task the ability to rewrite the user's history, which is the defect P16 removed at the type level everywhere else. Routing more callers through the tab for per-library journals had widened it. The accessor now returns UndoManager, and getGuiUndoManager() serves the six that drive the stacks: UndoAction and RedoAction, SaveDatabaseAction#markUnchanged, and the tab's own hasChanged. Subtyping means the fifteen recording sites are unchanged. The journal is still resolved on each call rather than held, because the context a tab shows is replaced when loading finishes. Closing the library is the exception: resolving creates the journal when it is missing, so a caller holding a closed tab would have put a fresh one back into a map that nothing clears again. The tab keeps the journal it had at close and answers with that, logging a warning. Nothing returns to the long-lived map and the journal goes when the tab does, rather than throwing at a caller we have no evidence of. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Give a library one journal accessor and let the stack drivers name their library The previous commit put getUndoManager and getGuiUndoManager side by side on LibraryTab, returning the same object under two static types. That is naming a method after its return type, and it segregates nothing: any caller can reach for the wider one, and the fifteen recording call sites were already declaring the narrow type where it counts — as the parameter of ImportHandler, DatabaseChangeMonitor and ChangeEntryTypeMenu, or by calling addEdit on the result and declaring nothing at all. So the tab keeps a single accessor, the recording one, and the six callers that drive the stacks ask the state manager for the journal of a named library. UndoAction, RedoAction and SaveDatabaseAction all already hold a state manager, so this costs no new dependency, and it puts "this drives the stacks of that library" at the call site rather than hiding it behind an accessor a recorder could also call. The close guard stays, now on the tab's private resolver. Two things fixed while in there, both raised in the review of this branch: - UndoAction and RedoAction resolved the tab from a Supplier<LibraryTab> but bound enablement through the state manager: one fact, two paths. JabRefFrame's supplier returns null when no library tab is selected, so execute() could dereference it. They now read activeTabProperty for both, and executing with nothing open is a no-op rather than a NullPointerException. - The enablement binding was copy-pasted between the two. It is now ActionHelper.needsUndo and needsRedo, beside needsDatabase and the other per-library predicates. UndoRedoActionTest stubs the journals by library rather than by tab, and gives the two libraries distinct paths on purpose: BibDatabaseContext.equals compares content, so two empty libraries are equal and one stub would answer for both. The test failed exactly that way before the paths were added, which is the same hazard that makes the journals keyed by uid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Resolve the library before starting the key generation and the identifier lookup Two findings from the review of this branch, both in code it introduced. GenerateCitationKeyAction could throw a NullPointerException. Its background task built the CompoundEdit inside databaseContext.ifPresent(...) in call(), while onSuccess called compound.hasEdits() unguarded, so with no active library the field was still null when the task finished. The shape predates this branch — the old code had the same ifPresent — but the branch is where the library resolution moved, so it is fixed here: execute() resolves the library and returns if there is none, generateKeysInBackground takes it as a parameter, and the CompoundEdit is final and built with the task. Nothing in the task is conditional on a library any more, because the task is not built without one. LookupIdentifierAction passed an Optional<BibDatabaseContext> as a parameter, which Effective Java Item 55 argues against: Optional is a return type, and as a parameter it makes every caller wrap and the body unwrap. Same fix — the guard moves to execute(), and lookupIdentifiers takes the library itself. Both guards are unreachable from the UI, since both actions bind their enablement to needsDatabase, but neither class enforced that on its own. Neither class has a test; verified by compiling, checkstyle and reading. Testing either would mean standing up a background task, a task executor and a tab supplier for what is now an early return. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * State the per-library undo history as a requirement The undo requirements said the opposite of what the code now does: "one journal currently serves the whole application: with several libraries open, the saved position of one is the saved position of all. Closing that gap is separate work and is not covered here." This branch closed that gap, so the caveat goes and the behaviour it deferred is written down as req~logic.undo.journal-per-library~1: each library has its own history, undo acts on the library being worked in, saving one library sets only its own saved position, closing one discards its history, and a task that finishes after the user switched libraries still records against the library it ran on. Traced the way the two existing undo requirements are: an impl tag on the registry that keys journals by library, and utest tags on the three tests that cover separation, close, and undo following the active library. The confirmation section of ADR 0070 named JabRefGUI as the class that creates the single instance. That is how it read when the decision was taken; as a description of what to check today it was wrong, so it now names JabRefGuiStateManager and one journal per library. The decision itself is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * CHANGELOG.md * CHANGELOG.md * Give the entry editor and the dialogs the journal of the library they edit Fixes the defect the automated reviews found: sixteen classes carried `@Inject private UndoManager` while nothing registers one with the injector any more. Afterburner falls back to instantiating the type when a key is missing (Injector:110), and the default supplier calls newInstance(), which cannot instantiate an interface — so opening the entry editor, a field editor, the library properties dialogs, "Manage keywords", the unlinked files wizard or the related work dialog would have thrown. This was mine to catch. Removing the registrations, I grepped instantiateModelOrService and concluded nothing read the journal from the injector, without grepping @Inject. Compile, checkstyle and the test suites all stayed green because none of them load a view through ViewLoader — the tests that touch these classes construct them with new, so the injected field is simply never populated. Restoring a registration would have undone P18, so each site names its library instead: - The entry editor chain resolves at the point where the library is known and current. FieldsEditorTab already has the context of the entry being shown and asks for that library's journal; the ten field editors take it as a constructor parameter, as KeywordsEditor and GroupsEditor already did. EntryEditor, EntryEditorTabFactory, AllFieldsTab, UserDefinedFieldsTab, SourceTab, CitationRelationsTab and CitationsRelationsTabViewModel stop carrying one. This matters beyond the crash: EntryEditor is a singleton reused by every library, so a journal captured when it was built would have been one library's forever. - The library properties views name the library they were opened for, which is their constructor argument. - ManageKeywordsDialog, UnlinkedFilesWizard and RelatedWorkResultDialogView act on the active library and now ask the state manager for it. @NullMarked added to the two actions rewritten here and to the new test, per the convention the project is migrating towards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Record a vanished shared entry against the library it was shared from SharedDatabaseUIManager listens to the synchronizer of the library it opened, but when that library lost an entry on the server it recorded the removal against tabContainer.getCurrentLibraryTab() — whichever library happened to be in front. It also built the UndoableRemoveEntries over that tab's database rather than the shared one, so with a second library active the change described entries the recorded database does not contain. Wrong database is older than this branch; wrong journal is new, because until each library had its own journal there was only one to record into. Found by the automated review of the pull request. The manager now keeps the context it opened, assigned where it already assigns the synchronizer it listens to, and records against that. The null check on the current tab goes with it: the removal no longer depends on a tab being selected, and the dialog and selection reset that followed it are about the user's session rather than about which library the event came from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Copy the selected entries before handing them to a background task StateManager.getSelectedEntries() returns the list itself, and setSelectedEntries replaces its contents when the user switches libraries. Three actions passed that list into a background task: the task then walked whatever the selection had become, while recording into the journal of the library it started on. Since each library now has its own journal, that means entries of one library recorded in another's history. Found by the automated review of the pull request. GenerateCitationKeyAction, AutoLinkFilesAction and LookupIdentifierAction now take a copy while still on the JavaFX thread. LookupIdentifierAction read the selection inside the background lambda, so it also stops reading it late. GenerateCitationKeyAction takes a mutable copy on purpose: checkOverwriteKeysChosen prunes the entries that already have a key, and until now it pruned them from the user's selection in the main table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Drop the journal of the placeholder library a tab shows while loading A tab opened for a file that still has to be parsed shows a dummy context first, and building its components asks that context for a journal. setDatabaseContext then swaps in the parsed library, leaving the dummy's journal in the registry with nothing able to reach it — one entry per asynchronously opened library, for the rest of the session. Found by the automated review of the pull request. The swap now drops it, next to the line that already removes the dummy from the open databases. The second half of that review point — a caller naming a library after it closed gets a fresh journal from computeIfAbsent — is not closed here, and the state manager now says so. Gating creation on the library being open does not work: a tab asks for its journal while it is being built, which is before the context reaches getOpenDatabases (LibraryTab:381 runs before :383). What limits it is that the tab answers with the journal the library had rather than asking again, and that every converted recorder resolves its journal while the library is open and then holds that object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Consume the active library as an Optional instead of testing and unwrapping it CHECKLIST.md asks for Optional to be consumed with ifPresent, map or orElseThrow, never an isPresent()/get() pair, and this branch had introduced nine such pairs while converting actions to name the library they record against. Each one guarded a method whose whole body is the "a library is open" case, so the body becomes a method taking the library and execute() hands it over: CleanupAction, MergeEntriesAction, BatchEntryMergeWithFetchedDataAction, MergeWithFetchedEntryAction, UpdateWithBibliographicInformationByWebFetchers, GenerateCitationKeyAction, LookupIdentifierAction, UndoAction and RedoAction. OpenOfficePanel's "Cite" returns through map instead of unwrapping inside the branch. UpdateWithBibliographicInformationByWebFetchers also drops an `assert` that guarded a get(). Assertions are off unless the JVM is started with -ea, so it documented an expectation rather than enforcing one. The .get() calls left in these files are older than this branch and are left alone. The state manager's javadoc still said one journal serves every open library, which was true when the accessor was introduced two commits before the flip and false afterwards. Found by the automated review of the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Add the changelog entries for per-library undo The two user-visible fixes of this branch: undo no longer crosses libraries, and saving one library no longer clears another's modified marker. Linked to the pull request, since no issue was confidently matched — searching the open issues for "undo" turned up JabRef#7557 and JabRef#8770, and neither is this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Let the state manager hand out a library's journal through one accessor getUndoManager and getGuiUndoManager sat next to each other returning the same object under two static types — the same ceremony that was removed from LibraryTab two commits ago and then repeated here. A caller wanting the wide type could simply ask for it, so the pair segregated nothing; and the narrow return bought nothing either, since a recorder either calls addEdit straight on the result, declaring no type at all, or passes it to a parameter that already says UndoManager. The registry keeps one accessor and hands out the whole journal, because owning it is what it does. Narrowing stays where it is enforced: the parameter a recorder declares, and LibraryTab#getUndoManager, which hands a tab's collaborators the recording half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Inline and Fail Fast * Undo changelog duplication * Take a library's journal while it is open, not when the task finishes The review of the pull request pointed out that a closed library's journal can be resurrected: getUndoManager creates on demand, so a background task that asks for one after LibraryTab.onClosed dropped it puts an orphan back in the registry, holding the closed library's entries for the session and recording into a journal no undo can reach. The commit that removed the dummy-context leak claimed this was limited by every converted recorder resolving its journal while the library is open and then holding that object. That was wrong, and the review named the three exceptions: GenerateCitationKeyAction, AutoLinkFilesAction and LookupIdentifierAction all called getUndoManager at push time, on the background thread, after the work had finished. All three now take the journal where they already take the library and the selection — on the JavaFX thread, before the task starts — and hold it. The statement in the earlier commit is true of the code as it now stands. The registry itself still creates on demand, so naming a closed library there still produces a journal. What is left after this is a caller that resolves late, and there is none in the paths this branch converted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Shorten the per-library undo requirement Review feedback on the pull request: the description spelled out consequences that follow from the main idea. Two sentences instead of five, keeping what is not inferable — that a change belongs to the library it was made in rather than to the one in front when it is recorded, which is what the background tasks had to be changed for, and that closing a library discards its history. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Shorten the other two undo requirements Same review feedback as the per-library one: state the rule, let the consequences follow from it. The saved-position requirement drops the worked example of the redo-stack clear and the stack-limit trim, keeping identity-not-distance and why a wrong answer costs the user work. The atomicity requirement folds its consequence into the sentence that states the rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Remove obvious comments * Say where the selection list comes from, and assert each step of the undo tests Review feedback on the pull request. The comments explaining that the state manager's selection is a live list were removed as noise, correctly: the same paragraph stood at three call sites. The fact itself is worth keeping once, so it is now on getSelectedEntries, where the next caller meets it and next to the setter that replaces the contents. The javadoc of getUndoManager said "the undo journal of `context`" while describing libraries; it now says which of the two the parameter is. Both cross-library tests acted twice and asserted once at the end, so a failure did not say which step broke. Each now asserts after acting: undo in the library in front, check the other is untouched, switch, undo again, check both. They also cover one step more than before — that the library switched away from stays where it was left. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Hand LinkedFilesEditor the journal of the library it edits A reviewer hit the exception this branch was supposed to have removed: Cannot instantiate view: interface org.jabref.logic.undo.UndoManager at Injector.instantiateModelOrService(Injector.java:111) ... LinkedFilesEditor.<init>(LinkedFilesEditor.java:124) LinkedFilesEditor is a seventeenth class injecting the journal, missed when the other sixteen were converted because its annotation sits on its own line and the grep that found the others looked for "@Inject private UndoManager" on one. It now takes the journal from FieldEditors like every other field editor. It only shows for an entry with a `file` field, which is why it survived the entry editor being opened during review of the earlier fix. AllFieldsTabTest stubs the state manager's journal, since FieldsEditorTab resolves it from there. It also keeps registering an UndoManager with the injector: removing that made TagsEditorTest fail in the same test run while passing alone, so something else in that JVM still relies on the registration being there. Worth its own look; this branch is not the place. Verified: fieldeditors, entryeditor and undo, 144 tests, only the IdentifierEditorTest failure this machine has with or without these changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Format * Reword the note on the selection list One sentence carried three facts and an ambiguous "which". Now: what the method returns, what changes it and when, and what a caller has to do about it — one point each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Make an accepted external metadata change undoable Accepting external changes records every kind but one. MetadataChange took the CompoundEdit it was handed and never used it: it set the new metadata on the context directly, under a "TODO: Metadata edit should be undoable". So accepting a change to a library's own settings — mode, encoding, citation key pattern, file directories, save actions, content selectors, protection — could not be taken back, while accepting an entry or string change beside it could. This is the defect the record model exists to prevent, the same one that left keyword management, file import and "Replace string" silently unrecorded. UndoableMetaDataChange holds the context and both metadata objects and swaps them, which is what applying already did; inverting swaps them back. The groups root is pinned to its original value before the record is built rather than after the metadata is installed, so redo does it too and does not fight GroupChange, which owns the groups. BibChangeTest covers it with the others: inverting twice is identity, and undoing puts the previous settings object back. Note for whoever picks up the metadata work: setMetaData replaces the object, and LibraryTab and CoarseChangeFilter register their listeners on the instance (LibraryTab:260, CoarseChangeFilter:31). After a swap they listen to the object that was replaced. That predates this change and is not altered by it — undo happens to restore the instance they are attached to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194MmVeMweN5EKQEx5yNoC1 * Remove guard on orphaned journals after library closed * Reorder methods --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

calixtus and others added 14 commits August 25, 2026 13:48

calixtus added the dev: code-quality Issues related to code or architecture decisions label Aug 25, 2026

Merge branch 'main' into fix-undoredomore

4455861

github-actions Bot removed the component: internationalization i18n label Aug 27, 2026

koppor and others added 2 commits August 28, 2026 03:05

Merge branch 'main' into fix-undoredomore

bde67af

Merge branch 'main' into fix-undoredomore

1e1851a

Adapts DatabaseChangeMonitorTest from main to the UndoManager interface split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFPhW8W2qKVhhhRz7mYwPD

koppor reviewed Aug 28, 2026

View reviewed changes

koppor requested changes Aug 28, 2026

View reviewed changes

Merge branch 'main' into fix-undoredomore

bb8094a

JabRef deleted a comment from github-actions Bot Aug 29, 2026

github-actions Bot added status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Aug 29, 2026

calixtus added 2 commits August 30, 2026 09:37

Merge branch 'main' into fix-undoredomore

1c6576f

Fix tests

3d62b33

github-actions Bot added component: preferences status: changes-required Pull requests that are not yet complete and removed status: no-bot-comments labels Aug 30, 2026

Merge branch 'main' into fix-undoredomore

ec86fee

github-actions Bot added status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Aug 30, 2026

koppor approved these changes Aug 30, 2026

View reviewed changes

koppor enabled auto-merge August 30, 2026 20:18

koppor added this pull request to the merge queue Aug 30, 2026

github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label Aug 30, 2026

Merged via the queue into main with commit 667bc65 Aug 30, 2026
74 checks passed

koppor deleted the fix-undoredomore branch August 30, 2026 20:38

koppor mentioned this pull request Aug 30, 2026

Fix duplicate jabref-machine approvals #16750

Merged

3 tasks

calixtus mentioned this pull request Sep 5, 2026

Fix undo C - One undomanager / journal per library and some cleanups #16857

Merged

28 tasks

Navigation Menu

Uh oh!

Uh oh!

Fix undo B - Ids on undo changes, rewordings and split of UndoManager, minor clarifications - #16680

Fix undo B - Ids on undo changes, rewordings and split of UndoManager, minor clarifications#16680
koppor merged 35 commits into
mainfrom
fix-undoredomore

calixtus commented Aug 25, 2026 •

edited

Loading

Uh oh!

koppor left a comment

Uh oh!

koppor Aug 28, 2026

Uh oh!

calixtus Aug 28, 2026 •

edited

Loading

Uh oh!

calixtus Aug 28, 2026

Uh oh!

calixtus commented Aug 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		* `UndoManager` (jablib) — the recording interface: what about 120 classes depend on, and all they can do.
		* `JabRefUndoManager` (jablib) — the journal: stacks, undo, redo, saved position. Plain Java, no toolkit.

Uh oh!

Uh oh!

Conversation

calixtus commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Steps to test

Still open issues for follow-up:

Related issues and pull requests

AI usage

1. Code self-review

Nullability and control flow

Style and idioms

User-facing text

Tests

2. Verification commands

3. Documentation

4. Pull request

Checklist

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Required

Verified

Suggestions (non-blocking)

Uh oh!

koppor Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

calixtus Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calixtus Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

calixtus commented Aug 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

calixtus commented Aug 25, 2026 •

edited

Loading

calixtus Aug 28, 2026 •

edited

Loading