-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Chronological navigation in libraries #13863
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
Changes from 6 commits
1d75410
df50edc
b042e38
589bdd8
a9c95eb
8304feb
a8f57d6
0559d11
e33973a
fbb37fa
d890eea
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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import java.io.IOException; | ||
| import java.nio.file.Path; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
|
|
@@ -118,6 +119,13 @@ public class LibraryTab extends Tab implements CommandSelectionTab { | |
| private final BibEntryTypesManager entryTypesManager; | ||
| private final BooleanProperty changedProperty = new SimpleBooleanProperty(false); | ||
| private final BooleanProperty nonUndoableChangeProperty = new SimpleBooleanProperty(false); | ||
| private final List<BibEntry> previousEntries = new ArrayList<>(); | ||
| private final List<BibEntry> nextEntries = new ArrayList<>(); | ||
| private BibEntry currentlyShowing; | ||
|
||
| private final BooleanProperty canGoBackProperty = new SimpleBooleanProperty(false); | ||
| private final BooleanProperty canGoForwardProperty = new SimpleBooleanProperty(false); | ||
| private boolean backOrForwardInProgress = false; | ||
|
|
||
|
|
||
| private BibDatabaseContext bibDatabaseContext; | ||
|
|
||
|
|
@@ -490,6 +498,15 @@ private void createMainTable() { | |
| mainTable.addSelectionListener(event -> { | ||
| List<BibEntry> entries = event.getList().stream().map(BibEntryTableViewModel::getEntry).toList(); | ||
| stateManager.setSelectedEntries(entries); | ||
|
|
||
| // track navigation history for single selections | ||
| if (entries.size() == 1) { | ||
| newEntryShowing(entries.getFirst()); | ||
| } else if (entries.isEmpty()) { | ||
| // when no entries are selected, don't update navigation history | ||
| // but still update the navigation state | ||
|
||
| updateNavigationState(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -964,6 +981,68 @@ public void resetChangedProperties() { | |
| this.changedProperty.setValue(false); | ||
| } | ||
|
|
||
| public void back() { | ||
| navigateToEntry(previousEntries, nextEntries); | ||
| } | ||
|
|
||
| public void forward() { | ||
| navigateToEntry(nextEntries, previousEntries); | ||
| } | ||
|
|
||
| private void navigateToEntry(List<BibEntry> sourceHistory, List<BibEntry> destinationHistory) { | ||
| if (!sourceHistory.isEmpty()) { | ||
| BibEntry toShow = sourceHistory.getLast(); | ||
| sourceHistory.removeLast(); | ||
|
|
||
| // add current entry to destination history | ||
| if (currentlyShowing != null) { | ||
| destinationHistory.add(currentlyShowing); | ||
| } | ||
|
|
||
| backOrForwardInProgress = true; | ||
| clearAndSelect(toShow); | ||
| updateNavigationState(); | ||
| } | ||
| } | ||
|
|
||
| public boolean canGoBack() { | ||
| return !previousEntries.isEmpty(); | ||
| } | ||
|
|
||
| public boolean canGoForward() { | ||
| return !nextEntries.isEmpty(); | ||
| } | ||
|
|
||
| private void newEntryShowing(BibEntry entry) { | ||
| // skip history updates if this is from a back/forward operation | ||
| if (backOrForwardInProgress) { | ||
|
||
| currentlyShowing = entry; | ||
| backOrForwardInProgress = false; | ||
| updateNavigationState(); | ||
| return; | ||
| } | ||
|
|
||
| nextEntries.clear(); // clear forward history when new entry selected in existing chronological sequence | ||
|
|
||
| if (!Objects.equals(entry, currentlyShowing)) { | ||
| // add the entry we are leaving to the history | ||
| if (currentlyShowing != null) { | ||
| previousEntries.add(currentlyShowing); | ||
| } | ||
| currentlyShowing = entry; | ||
| updateNavigationState(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Updates the StateManager with current navigation state | ||
| * Only update if this is the active tab | ||
| */ | ||
| public void updateNavigationState() { | ||
| canGoBackProperty.set(canGoBack()); | ||
| canGoForwardProperty.set(canGoForward()); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new library tab. Contents are loaded by the {@code dataLoadingTask}. Most of the other parameters are required by {@code resetChangeMonitor()}. | ||
| * | ||
|
|
@@ -1034,6 +1113,14 @@ public static LibraryTab createLibraryTab(@NonNull BibDatabaseContext databaseCo | |
| false); | ||
| } | ||
|
|
||
| public BooleanProperty canGoBackProperty() { | ||
| return canGoBackProperty; | ||
| } | ||
|
|
||
| public BooleanProperty canGoForwardProperty() { | ||
| return canGoForwardProperty; | ||
| } | ||
|
|
||
| private class GroupTreeListener { | ||
|
|
||
| @Subscribe | ||
|
|
||
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.
We can replace this by one list and an index that points to the currently selected entry:
Uh oh!
There was an error while loading. Please reload this page.
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.
I thought about this a bit, I am a bit reluctant about this scenario:
You seelct entry A, then B, then C, then D.
You click Back twice. You are now viewing B.
From B, you click on a new entry, E.
The old "forward" history (entries C, D) must be discarded. The new history should be A -> B -> E.
The logic to truncate the "forward" part of the list will be slightly less cleaner (clearing the forward "sublist" of the original list and adding the new entry) than the way we just do
nextEntries.clear()now, but if two people are in favor of this I'll shift to a single list.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.
I thought whether Eclipse Collections and sublists could help here. However, the current code is very clean and I would keep it.
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.
IMO, managing one state is much easier them managing 2+, because you need to keep them in sync: Whenever you modify one you need to think which one needs to also change.
But, you can still make the current design better by encapsulating the forward and backward list into a record.
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.
Record is suitable in a context of immutability. If we wish to encapsulate, can do with a separate class
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.
Refactored in commit d890eea, lets see if you guys like this