From e0668a091c6c91c6da9dc8c429f567b6a6e0f3a7 Mon Sep 17 00:00:00 2001 From: Melis Olmez Date: Thu, 26 Sep 2024 14:47:50 +0300 Subject: [PATCH 1/7] fix remove listeners in RedoAction for memory efficiency, used WeakChangeListeners --- src/main/java/org/jabref/gui/undo/RedoAction.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/gui/undo/RedoAction.java b/src/main/java/org/jabref/gui/undo/RedoAction.java index 4ec944731e7..068d029e347 100644 --- a/src/main/java/org/jabref/gui/undo/RedoAction.java +++ b/src/main/java/org/jabref/gui/undo/RedoAction.java @@ -1,7 +1,10 @@ package org.jabref.gui.undo; +import java.util.Optional; import java.util.function.Supplier; +import javafx.beans.value.ChangeListener; +import javafx.beans.value.WeakChangeListener; import javax.swing.undo.CannotRedoException; import org.jabref.gui.DialogService; @@ -21,11 +24,15 @@ public RedoAction(Supplier tabSupplier, DialogService dialogService, this.tabSupplier = tabSupplier; this.dialogService = dialogService; - // TODO: The old listener should be removed. Otherwise, memory consumption will increase. - stateManager.activeTabProperty().addListener((observable, oldValue, activeLibraryTab) -> { + ChangeListener> listener = (observable, oldValue, activeLibraryTab) -> { activeLibraryTab.ifPresent(libraryTab -> - this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); - }); + this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); + + oldValue.ifPresent(libraryTab -> this.executable.unbind()); + }; + + WeakChangeListener> weakListener = new WeakChangeListener<>(listener); + stateManager.activeTabProperty().addListener(weakListener); } @Override From 67035ee57d9cd8a1c0b1922cd1652d4274d51461 Mon Sep 17 00:00:00 2001 From: Melis Olmez Date: Thu, 26 Sep 2024 20:32:09 +0300 Subject: [PATCH 2/7] fix checkstyle error --- src/main/java/org/jabref/gui/undo/RedoAction.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/undo/RedoAction.java b/src/main/java/org/jabref/gui/undo/RedoAction.java index 068d029e347..ab49766ca71 100644 --- a/src/main/java/org/jabref/gui/undo/RedoAction.java +++ b/src/main/java/org/jabref/gui/undo/RedoAction.java @@ -3,9 +3,11 @@ import java.util.Optional; import java.util.function.Supplier; +import javax.swing.undo.CannotRedoException; + import javafx.beans.value.ChangeListener; import javafx.beans.value.WeakChangeListener; -import javax.swing.undo.CannotRedoException; + import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; From 6cfe017a99e35ed05d8c46214de57413e3cd0984 Mon Sep 17 00:00:00 2001 From: Melis Olmez Date: Thu, 26 Sep 2024 21:10:13 +0300 Subject: [PATCH 3/7] fix checkstyle error --- src/main/java/org/jabref/gui/undo/RedoAction.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/gui/undo/RedoAction.java b/src/main/java/org/jabref/gui/undo/RedoAction.java index ab49766ca71..624dfe97f23 100644 --- a/src/main/java/org/jabref/gui/undo/RedoAction.java +++ b/src/main/java/org/jabref/gui/undo/RedoAction.java @@ -8,7 +8,6 @@ import javafx.beans.value.ChangeListener; import javafx.beans.value.WeakChangeListener; - import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; import org.jabref.gui.StateManager; @@ -16,7 +15,11 @@ import org.jabref.logic.l10n.Localization; /** - * @implNote See also {@link UndoAction} + * @implNote + * See + * also + * {@link + * UndoAction} */ public class RedoAction extends SimpleCommand { private final Supplier tabSupplier; @@ -28,7 +31,7 @@ public RedoAction(Supplier tabSupplier, DialogService dialogService, ChangeListener> listener = (observable, oldValue, activeLibraryTab) -> { activeLibraryTab.ifPresent(libraryTab -> - this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); + this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); oldValue.ifPresent(libraryTab -> this.executable.unbind()); }; @@ -43,7 +46,8 @@ public void execute() { try { libraryTab.getUndoManager().redo(); dialogService.notify(Localization.lang("Redo")); - } catch (CannotRedoException ex) { + } catch ( + CannotRedoException ex) { dialogService.notify(Localization.lang("Nothing to redo") + '.'); } libraryTab.markChangedOrUnChanged(); From 955d4da8ee15aa893a4758b0df2043e2951ea636 Mon Sep 17 00:00:00 2001 From: Melis Olmez Date: Fri, 27 Sep 2024 09:24:56 +0300 Subject: [PATCH 4/7] revert javadoc and fix catch indent --- src/main/java/org/jabref/gui/undo/RedoAction.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jabref/gui/undo/RedoAction.java b/src/main/java/org/jabref/gui/undo/RedoAction.java index 624dfe97f23..94de5231c91 100644 --- a/src/main/java/org/jabref/gui/undo/RedoAction.java +++ b/src/main/java/org/jabref/gui/undo/RedoAction.java @@ -15,11 +15,7 @@ import org.jabref.logic.l10n.Localization; /** - * @implNote - * See - * also - * {@link - * UndoAction} + * @implNote See also {@link UndoAction} */ public class RedoAction extends SimpleCommand { private final Supplier tabSupplier; @@ -46,8 +42,7 @@ public void execute() { try { libraryTab.getUndoManager().redo(); dialogService.notify(Localization.lang("Redo")); - } catch ( - CannotRedoException ex) { + } catch (CannotRedoException ex) { dialogService.notify(Localization.lang("Nothing to redo") + '.'); } libraryTab.markChangedOrUnChanged(); From 0f9088d288c27e33df96739ce60634d310b3c6d8 Mon Sep 17 00:00:00 2001 From: Melis Olmez Date: Mon, 30 Sep 2024 13:37:23 +0300 Subject: [PATCH 5/7] Remove listeners in UndoAction for memory efficiency --- src/main/java/org/jabref/gui/undo/UndoAction.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jabref/gui/undo/UndoAction.java b/src/main/java/org/jabref/gui/undo/UndoAction.java index a61551bcf6b..12b35db8525 100644 --- a/src/main/java/org/jabref/gui/undo/UndoAction.java +++ b/src/main/java/org/jabref/gui/undo/UndoAction.java @@ -1,9 +1,13 @@ package org.jabref.gui.undo; +import java.util.Optional; import java.util.function.Supplier; import javax.swing.undo.CannotUndoException; +import javafx.beans.value.ChangeListener; +import javafx.beans.value.WeakChangeListener; + import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; import org.jabref.gui.StateManager; @@ -21,10 +25,15 @@ public UndoAction(Supplier tabSupplier, DialogService dialogService, this.tabSupplier = tabSupplier; this.dialogService = dialogService; - stateManager.activeTabProperty().addListener((observable, oldValue, activeLibraryTab) -> { + ChangeListener> listener = (observable, oldValue, activeLibraryTab) -> { activeLibraryTab.ifPresent(libraryTab -> - this.executable.bind(libraryTab.getUndoManager().getUndoableProperty())); - }); + this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); + + oldValue.ifPresent(libraryTab -> this.executable.unbind()); + }; + + WeakChangeListener> weakListener = new WeakChangeListener<>(listener); + stateManager.activeTabProperty().addListener(weakListener); } @Override From 4c5e4d53689dc8c8249b46db89b03ce631bd8909 Mon Sep 17 00:00:00 2001 From: HoussemNasri Date: Fri, 4 Oct 2024 16:49:52 +0100 Subject: [PATCH 6/7] Use the fact that there is only one UndoManager instance shared between libraries --- src/main/java/org/jabref/gui/LibraryTab.java | 4 +-- .../java/org/jabref/gui/frame/MainMenu.java | 11 ++++---- .../org/jabref/gui/frame/MainToolBar.java | 4 +-- .../java/org/jabref/gui/undo/RedoAction.java | 28 +++++++++---------- .../java/org/jabref/gui/undo/UndoAction.java | 28 +++++++++---------- 5 files changed, 35 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/jabref/gui/LibraryTab.java b/src/main/java/org/jabref/gui/LibraryTab.java index 93901045848..791eec0dee1 100644 --- 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 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 index 7500cf72e03..96ab72d70d4 100644 --- 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 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 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 index 2f61f953c62..374d8f3e3a8 100644 --- 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 index 94de5231c91..d5d3170a993 100644 --- a/src/main/java/org/jabref/gui/undo/RedoAction.java +++ b/src/main/java/org/jabref/gui/undo/RedoAction.java @@ -1,12 +1,10 @@ package org.jabref.gui.undo; -import java.util.Optional; import java.util.function.Supplier; import javax.swing.undo.CannotRedoException; -import javafx.beans.value.ChangeListener; -import javafx.beans.value.WeakChangeListener; +import javafx.beans.binding.Bindings; import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; @@ -14,34 +12,34 @@ 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 tabSupplier; private final DialogService dialogService; + private final CountingUndoManager undoManager; - public RedoAction(Supplier tabSupplier, DialogService dialogService, StateManager stateManager) { + public RedoAction(Supplier tabSupplier, CountingUndoManager undoManager, DialogService dialogService, StateManager stateManager) { this.tabSupplier = tabSupplier; this.dialogService = dialogService; + this.undoManager = undoManager; - ChangeListener> listener = (observable, oldValue, activeLibraryTab) -> { - activeLibraryTab.ifPresent(libraryTab -> - this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); - - oldValue.ifPresent(libraryTab -> this.executable.unbind()); - }; - - WeakChangeListener> weakListener = new WeakChangeListener<>(listener); - stateManager.activeTabProperty().addListener(weakListener); + 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 index 12b35db8525..5eab905a9f8 100644 --- a/src/main/java/org/jabref/gui/undo/UndoAction.java +++ b/src/main/java/org/jabref/gui/undo/UndoAction.java @@ -1,12 +1,10 @@ package org.jabref.gui.undo; -import java.util.Optional; import java.util.function.Supplier; import javax.swing.undo.CannotUndoException; -import javafx.beans.value.ChangeListener; -import javafx.beans.value.WeakChangeListener; +import javafx.beans.binding.Bindings; import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; @@ -14,34 +12,34 @@ 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 tabSupplier; private final DialogService dialogService; + private final CountingUndoManager undoManager; - public UndoAction(Supplier tabSupplier, DialogService dialogService, StateManager stateManager) { + public UndoAction(Supplier tabSupplier, CountingUndoManager undoManager, DialogService dialogService, StateManager stateManager) { this.tabSupplier = tabSupplier; this.dialogService = dialogService; + this.undoManager = undoManager; - ChangeListener> listener = (observable, oldValue, activeLibraryTab) -> { - activeLibraryTab.ifPresent(libraryTab -> - this.executable.bind(libraryTab.getUndoManager().getRedoableProperty())); - - oldValue.ifPresent(libraryTab -> this.executable.unbind()); - }; - - WeakChangeListener> weakListener = new WeakChangeListener<>(listener); - stateManager.activeTabProperty().addListener(weakListener); + 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") + '.'); } From 2effee847cfdc52bc6d44839056fd94e8efc1788 Mon Sep 17 00:00:00 2001 From: HoussemNasri Date: Fri, 4 Oct 2024 16:59:14 +0100 Subject: [PATCH 7/7] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26b802b3240..0c1003278d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,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 when all libraries were closed. [#11837](https://github.com/JabRef/jabref/issues/11837) ### Removed