Refactor Sidepane logic (#8202)

* Add visibleProperty to SidePaneComponent * Making progress into refactoring Sidepane logic - Add immutable info to SidePaneType enum - Replace SidePaneComponent by SidePaneView which has a SidePaneHeaderView that contains all input handling - Create SidePaneContainerView the replacement for SidePane * Populate SidePaneType with information * Wrap the content of web search side pane into WebSearchPaneView * Revert changes to SidePane * Refactor SidePaneContainer - Move some state and logic to SidePaneContainerViewModel - Implement the moveUp and moveDown action - Implement lazy loading for SidePaneView - Expose some initial API for fixing #8115 * Bind side pane check menu selection state to side pane visibility * Fix checkstyle * Remove SidePaneViewModel.java * Fix view menu for sidebar option out of sync with sidebar state - Fix #8115 - Deprecate the old implementation of Sidepane and remove all of its usage in the rest of the application - Implement the toggle method in SidePaneContainerView * Remove unused constructor parameters * Remove deprecated Sidepane component * Move Vbox.setVgrow() to SidePaneView * Merge SidePaneHeaderView into SidePaneView * Avoid firing removed and added events on ObservableList - Swapping elements in ObservableLists is rather difficult to handle. The problem with this code here is that two events are fired (one for removing, and on for adding). Perhaps the easiest solution is to use the sort method using a custom comparator. Since the list is small we can also be inefficient and do this as follows: create a copy of visiblePanes, swap the elements there and then use this new list as a reference for the comparator * Use shorter more declarative code for binding * Rename SidePaneView to SidePaneComponent and SidePaneContainerView to SidePane * Rename initView() to initialize() * Add an overloading of createCheckMenuItem() with a selected property * Rename sidePaneVisibleProperty() to paneVisibleProperty() * Use the name pane rather than sidePane to call components in the SidePane * Remove unused method createSidePaneCheckMenuItem() * Share the visibility property of side pane components by moving it to StateManager * Remove comment * Use better names * Fix checkstyle * Move close/toggle pane actions to separate files * Move sidepane binding logic to SidePaneViewModel * Removed unnecessary properties in stateManager, refactored to mvvm pattern * Moved listener from sidepane visibility property to list of children in sidepane * Removed unused method * Fixed sidepane width issue on startup * Created tests * Cleanups * CHANGELOG.md * more CHANGELOG.md Co-authored-by: Carl Christian Snethlage <cc.snethlage@gmail.com>
JabRef · Dec 14, 2021 · 851fa4b · 851fa4b
1 parent 9d55f53
commit 851fa4b
Show file tree

Hide file tree

Showing 15 changed files with 622 additions and 555 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -41,6 +41,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We reworked the export order in the preferences and the save order in the library preferences. You can now set more than three sort criteria in your library preferences. [#7935](https://github.com/JabRef/jabref/pull/7935)
 - The metadata-to-pdf actions now also embeds the bibfile to the PDF. [#8037](https://github.com/JabRef/jabref/pull/8037)
 - The snap was updated to use the core20 base and to use lzo compression for better startup performance [#8109](https://github.com/JabRef/jabref/pull/8109)
+- We moved the union/intersection view button in the group sidepane to the left of the other controls. [#8202](https://github.com/JabRef/jabref/pull/8202)
 - We improved the Drag and Drop behavior in the "Customize Entry Types" Dialog [#6338](https://github.com/JabRef/jabref/issues/6338)
 - When determining the URL of an ArXiV eprint, the URL now points to the version [#8149](https://github.com/JabRef/jabref/pull/8149)
 - We Included all standard fields with citation key when exporting to Old OpenOffice/LibreOffice Calc Format [#8176](https://github.com/JabRef/jabref/pull/8176)
@@ -62,6 +63,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed some icons that were drawn in the wrong color when JabRef used a custom theme. [#7853](https://github.com/JabRef/jabref/issues/7853)
 - We fixed an issue where the `Aux file` on `Edit group` doesn't support relative sub-directories path to import. [#7719](https://github.com/JabRef/jabref/issues/7719).
 - We fixed an issue where it was impossible to add or modify groups. [#7912](https://github.com/JabRef/jabref/pull/793://github.com/JabRef/jabref/pull/7921)
+- We fixed an issue about the visible side pane components being out of sync with the view menu. [#8115](https://github.com/JabRef/jabref/issues/8115)
+- We fixed an issue where the side pane would not close when all its components were closed. [#8082]((https://github.com/JabRef/jabref/issues/8082))
 - We fixed an issue where exported entries from a Citavi bib containing URLs could not be imported [#7892](https://github.com/JabRef/jabref/issues/7882)
 - We fixed an issue where the icons in the search bar had the same color, toggled as well as untoggled. [#8014](https://github.com/JabRef/jabref/pull/8014)
 - We fixed an issue where typing an invalid UNC path into the "Main file directory" text field caused an error. [#8107](https://github.com/JabRef/jabref/issues/8107)
@@ -76,6 +79,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed an issue where XMP and embedded BibTeX export would not work [#8278](https://github.com/JabRef/jabref/issues/8278)
 - We fixed an issue where the XMP and embedded BibTeX import of a file containing multiple schemas failed [#8278](https://github.com/JabRef/jabref/issues/8278)
 
+
 ### Removed
 
 - We removed two orphaned preferences options [#8164](https://github.com/JabRef/jabref/pull/8164)

diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java
@@ -12,10 +12,10 @@
 import java.util.stream.Collectors;
 
 import javafx.application.Platform;
+import javafx.beans.InvalidationListener;
+import javafx.beans.Observable;
 import javafx.beans.binding.Bindings;
 import javafx.beans.binding.StringBinding;
-import javafx.beans.value.ChangeListener;
-import javafx.beans.value.ObservableValue;
 import javafx.collections.transformation.FilteredList;
 import javafx.concurrent.Task;
 import javafx.geometry.Orientation;
@@ -109,7 +109,6 @@
 import org.jabref.gui.shared.ConnectToSharedDatabaseCommand;
 import org.jabref.gui.shared.PullChangesFromSharedAction;
 import org.jabref.gui.sidepane.SidePane;
-import org.jabref.gui.sidepane.SidePaneComponent;
 import org.jabref.gui.sidepane.SidePaneType;
 import org.jabref.gui.slr.ExistingStudySearchAction;
 import org.jabref.gui.slr.StartNewStudyAction;
@@ -143,6 +142,7 @@
 import com.google.common.eventbus.Subscribe;
 import com.tobiasdiez.easybind.EasyBind;
 import com.tobiasdiez.easybind.EasyObservableList;
+import com.tobiasdiez.easybind.Subscription;
 import org.controlsfx.control.PopOver;
 import org.controlsfx.control.TaskProgressView;
 import org.fxmisc.richtext.CodeArea;
@@ -176,6 +176,8 @@ public class JabRefFrame extends BorderPane {
     private PopOver progressViewPopOver;
     private PopOver entryFromIdPopOver;
 
+    private Subscription dividerSubscription;
+
     private final TaskExecutor taskExecutor;
 
     public JabRefFrame(Stage mainStage) {
@@ -341,7 +343,7 @@ public void about() {
      * FIXME: Currently some threads remain and therefore hinder JabRef to be closed properly
      *
      * @param filenames the filenames of all currently opened files - used for storing them if prefs openLastEdited is
-     *                  set to true
+     * set to true
      */
     private void tearDownJabRef(List<String> filenames) {
         if (prefs.getGuiPreferences().shouldOpenLastEdited()) {
@@ -433,29 +435,18 @@ private void initLayout() {
         head.setSpacing(0d);
         setTop(head);
 
-        splitPane.getItems().addAll(sidePane, tabbedPane);
+        splitPane.getItems().addAll(tabbedPane);
         SplitPane.setResizableWithParent(sidePane, false);
 
-        // We need to wait with setting the divider since it gets reset a few times during the initial set-up
-        mainStage.showingProperty().addListener(new ChangeListener<>() {
+        sidePane.getChildren().addListener((InvalidationListener) c -> updateSidePane());
+        updateSidePane();
 
+        // We need to wait with setting the divider since it gets reset a few times during the initial set-up
+        mainStage.showingProperty().addListener(new InvalidationListener() {
             @Override
-            public void changed(ObservableValue<? extends Boolean> observable, Boolean oldValue, Boolean showing) {
-                if (showing) {
+            public void invalidated(Observable observable) {
+                if (mainStage.isShowing()) {
                     setDividerPosition();
-
-                    EasyBind.subscribe(sidePane.visibleProperty(), visible -> {
-                        if (visible) {
-                            if (!splitPane.getItems().contains(sidePane)) {
-                                splitPane.getItems().add(0, sidePane);
-                                setDividerPosition();
-                            }
-                        } else {
-                            splitPane.getItems().remove(sidePane);
-                        }
-                    });
-
-                    mainStage.showingProperty().removeListener(this);
                     observable.removeListener(this);
                 }
             }
@@ -464,10 +455,24 @@ public void changed(ObservableValue<? extends Boolean> observable, Boolean oldVa
         setCenter(splitPane);
     }
 
+    private void updateSidePane() {
+        if (sidePane.getChildren().isEmpty()) {
+            if (dividerSubscription != null) {
+                dividerSubscription.unsubscribe();
+            }
+            splitPane.getItems().remove(sidePane);
+        } else {
+            if (!splitPane.getItems().contains(sidePane)) {
+                splitPane.getItems().add(0, sidePane);
+                setDividerPosition();
+            }
+        }
+    }
+
     private void setDividerPosition() {
         splitPane.setDividerPositions(prefs.getGuiPreferences().getSidePaneWidth());
-        if (!splitPane.getDividers().isEmpty()) {
-            EasyBind.subscribe(splitPane.getDividers().get(0).positionProperty(),
+        if (mainStage.isShowing() && !sidePane.getChildren().isEmpty()) {
+            dividerSubscription = EasyBind.subscribe(splitPane.getDividers().get(0).positionProperty(),
                     position -> prefs.getGuiPreferences().setSidePaneWidth(position.doubleValue()));
         }
     }
@@ -577,7 +582,6 @@ public void showLibraryTab(LibraryTab libraryTab) {
 
     public void init() {
         sidePane = new SidePane(prefs, taskExecutor, dialogService, stateManager, undoManager);
-
         tabbedPane = new TabPane();
         tabbedPane.setTabDragPolicy(TabPane.TabDragPolicy.REORDER);
 
@@ -843,15 +847,13 @@ private MenuBar createMenu() {
 
                 factory.createMenuItem(StandardActions.REBUILD_FULLTEXT_SEARCH_INDEX, new RebuildFulltextSearchIndexAction(stateManager, this::getCurrentLibraryTab, dialogService, prefs.getFilePreferences()))
         );
-
-        SidePaneComponent webSearch = sidePane.getComponent(SidePaneType.WEB_SEARCH);
-        SidePaneComponent groups = sidePane.getComponent(SidePaneType.GROUPS);
-        SidePaneComponent openOffice = sidePane.getComponent(SidePaneType.OPEN_OFFICE);
-
+        SidePaneType webSearchPane = SidePaneType.WEB_SEARCH;
+        SidePaneType groupsPane = SidePaneType.GROUPS;
+        SidePaneType openOfficePane = SidePaneType.OPEN_OFFICE;
         view.getItems().addAll(
-                factory.createCheckMenuItem(webSearch.getToggleAction(), webSearch.getToggleCommand(), sidePane.isComponentVisible(SidePaneType.WEB_SEARCH)),
-                factory.createCheckMenuItem(groups.getToggleAction(), groups.getToggleCommand(), sidePane.isComponentVisible(SidePaneType.GROUPS)),
-                factory.createCheckMenuItem(openOffice.getToggleAction(), openOffice.getToggleCommand(), sidePane.isComponentVisible(SidePaneType.OPEN_OFFICE)),
+                factory.createCheckMenuItem(webSearchPane.getToggleAction(), sidePane.getToggleCommandFor(webSearchPane), sidePane.paneVisibleBinding(webSearchPane)),
+                factory.createCheckMenuItem(groupsPane.getToggleAction(), sidePane.getToggleCommandFor(groupsPane), sidePane.paneVisibleBinding(groupsPane)),
+                factory.createCheckMenuItem(openOfficePane.getToggleAction(), sidePane.getToggleCommandFor(openOfficePane), sidePane.paneVisibleBinding(openOfficePane)),
 
                 new SeparatorMenuItem(),
 
@@ -1116,7 +1118,7 @@ private boolean readyForAutosave(BibDatabaseContext context) {
     /**
      * Opens the import inspection dialog to let the user decide which of the given entries to import.
      *
-     * @param panel        The BasePanel to add to.
+     * @param panel The BasePanel to add to.
      * @param parserResult The entries to add.
      */
     private void addImportedEntries(final LibraryTab panel, final ParserResult parserResult) {
@@ -1296,7 +1298,7 @@ public CloseDatabaseAction(LibraryTab libraryTab) {
 
         /**
          * Using this constructor will result in executing the command on the currently open library tab
-         * */
+         */
         public CloseDatabaseAction() {
             this(null);
         }

diff --git a/src/main/java/org/jabref/gui/StateManager.java b/src/main/java/org/jabref/gui/StateManager.java
@@ -17,6 +17,7 @@
 import javafx.concurrent.Task;
 import javafx.scene.Node;
 
+import org.jabref.gui.sidepane.SidePaneType;
 import org.jabref.gui.util.CustomLocalDragboard;
 import org.jabref.gui.util.DialogWindowState;
 import org.jabref.gui.util.OptionalObjectProperty;
@@ -56,11 +57,16 @@ public class StateManager {
     private final EasyBinding<Boolean> anyTaskRunning = EasyBind.reduce(backgroundTasks, tasks -> tasks.anyMatch(Task::isRunning));
     private final EasyBinding<Double> tasksProgress = EasyBind.reduce(backgroundTasks, tasks -> tasks.filter(Task::isRunning).mapToDouble(Task::getProgress).average().orElse(1));
     private final ObservableMap<String, DialogWindowState> dialogWindowStates = FXCollections.observableHashMap();
+    private final ObservableList<SidePaneType> visibleSidePanes = FXCollections.observableArrayList();
 
     public StateManager() {
         activeGroups.bind(Bindings.valueAt(selectedGroups, activeDatabase.orElse(null)));
     }
 
+    public ObservableList<SidePaneType> getVisibleSidePaneComponents() {
+        return visibleSidePanes;
+    }
+
     public CustomLocalDragboard getLocalDragboard() {
         return localDragboard;
     }

diff --git a/src/main/java/org/jabref/gui/actions/ActionFactory.java b/src/main/java/org/jabref/gui/actions/ActionFactory.java
@@ -5,6 +5,7 @@
 import java.lang.reflect.Method;
 import java.util.Objects;
 
+import javafx.beans.binding.BooleanExpression;
 import javafx.scene.control.Button;
 import javafx.scene.control.ButtonBase;
 import javafx.scene.control.CheckMenuItem;
@@ -113,6 +114,14 @@ public CheckMenuItem createCheckMenuItem(Action action, Command command, boolean
         return checkMenuItem;
     }
 
+    public CheckMenuItem createCheckMenuItem(Action action, Command command, BooleanExpression selectedBinding) {
+        CheckMenuItem checkMenuItem = ActionUtils.createCheckMenuItem(new JabRefAction(action, command, keyBindingRepository, Sources.FromMenu));
+        EasyBind.subscribe(selectedBinding, checkMenuItem::setSelected);
+        setGraphic(checkMenuItem, action);
+
+        return checkMenuItem;
+    }
+
     public Menu createMenu(Action action) {
         Menu menu = ActionUtils.createMenu(new JabRefAction(action, keyBindingRepository));
 

diff --git a/src/main/java/org/jabref/gui/groups/GroupSidePane.java b/src/main/java/org/jabref/gui/groups/GroupSidePane.java