Fix issue where "Close library" menu item is not disabled correctly#10950
Conversation
calixtus
left a comment
There was a problem hiding this comment.
Hi, thanks for your interest in JabRef and Open Source programming.
The changes you made look really good and i think codewise they are already pretty good. I just have a suggestion of placement of the null check.
| LibraryTab currentTab = getCurrentLibraryTab(); | ||
| if (currentTab == null) { | ||
| return false; | ||
| } | ||
| return closeTab(currentTab); |
There was a problem hiding this comment.
I would suggest moving this check on null to CloseDatabaseAction#execute, then this will be consistent with other actions and we keep the view (yes i know, right now according to the mvvm pattern, jabrefframe is still way off a pure view, but it gets there eventually).
There was a problem hiding this comment.
That makes sense. But in this case, do you suggest to call the getCurrentLibraryTab on tabContainer before calling closeCurrentTab ? That would make two calls to getCurrentLibraryTab.
Or maybe I should implement a method getTabCount to know if there is at least one tab.
There was a problem hiding this comment.
No, just return from the execute method (fail fast) with a log message if the the library tab and the currently opened tab is null.
| - We fixed an issue where the `File -> Close library` menu item was not disabled when no library was open. [#10948](https://github.com/JabRef/jabref/issues/10948) | ||
| - We fixed an issue where the Document Viewer would show the PDF in only half the window when maximized. [#10934](https://github.com/JabRef/jabref/issues/10934) | ||
|
|
||
|
|
There was a problem hiding this comment.
For the next PR: Please no additional empty lines in CHANGELOG.md. This will lead to merge conflicts.
| public void execute() { | ||
| Platform.runLater(() -> { | ||
| if (libraryTab == null) { | ||
| if (tabContainer.getCurrentLibraryTab() == null) { |
There was a problem hiding this comment.
I think, this code will never be reached, but good to be defensive.
Fixed a bug that allowed you to click on the
File -> Close librarymenu item even if no library was open. When the button was pressed in this situation, the following error was uncaught.This PR prevents the
closeCurrentTabmethod from callingcloseTabwithnullas a parameter, and disables the menu item (and its associated shortcut) when there is no library tab open.Screen.Recording.2024-02-29.at.18.10.12.mov
Closes #10948
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)