-
-
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
Conversation
|
|
||
| private void newEntryShowing(BibEntry entry) { | ||
| // skip history updates if this is from a back/forward operation | ||
| if (backOrForwardInProgress) { |
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.
Is this condition necessary? I would use such a check if scrolling and entry showing run on different threads, and use AtomicBoolean instead. But here both would run on the JavaFX thread, so it is not possible for this method to be called while scrolling is in progress.
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.
This is to differentiate if the call is coming from back/forward navigation button (in which case history management is nit needed, just need to update the UI) or if the user clicked a new entry (in which case history list needs an update).
In other words, without this, a back navigation call would be treated as direct navigation on the maintable and nextEntries.clear() would immediately undo the work the back() method just did and break the forward button.
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 changed the name of the boolean to backOrForwardNavigation to avoid confusion
| private final List<BibEntry> previousEntries = new ArrayList<>(); | ||
| private final List<BibEntry> nextEntries = new ArrayList<>(); |
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:
- When an entry is selected we add it to the list.
- When back called we decrement (if possible) the index and navigate to that entry
- When forward called we increment (if possible) and navigate.
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.
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
| // when no entries are selected, don't update navigation history | ||
| // but still update the navigation state |
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.
This comment does not add anything that the code doesn't already show, a better comment would describe the why.
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 just checked, it is not possible to "de-select" an entry by clicking on an empty cell in the main table.
I am refining the comments and keeping this as a fallback just in case there is a way to do that.
If there is not, maybe this can be removed,
|
Don't show the arrows in the GUI, if only one library is open. They take a lot of screenspace. Especially on small screens, if I remember right, there was a bug that would cause JabRef to crash when there are too many symbols in that row. That was one of the reasons that we removed social buttons from that row (+ the fact that social buttons for Linkedin or Twitter are really just a distraction in the GUI). |
Do you mean when no library is open? As this will make the user rely only on keyboard shortcuts? Also, the undo/redo buttons also have the same behavior. |
|
That's interesting |
Aren't those buttons to switch between libraries? Or what is it for? If there is only one library, they would be useless, right? |
No, it's for history navigation amongst entries of a library (like the back/forward button in a browser). Check the linked issue once, it'll be clear. |
| private final BooleanProperty nonUndoableChangeProperty = new SimpleBooleanProperty(false); | ||
| private final List<BibEntry> previousEntries = new ArrayList<>(); | ||
| private final List<BibEntry> nextEntries = new ArrayList<>(); | ||
| private BibEntry currentlyShowing; |
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.
This misses a connection to org.jabref.gui.maintable.MainTable#clearAndSelect(org.jabref.model.entry.BibEntry).
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 didn't get this - there is BibEntry toShow in navigateToEntry, on which this class' clearAndSelect is called, which is indeed MainTable#clearAndSelect internally. I don't think we can use it for currentlyShowing?
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.
My quick thoughts were: If there is an entry selected, I push the back button and then the forward button, the entry should be selected - and not an "arbitrary" one.
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.
Oh got it. Yeah, no arbitrary entries are selected.
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 hope, its just a binding thing 😅
| private final List<BibEntry> previousEntries = new ArrayList<>(); | ||
| private final List<BibEntry> nextEntries = new ArrayList<>(); |
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.
Using plain ArrayList instead of JavaFX ObservableList prevents UI from automatically updating when the collection changes. This impacts reactive UI updates.
| if (currentEntry != null) { | ||
| previousEntries.add(currentEntry); | ||
| } |
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.
Null check for currentEntry indicates possible null state handling. Methods should use Optional instead of null for better type safety and clarity.
| return; | ||
| } | ||
|
|
||
| // a new selection invalidates the forward history |
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.
Comment merely restates what the code does (clearing nextEntries). It doesn't provide additional information about why this is necessary.
|
If I have selected several entries, will this work? Use case (please allow me to use I find this immensively useful |
This is a very good idea. While developing, I did not think about multi-select behaviour. Can you file an issue and mark it good third? |


Closes #6352
This brings back an "improved" (per-tab) version of chronological navigation across entries that existed in JabRef 4.x.
Steps to test
alt+left,alt+right)Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)