Remove listeners in UndoAction and RedoAction for memory efficiency (#…

…11839) * fix remove listeners in RedoAction for memory efficiency, used WeakChangeListeners * fix checkstyle error * fix checkstyle error * revert javadoc and fix catch indent * Remove listeners in UndoAction for memory efficiency * Use the fact that there is only one UndoManager instance shared between libraries * Update CHANGELOG.md --------- Co-authored-by: HoussemNasri <[email protected]> Co-authored-by: Houssem Nasri <[email protected]>
JabRef · Oct 4, 2024 · ecd78de · ecd78de
1 parent 03f7a49
commit ecd78de
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 25 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -82,6 +82,8 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - We fixed an issue where unescaped braces in the arXiv fetcher were not treated. [#11704](https://github.com/JabRef/jabref/issues/11704)
 - We fixed an issue where HTML instead of the fulltext pdf was downloaded when importing arXiv entries. [#4913](https://github.com/JabRef/jabref/issues/4913)
 - We fixed an issue where the keywords and crossref fields were not properly focused. [#11177](https://github.com/JabRef/jabref/issues/11177)
+- We fixed an issue where listeners were being attached without being released later leading to a memory leak. [#11809](https://github.com/JabRef/jabref/issues/11809)
+- We fixed an issue where the Undo/Redo buttons were active even when all libraries are closed. [#11837](https://github.com/JabRef/jabref/issues/11837)
 - We fixed an issue where recently opened files were not displayed in the main menu properly. [#9042](https://github.com/JabRef/jabref/issues/9042)
 - We fixed an issue where the DOI lookup would show an error when a DOI was found for an entry. [#11850](https://github.com/JabRef/jabref/issues/11850)
 

diff --git a/src/main/java/org/jabref/gui/LibraryTab.java b/src/main/java/org/jabref/gui/LibraryTab.java
@@ -260,8 +260,8 @@ private EntryEditor createEntryEditor() {
         Supplier<LibraryTab> tabSupplier = () -> this;
         return new EntryEditor(this,
                 // Actions are recreated here since this avoids passing more parameters and the amount of additional memory consumption is neglegtable.
-                new UndoAction(tabSupplier, dialogService, stateManager),
-                new RedoAction(tabSupplier, dialogService, stateManager));
+                new UndoAction(tabSupplier, undoManager, dialogService, stateManager),
+                new RedoAction(tabSupplier, undoManager, dialogService, stateManager));
     }
 
     private static void addChangedInformation(StringBuilder text, String fileName) {

diff --git a/src/main/java/org/jabref/gui/frame/MainMenu.java b/src/main/java/org/jabref/gui/frame/MainMenu.java
@@ -2,8 +2,6 @@
 
 import java.util.function.Supplier;
 
-import javax.swing.undo.UndoManager;
-
 import javafx.event.ActionEvent;
 import javafx.scene.control.Menu;
 import javafx.scene.control.MenuBar;
@@ -70,6 +68,7 @@
 import org.jabref.gui.slr.StartNewStudyAction;
 import org.jabref.gui.specialfields.SpecialFieldMenuItemFactory;
 import org.jabref.gui.texparser.ParseLatexAction;
+import org.jabref.gui.undo.CountingUndoManager;
 import org.jabref.gui.undo.RedoAction;
 import org.jabref.gui.undo.UndoAction;
 import org.jabref.gui.util.UiTaskExecutor;
@@ -98,7 +97,7 @@ public class MainMenu extends MenuBar {
     private final DialogService dialogService;
     private final JournalAbbreviationRepository abbreviationRepository;
     private final BibEntryTypesManager entryTypesManager;
-    private final UndoManager undoManager;
+    private final CountingUndoManager undoManager;
     private final ClipBoardManager clipBoardManager;
     private final Supplier<OpenDatabaseAction> openDatabaseActionSupplier;
     private final AiService aiService;
@@ -114,7 +113,7 @@ public MainMenu(JabRefFrame frame,
                     DialogService dialogService,
                     JournalAbbreviationRepository abbreviationRepository,
                     BibEntryTypesManager entryTypesManager,
-                    UndoManager undoManager,
+                    CountingUndoManager undoManager,
                     ClipBoardManager clipBoardManager,
                     Supplier<OpenDatabaseAction> openDatabaseActionSupplier,
                     AiService aiService) {
@@ -184,8 +183,8 @@ private void createMenu() {
         );
 
         edit.getItems().addAll(
-                factory.createMenuItem(StandardActions.UNDO, new UndoAction(frame::getCurrentLibraryTab, dialogService, stateManager)),
-                factory.createMenuItem(StandardActions.REDO, new RedoAction(frame::getCurrentLibraryTab, dialogService, stateManager)),
+                factory.createMenuItem(StandardActions.UNDO, new UndoAction(frame::getCurrentLibraryTab, undoManager, dialogService, stateManager)),
+                factory.createMenuItem(StandardActions.REDO, new RedoAction(frame::getCurrentLibraryTab, undoManager, dialogService, stateManager)),
 
                 new SeparatorMenuItem(),
 

diff --git a/src/main/java/org/jabref/gui/frame/MainToolBar.java b/src/main/java/org/jabref/gui/frame/MainToolBar.java
@@ -133,8 +133,8 @@ private void createToolBar() {
                 new Separator(Orientation.VERTICAL),
 
                 new HBox(
-                        factory.createIconButton(StandardActions.UNDO, new UndoAction(frame::getCurrentLibraryTab, dialogService, stateManager)),
-                        factory.createIconButton(StandardActions.REDO, new RedoAction(frame::getCurrentLibraryTab, dialogService, stateManager)),
+                        factory.createIconButton(StandardActions.UNDO, new UndoAction(frame::getCurrentLibraryTab, undoManager, dialogService, stateManager)),
+                        factory.createIconButton(StandardActions.REDO, new RedoAction(frame::getCurrentLibraryTab, undoManager, dialogService, stateManager)),
                         factory.createIconButton(StandardActions.CUT, new EditAction(StandardActions.CUT, frame::getCurrentLibraryTab, stateManager, undoManager)),
                         factory.createIconButton(StandardActions.COPY, new EditAction(StandardActions.COPY, frame::getCurrentLibraryTab, stateManager, undoManager)),
                         factory.createIconButton(StandardActions.PASTE, new EditAction(StandardActions.PASTE, frame::getCurrentLibraryTab, stateManager, undoManager))),

diff --git a/src/main/java/org/jabref/gui/undo/RedoAction.java b/src/main/java/org/jabref/gui/undo/RedoAction.java
@@ -4,36 +4,42 @@
 
 import javax.swing.undo.CannotRedoException;
 
+import javafx.beans.binding.Bindings;
+
 import org.jabref.gui.DialogService;
 import org.jabref.gui.LibraryTab;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 import org.jabref.logic.l10n.Localization;
 
+import static org.jabref.gui.actions.ActionHelper.needsDatabase;
+
 /**
  * @implNote See also {@link UndoAction}
  */
 public class RedoAction extends SimpleCommand {
     private final Supplier<LibraryTab> tabSupplier;
     private final DialogService dialogService;
+    private final CountingUndoManager undoManager;
 
-    public RedoAction(Supplier<LibraryTab> tabSupplier, DialogService dialogService, StateManager stateManager) {
+    public RedoAction(Supplier<LibraryTab> tabSupplier, CountingUndoManager undoManager, DialogService dialogService, StateManager stateManager) {
         this.tabSupplier = tabSupplier;
         this.dialogService = dialogService;
+        this.undoManager = undoManager;
 
-        // TODO: The old listener should be removed. Otherwise, memory consumption will increase.
-        stateManager.activeTabProperty().addListener((observable, oldValue, activeLibraryTab) -> {
-            activeLibraryTab.ifPresent(libraryTab ->
-                    this.executable.bind(libraryTab.getUndoManager().getRedoableProperty()));
-        });
+        this.executable.bind(Bindings.and(needsDatabase(stateManager), undoManager.getRedoableProperty()));
     }
 
     @Override
     public void execute() {
         LibraryTab libraryTab = this.tabSupplier.get();
         try {
-            libraryTab.getUndoManager().redo();
-            dialogService.notify(Localization.lang("Redo"));
+            if (undoManager.canRedo()) {
+                undoManager.redo();
+                dialogService.notify(Localization.lang("Redo"));
+            } else {
+                throw new CannotRedoException();
+            }
         } catch (CannotRedoException ex) {
             dialogService.notify(Localization.lang("Nothing to redo") + '.');
         }

diff --git a/src/main/java/org/jabref/gui/undo/UndoAction.java b/src/main/java/org/jabref/gui/undo/UndoAction.java
@@ -4,35 +4,42 @@
 
 import javax.swing.undo.CannotUndoException;
 
+import javafx.beans.binding.Bindings;
+
 import org.jabref.gui.DialogService;
 import org.jabref.gui.LibraryTab;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 import org.jabref.logic.l10n.Localization;
 
+import static org.jabref.gui.actions.ActionHelper.needsDatabase;
+
 /**
  * @implNote See also {@link RedoAction}
  */
 public class UndoAction extends SimpleCommand {
     private final Supplier<LibraryTab> tabSupplier;
     private final DialogService dialogService;
+    private final CountingUndoManager undoManager;
 
-    public UndoAction(Supplier<LibraryTab> tabSupplier, DialogService dialogService, StateManager stateManager) {
+    public UndoAction(Supplier<LibraryTab> tabSupplier, CountingUndoManager undoManager, DialogService dialogService, StateManager stateManager) {
         this.tabSupplier = tabSupplier;
         this.dialogService = dialogService;
+        this.undoManager = undoManager;
 
-        stateManager.activeTabProperty().addListener((observable, oldValue, activeLibraryTab) -> {
-            activeLibraryTab.ifPresent(libraryTab ->
-                    this.executable.bind(libraryTab.getUndoManager().getUndoableProperty()));
-        });
+        this.executable.bind(Bindings.and(needsDatabase(stateManager), undoManager.getUndoableProperty()));
     }
 
     @Override
     public void execute() {
         LibraryTab libraryTab = this.tabSupplier.get();
         try {
-            libraryTab.getUndoManager().undo();
-            dialogService.notify(Localization.lang("Undo"));
+            if (undoManager.canUndo()) {
+                undoManager.undo();
+                dialogService.notify(Localization.lang("Undo"));
+            } else {
+                throw new CannotUndoException();
+            }
         } catch (CannotUndoException ex) {
             dialogService.notify(Localization.lang("Nothing to undo") + '.');
         }