-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix bulk imports adding entries to navigation history #14575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 19 commits
41c6116
e760841
3c5271f
f1d5717
bdb70b9
cbd9813
4be103c
4fd6f05
b6b2b62
08d6bbd
0042466
f43ade9
db97184
e4922b9
3f4cbfb
7612988
a75ba39
db4a930
14faff5
2b3aed4
efdd797
8fd45e8
6ee1150
23a27e2
eb293dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,19 +11,30 @@ | |
| * Manages the navigation history of viewed entries using two stacks. | ||
| * This class encapsulates the logic of moving back and forward by maintaining a "back" stack for past entries | ||
| * and a "forward" stack for future entries. | ||
| * | ||
| * <p>History updates can be suppressed during bulk operations (e.g., file imports, drag-and-drop) | ||
| * using the {@link #suppressUpdates()} method, which employs a nesting-aware boolean flag to handle | ||
| * multiple concurrent suppression contexts correctly.</p> | ||
| */ | ||
| public class NavigationHistory { | ||
| private final List<BibEntry> previousEntries = new ArrayList<>(); | ||
| private final List<BibEntry> nextEntries = new ArrayList<>(); | ||
| private BibEntry currentEntry; | ||
| private boolean suppressNavigation = false; | ||
|
|
||
| /** | ||
| * Sets a new entry as the current one, clearing the forward history. | ||
| * The previously current entry is moved to the back stack. | ||
| * | ||
| * <p>This operation is skipped if navigation updates are currently suppressed.</p> | ||
| * | ||
| * @param entry The BibEntry to add to the history. | ||
| */ | ||
| public void add(BibEntry entry) { | ||
| if (suppressNavigation) { | ||
| return; | ||
| } | ||
|
|
||
| if (Objects.equals(currentEntry, entry)) { | ||
| return; | ||
| } | ||
|
|
@@ -74,4 +85,69 @@ public boolean canGoBack() { | |
| public boolean canGoForward() { | ||
| return !nextEntries.isEmpty(); | ||
| } | ||
|
|
||
| /** | ||
| * Suppresses navigation history updates while the returned guard is open. | ||
| * Handles nesting correctly: if already suppressed, the flag is only cleared when the outermost guard closes. | ||
| * | ||
| * @return A {@link Suppression} guard that restores navigation tracking when closed. | ||
| */ | ||
| public Suppression suppressUpdates() { | ||
| return new Suppression(this); | ||
| } | ||
|
|
||
| /** | ||
| * Convenience helper to suppress updates conditionally. | ||
| * | ||
| * @param active If true, returns an active suppression; otherwise returns a no-op suppression. | ||
| * @return A {@link Suppression} guard. | ||
| */ | ||
| public Suppression suppressUpdatesIf(boolean active) { | ||
| return active ? suppressUpdates() : Suppression.noOp(); | ||
| } | ||
|
|
||
| /** | ||
| * AutoCloseable guard for suppressing navigation history updates. | ||
| * | ||
| * <p>Uses a nesting-aware approach: captures the suppression state when created, | ||
| * and only restores it if this instance was the one that activated suppression.</p> | ||
| */ | ||
| public static final class Suppression implements AutoCloseable { | ||
|
||
| private static final Suppression NO_OP = new Suppression(); | ||
|
|
||
| private final NavigationHistory owner; | ||
| private boolean closed; | ||
| private final boolean active; | ||
| private final boolean wasAlreadySuppressed; | ||
|
|
||
| private Suppression() { | ||
| this.owner = null; | ||
| this.active = false; | ||
| this.wasAlreadySuppressed = false; | ||
| } | ||
|
|
||
| private Suppression(NavigationHistory owner) { | ||
| this.owner = owner; | ||
| this.active = true; | ||
| this.wasAlreadySuppressed = owner.suppressNavigation; | ||
| owner.suppressNavigation = true; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| if (closed || !active) { | ||
| return; | ||
| } | ||
| closed = true; | ||
|
|
||
| // Only turn off suppression if we were the ones who turned it on | ||
| if (!wasAlreadySuppressed) { | ||
| owner.suppressNavigation = false; | ||
| } | ||
| } | ||
|
|
||
| public static Suppression noOp() { | ||
| return NO_OP; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package org.jabref.gui; | ||
|
|
||
| import java.util.Optional; | ||
|
|
||
| import org.jabref.model.entry.BibEntry; | ||
| import org.jabref.model.entry.types.StandardEntryType; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| class NavigationHistoryTest { | ||
|
|
||
| @Test | ||
| void addsEntriesAndNavigatesBackAndForward() { | ||
| NavigationHistory history = new NavigationHistory(); | ||
| BibEntry first = new BibEntry(StandardEntryType.Article); | ||
| BibEntry second = new BibEntry(StandardEntryType.Book); | ||
|
|
||
| history.add(first); | ||
| history.add(second); | ||
|
|
||
| assertTrue(history.canGoBack()); | ||
| Optional<BibEntry> back = history.back(); | ||
| assertTrue(back.isPresent()); | ||
| assertEquals(first, back.get()); | ||
|
|
||
| assertTrue(history.canGoForward()); | ||
| Optional<BibEntry> forward = history.forward(); | ||
| assertTrue(forward.isPresent()); | ||
| assertEquals(second, forward.get()); | ||
| } | ||
|
|
||
| @Test | ||
| void suppressedAddsDoNotAffectHistory() { | ||
| NavigationHistory history = new NavigationHistory(); | ||
| BibEntry initial = new BibEntry(StandardEntryType.Article); | ||
| BibEntry suppressed = new BibEntry(StandardEntryType.Book); | ||
| BibEntry later = new BibEntry(StandardEntryType.InProceedings); | ||
|
|
||
| history.add(initial); | ||
|
|
||
| try (NavigationHistory.Suppression ignored = history.suppressUpdates()) { | ||
| history.add(suppressed); | ||
| } | ||
|
|
||
| // still only the initial entry is tracked | ||
| assertFalse(history.canGoBack()); | ||
|
|
||
| history.add(later); | ||
|
|
||
| assertTrue(history.canGoBack()); | ||
| Optional<BibEntry> back = history.back(); | ||
| assertTrue(back.isPresent()); | ||
| assertEquals(initial, back.get()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A. Why is this in a try block
B. Why can't this be replaced with an if-condition inside
NavigationHistorywith equivalent logic without all theSuppressionclass complication