Remove listeners in UndoAction and RedoAction for memory efficiency #11839

melisolmez · 2024-09-26T12:52:03Z

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

…angeListeners

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

koppor · 2024-09-26T15:21:57Z

Checkstyle says

Error: eckstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/gui/undo/RedoAction.java:8:1: Wrong order for 'javax.swing.undo.CannotRedoException' import. [ImportOrder]

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

subhramit

Minor formatting changes requested. Otherwise LGTM.
Thank you for your contribution!

P.S. Do mark the mandatory checks that you have completed in the PR description when you file one (the ones that apply).

subhramit · 2024-09-26T22:08:24Z

src/main/java/org/jabref/gui/undo/RedoAction.java

- * @implNote See also {@link UndoAction}
+ * @implNote
+ * See
+ * also
+ * {@link
+ * UndoAction}


Revert this

subhramit · 2024-09-26T22:10:03Z

src/main/java/org/jabref/gui/undo/RedoAction.java

-        } catch (CannotRedoException ex) {
+        } catch (
+                CannotRedoException ex) {


Please also fix the catch indent (not your fault, IntelliJ does this by default when we format the document).

koppor · 2024-09-27T06:31:56Z

src/main/java/org/jabref/gui/undo/RedoAction.java

            activeLibraryTab.ifPresent(libraryTab ->
                    this.executable.bind(libraryTab.getUndoManager().getRedoableProperty()));
-        });
+


This empty line is not necessary, is it?

actually this empty line is for exists to separate lines 30 and 32 from each other. But I can remove.

koppor · 2024-09-27T06:35:16Z

@subhramit We should test this. This PR implements both suggestions of you (#11809 (comment)). I think, one needs to open 5 to 10 libraries, switch tabs back and forth, modifying the library and see if undo/redo is enabled properly.

@melisolmez For consistency reasons, this patch also nees to be ported to the UndoAction. Undo is the twin of Redo. --> https://github.com/JabRef/jabref/blob/8412e651b671a427c828924ce5698eb4c32f269d/src/main/java/org/jabref/gui/undo/UndoAction.java

melisolmez · 2024-09-27T06:50:42Z

@subhramit We should test this. This PR implements both suggestions of you (#11809 (comment)). I think, one needs to open 5 to 10 libraries, switch tabs back and forth, modifying the library and see if undo/redo is enabled properly.

@melisolmez For consistency reasons, this patch also nees to be ported to the UndoAction. Undo is the twin of Redo. --> https://github.com/JabRef/jabref/blob/8412e651b671a427c828924ce5698eb4c32f269d/src/main/java/org/jabref/gui/undo/UndoAction.java

Okey. Should I send it to the same branch?

subhramit · 2024-09-27T07:49:09Z

@subhramit We should test this. This PR implements both suggestions of you (#11809 (comment)). I think, one needs to open 5 to 10 libraries, switch tabs back and forth, modifying the library and see if undo/redo is enabled properly.

Actually, suggestion 1 was to conditionally but explicitly use removeListener, which is not what has been done in the PR. WeakChangeListner automatically takes care of that.

What is probably confusing - use of unbind - I think not really necessary here (I dont remember how the garbage collecter deals with a chain of references), but still a safe practice as there is a life cycle mismatch between RedoAction and tabs - RedoAction might live longer than the relevance of a particular tab. There can be "zombie" bindings between executable and the tab. By unbinding, the old/irrelevant tabs can be garbage collected even if RedoAction itself is still alive (provided they're not referenced anywhere else).

subhramit · 2024-09-27T08:36:01Z

src/main/java/org/jabref/gui/undo/RedoAction.java

                    this.executable.bind(libraryTab.getUndoManager().getRedoableProperty()));
-        });
+
+            oldValue.ifPresent(libraryTab -> this.executable.unbind());


Thinking a bit more, I am a bit concerned about the order of operations here.
Case 1:
State A->B - change detected, bind to B, unbind if oldValue is not null. After this,
State B->A - change detected, bind to A (not needed), unbind if B (oldvalue) is not null, which is true, so unbind (this is fine).

Case 2:
State A->B - change detected, bind to B, unbind if oldValue is not null. After this,
State B->C - change detected, bind to C, unbind if B is not null (which is true) - thus executable is not bound.

@Siedlerchr would it be better if we don't unbind at all here? Or is this alright as LibraryTab can never have a third state "C"?

HoussemNasri · 2024-09-29T18:46:52Z

Okey. Should I send it to the same branch?

Undo/Redo are part of the same component, IMO this PR should handle both of them.

melisolmez · 2024-09-30T12:21:43Z

@HoussemNasri hello, Could you review the pr?

Siedlerchr · 2024-09-30T19:21:17Z

We have tested this with the following test cases and found some odd issues:

Create a couple of new libraries
In Each one enter a new article (+ button) with author a in the first, b in the second and so on
Do Undo, switch tabs, Redo, etc.

subhramit · 2024-09-30T19:28:58Z

We have tested this with the following test cases and found some odd issues:

Create a couple of new libraries

In Each one enter a new article (+ button) with author a in the first, b in the second and so on

Do Undo, switch tabs, Redo, etc.

More details: Compare it with the main branch. You will find that in your branch, on adding the author name, it is not reflected in the Author/Editor column of the entry (until some sequence of events - changing tabs, pressing enter, etc.)
Furthermore, the undo/redo changes are not reflected without switching tabs.

My hypothesis: the early unbinding (described in #11839 (comment)) is causing nothing to be bound, and thus no listeners are attached at all, and thus no changes are reflected.

Fix: The unbinding should not happen where it is currently. Remove it, profile the memory using VirtualVM to test the changes. We profiled it on the main branch - on adding tabs, the used heap size was shooting up, whereas in your branch the garbage collector did kick in, reducing the heap size on closing tabs.

If the sequence becomes too confusing, you can also use plain old addListener and removeListener conditionally.

Apologies that the testing part may make this a bit more involved than a good first issue.

melisolmez · 2024-09-30T19:34:12Z

We have tested this with the following test cases and found some odd issues:

Create a couple of new libraries

In Each one enter a new article (+ button) with author a in the first, b in the second and so on

Do Undo, switch tabs, Redo, etc.

More details: Compare it with the main branch. You will find that in your branch, on adding the author name, it is not reflected in the Author/Editor column of the entry (until some sequence of events - changing tabs, pressing enter, etc.) Furthermore, the undo/redo changes are not reflected without switching tabs.

My hypothesis: the early unbinding (described in #11839 (comment)) is causing nothing to be bound, and thus no listeners are attached at all, and thus no changes are reflected.

Fix: The unbinding should not happen where it is currently. Remove it, profile the memory using VirtualVM to test the changes. We profiled it on the main branch - on adding tabs, the used heap size was shooting up, whereas in your branch the garbage collector did kick in, reducing the heap size on closing tabs.

If the sequence becomes too confusing, you can also use plain old addListener and removeListener conditionally.

Apologies that the testing part may make this a bit more involved than a good first issue.

Thank you for your feedbak. I will look at it in more detail

melisolmez · 2024-10-01T16:55:38Z

Hi @subhramit, I tested what you said but I couldn't find where it was wrong. When I add author name it is reflected in the Author/Editor column. and the undo and redo buttons active.

I don't understand exactly what the problem is, can you explain it in more detail?

subhramit · 2024-10-01T17:03:08Z

I don't understand exactly what the problem is, can you explain it in more detail?

Will try to send you a video.

subhramit · 2024-10-02T07:39:08Z

I am unable to reproduce what @Siedlerchr did, the undo/redo buttons are staying disabled for me:

Comparison with main branch: The undo button becomes active as soon as I ~~type "a"~~ add a new entry in the new library.

subhramit · 2024-10-02T07:49:48Z

In the above case, I closed some previously opened libraries.
Here is another case (no libraries open on starting JabRef):
Add a library:

Both the Undo/Redo buttons are active even though no changes have been made/ no entries have been added.
Comparison with main branch: On opening JabRef with no previous libraries, the Undo/Redo buttons are enabled (because of the other bug #11837), but as soon as a new library is added, the buttons become disabled. When A new entry is added to the library, the Undo button becomes active (expected).
Conclusion: the listener in main is capturing changes properly, the one in this branch is not.

subhramit · 2024-10-02T07:55:42Z

The two issues mentioned above persist even on removing the line that unbinds the executable (my hypothesis fails). Hence, we need to redo this PR.
@HoussemNasri can you take a look at what could be wrong? Should we go some other way?

subhramit · 2024-10-03T08:05:47Z

@melisolmez I think the other maintainers are presently a bit busy, and we appreciate your patience in sticking to the task.
What I think you can do is meet the two conditions -

resolve the two issues mentioned above so that the unexpected behaviour above is handled (I suggest that you use the old logic and conditionally remove the listeners using an "else" to the ifPresent check for a tab), and
confirm with VirtualVM after closing tabs that the listeners are indeed cleaned up after that change.

I think testing can be less confusing if #11837 is solved first, but that is optional - you can also pick it up if interested, or continue with this one.

melisolmez · 2024-10-03T12:01:40Z

Hello @subhramit, I tested your sayng but I couldn't find the wrong things where.

@melisolmez I think the other maintainers are presently a bit busy, and we appreciate your patience in sticking to the task. What I think you can do is meet the two conditions -

resolve the two issues mentioned above so that the unexpected behaviour above is handled (I suggest that you use the old logic and conditionally remove the listeners using an "else" to the ifPresent check for a tab), and

confirm with VirtualVM after closing tabs that the listeners are indeed cleaned up after that change.

I think testing can be less confusing if #11837 is solved first, but that is optional - you can also pick it up if interested, or continue with this one.

Hello @subhramit thank you for help. I understand what is the problem and I will try fix it.
so I need a few details to solve this problem. I would be glad if you answer.

How to manage active and passive buttons? and where ?( for example in the main branch: when open a library these buttons can be disable. If I find this event where it can help me with issue)
If I examine where and how it is managed, I will try to establish a similar structure, so I will look for a solution to issue redo and undo buttons are active without open library #11837.

subhramit · 2024-10-03T12:20:34Z

How to manage active and passive buttons? and where ?( for example in the main branch: when open a library these buttons can be disable. If I find this event where it can help me with issue)

If I examine where and how it is managed, I will try to establish a similar structure, so I will look for a solution to issue redo and undo buttons are active without open library #11837.

Good questions.
I have not worked on this component, so @HoussemNasri would be able to give you more context. But I'll tell you how I would investigate this:
If we look at https://github.com/JabRef/jabref/pull/11758/files, we can see these that set the property undoable and redoable:

jabref/src/main/java/org/jabref/gui/undo/CountingUndoManager.java

Lines 27 to 31 in 4b8d668

    
           if (editAdded) { 
        
               incrementBalance(); 
        
               updateUndoableStatus(); 
        
               updateRedoableStatus(); 
        
               return true;

Thus, I would start at ~~UndoManager.java~~ CountingUndoManager.java(it contains logic for setting both undo and redo buttons).

subhramit · 2024-10-03T12:34:37Z

Thus, I would start at ~~UndoManager.java~~ CountingUndoManager.java(it contains logic for setting both undo and redo buttons).

More thoughts:
When no library is open (I think that translates to no open databases)

if (databases == null || databases.isEmpty())

Then the properties should be set to disabled.

subhramit · 2024-10-03T12:37:17Z

if (databases == null || databases.isEmpty())

@Siedlerchr can we use Injector to get BibDatabase here?
Update: Or stateManager.getActiveDatabase() would be better.

Siedlerchr · 2024-10-03T12:46:45Z

one liner
this.executable.bind(needsDatabase(stateManager)

melisolmez · 2024-10-04T15:14:04Z

Hi @subhramit, I am trying the fix problem. I noticed a few things and I wanted to share with you maybe they will help.

as first: I examined CountingUndoManager.java class with debug. While the project is getting off the ground, it does not fall into this class. It only entered this class when the library is added. When I added situation here and try to disable the buttons, they remain disabled throughout the application. so this was not the solution

second: As far as I understand, buttons were created using the RedoAction and UndoAction classes in the MainMenu.java class.And this one I added situation in RedoAction and UndoAction constructor and try to disable the buttons. but this solution didn't work either.

I don't know if I'm doing it right I'm just trying something
I will continue to review. Maybe some of the things you know will help me
(I just realized. Was this issue assigned to a @HoussemNasri ?)
thanks :)

HoussemNasri · 2024-10-04T15:25:47Z

Hello @melisolmez,

Thank you for your hard work in addressing this issue. The Undo/Redo feature is indeed quite challenging, especially given its current state, which requires significant refactoring.

We'll take over from here and continue refining the pull request to bring it to a mergeable state.

In the meantime, feel free to explore other good first issues and comment on any that interest you and someone will assign it to you.

…en libraries

HoussemNasri

Thank you again for your work! We truly appreciate your efforts and look forward to more contributions.

obsolete now

fix remove listeners in RedoAction for memory efficiency, used WeakCh…

e0668a0

…angeListeners

github-actions bot reviewed Sep 26, 2024

View reviewed changes

fix checkstyle error

67035ee

github-actions bot reviewed Sep 26, 2024

View reviewed changes

fix checkstyle error

6cfe017

subhramit previously requested changes Sep 26, 2024

View reviewed changes

subhramit changed the title ~~fix remove listeners in RedoAction for memory efficiency, used WeakCh…~~ Remove listeners in RedoAction for memory efficiency Sep 26, 2024

revert javadoc and fix catch indent

955d4da

koppor reviewed Sep 27, 2024

View reviewed changes

subhramit reviewed Sep 27, 2024

View reviewed changes

subhramit requested a review from HoussemNasri September 29, 2024 02:05

koppor mentioned this pull request Sep 29, 2024

redo and undo buttons are active without open library #11837

Closed

Remove listeners in UndoAction for memory efficiency

0f9088d

HoussemNasri self-assigned this Oct 4, 2024

HoussemNasri and others added 3 commits October 4, 2024 16:49

Use the fact that there is only one UndoManager instance shared betwe…

4c5e4d5

…en libraries

Update CHANGELOG.md

2effee8

Merge branch 'main' into fix-task-11809

769345c

HoussemNasri changed the title ~~Remove listeners in RedoAction for memory efficiency~~ Remove listeners in UndoAction and RedoAction for memory efficiency Oct 4, 2024

HoussemNasri approved these changes Oct 4, 2024

View reviewed changes

Siedlerchr approved these changes Oct 4, 2024

View reviewed changes

Siedlerchr added this pull request to the merge queue Oct 4, 2024

Merged via the queue into JabRef:main with commit ecd78de Oct 4, 2024
23 checks passed

subhramit mentioned this pull request Oct 5, 2024

Remove changelog entry #11882

Merged

6 tasks

Siedlerchr mentioned this pull request Oct 15, 2024

Can't use "Cleanup Entries" on more than one entry #11966

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove listeners in UndoAction and RedoAction for memory efficiency #11839

Remove listeners in UndoAction and RedoAction for memory efficiency #11839

melisolmez commented Sep 26, 2024 •

edited by HoussemNasri

Loading

github-actions bot left a comment

koppor commented Sep 26, 2024

github-actions bot left a comment

subhramit left a comment •

edited

Loading

subhramit Sep 26, 2024

subhramit Sep 26, 2024

koppor Sep 27, 2024

melisolmez Sep 27, 2024

koppor commented Sep 27, 2024

melisolmez commented Sep 27, 2024

subhramit commented Sep 27, 2024 •

edited

Loading

subhramit Sep 27, 2024

HoussemNasri commented Sep 29, 2024 •

edited

Loading

melisolmez commented Sep 30, 2024

Siedlerchr commented Sep 30, 2024 •

edited by subhramit

Loading

subhramit commented Sep 30, 2024 •

edited

Loading

melisolmez commented Sep 30, 2024

melisolmez commented Oct 1, 2024

subhramit commented Oct 1, 2024

subhramit commented Oct 2, 2024 •

edited

Loading

subhramit commented Oct 2, 2024 •

edited

Loading

subhramit commented Oct 2, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

melisolmez commented Oct 3, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

Siedlerchr commented Oct 3, 2024

melisolmez commented Oct 4, 2024 •

edited

Loading

HoussemNasri commented Oct 4, 2024

HoussemNasri left a comment

Remove listeners in UndoAction and RedoAction for memory efficiency #11839

Remove listeners in UndoAction and RedoAction for memory efficiency #11839

Conversation

melisolmez commented Sep 26, 2024 • edited by HoussemNasri Loading

Mandatory checks

github-actions bot left a comment

Choose a reason for hiding this comment

koppor commented Sep 26, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

subhramit left a comment • edited Loading

Choose a reason for hiding this comment

subhramit Sep 26, 2024

Choose a reason for hiding this comment

subhramit Sep 26, 2024

Choose a reason for hiding this comment

koppor Sep 27, 2024

Choose a reason for hiding this comment

melisolmez Sep 27, 2024

Choose a reason for hiding this comment

koppor commented Sep 27, 2024

melisolmez commented Sep 27, 2024

subhramit commented Sep 27, 2024 • edited Loading

subhramit Sep 27, 2024

Choose a reason for hiding this comment

HoussemNasri commented Sep 29, 2024 • edited Loading

melisolmez commented Sep 30, 2024

Siedlerchr commented Sep 30, 2024 • edited by subhramit Loading

subhramit commented Sep 30, 2024 • edited Loading

melisolmez commented Sep 30, 2024

melisolmez commented Oct 1, 2024

subhramit commented Oct 1, 2024

subhramit commented Oct 2, 2024 • edited Loading

subhramit commented Oct 2, 2024 • edited Loading

subhramit commented Oct 2, 2024 • edited Loading

subhramit commented Oct 3, 2024 • edited Loading

melisolmez commented Oct 3, 2024 • edited Loading

subhramit commented Oct 3, 2024 • edited Loading

subhramit commented Oct 3, 2024 • edited Loading

subhramit commented Oct 3, 2024 • edited Loading

Siedlerchr commented Oct 3, 2024

melisolmez commented Oct 4, 2024 • edited Loading

HoussemNasri commented Oct 4, 2024

HoussemNasri left a comment

Choose a reason for hiding this comment

melisolmez commented Sep 26, 2024 •

edited by HoussemNasri

Loading

subhramit left a comment •

edited

Loading

subhramit commented Sep 27, 2024 •

edited

Loading

HoussemNasri commented Sep 29, 2024 •

edited

Loading

Siedlerchr commented Sep 30, 2024 •

edited by subhramit

Loading

subhramit commented Sep 30, 2024 •

edited

Loading

subhramit commented Oct 2, 2024 •

edited

Loading

subhramit commented Oct 2, 2024 •

edited

Loading

subhramit commented Oct 2, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

melisolmez commented Oct 3, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

subhramit commented Oct 3, 2024 •

edited

Loading

melisolmez commented Oct 4, 2024 •

edited

Loading