Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove listeners in RedoAction for memory efficiency #11809

Closed
subhramit opened this issue Sep 23, 2024 · 14 comments · Fixed by #11839
Closed

Remove listeners in RedoAction for memory efficiency #11809

subhramit opened this issue Sep 23, 2024 · 14 comments · Fixed by #11839
Assignees
Labels
FirstTimeCodeContribution Triggers GitHub Greeter Workflow good first issue An issue intended for project-newcomers. Varies in difficulty. type: enhancement
Milestone

Comments

@subhramit
Copy link
Collaborator

subhramit commented Sep 23, 2024

Context: PR #11758
Based on the TODO comment:

// 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()));
});

Refs. #11758 (comment)

@subhramit
Copy link
Collaborator Author

Some hints:

  • Remove the listeners based on activeLibraryTab.
  • Alternatively, WeakChangeLIsteners can also be used.

@subhramit subhramit added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Sep 23, 2024
@github-project-automation github-project-automation bot moved this to Free to take in Good First Issues Sep 23, 2024
@subhramit subhramit changed the title Remove listeners for memory efficiency Remove listeners in RediAction for memory efficiency Sep 23, 2024
@subhramit subhramit changed the title Remove listeners in RediAction for memory efficiency Remove listeners in RedoAction for memory efficiency Sep 23, 2024
@subhramit subhramit added this to the 6.0-beta milestone Sep 23, 2024
@melisolmez
Copy link
Contributor

Hi, can ı work on this issue ?

@subhramit
Copy link
Collaborator Author

Hi, can ı work on this issue ?

Sure.

@subhramit subhramit added the FirstTimeCodeContribution Triggers GitHub Greeter Workflow label Sep 23, 2024
Copy link
Contributor

Welcome to the vibrant world of open-source development with JabRef!

Newcomers, we're excited to have you on board. Start by exploring our Contributing guidelines, and don't forget to check out our workspace setup guidelines to get started smoothly.

In case you encounter failing tests during development, please check our developer FAQs!

Having any questions or issues? Feel free to ask here on GitHub. Need help setting up your local workspace? Join the conversation on JabRef's Gitter chat. And don't hesitate to open a (draft) pull request early on to show the direction it is heading towards. This way, you will receive valuable feedback.

Happy coding! 🚀

@ThiloteE
Copy link
Member

ThiloteE commented Sep 24, 2024

@subhramit when assigning somebody, you also have to manually change the issue from "free to take" to "reserved" in the project. I tried to automate this, but GitHub Projects isn't capable of doing so, the last time I checked.
grafik

@ThiloteE ThiloteE moved this from Free to take to Reserved in Good First Issues Sep 24, 2024
@subhramit
Copy link
Collaborator Author

@subhramit when assigning somebody, you also have to manually change the issue from "free to take" to "reserved" in the project. I tried to automate this, but GitHub Projects isn't capable of doing so, the last time I checked.

Hey, I tried to change it, but I think only maintainers can do that.

@melisolmez
Copy link
Contributor

melisolmez commented Sep 25, 2024

Hello @subhramit, while I solve this isue I noticed this error:
error

Can we solve this problem with an if statement that checks for null in the RedoAction.excecute method?

@subhramit
Copy link
Collaborator Author

Can we solve this problem with an if statement that checks for null in the RedoAction.excecute method?

Did this error exist before? (if yes, then could you elaborate the steps to reproduce it?)

Else, if this was triggered after your changes, submit a PR so that we can review and make suggestions.

@melisolmez
Copy link
Contributor

Can we solve this problem with an if statement that checks for null in the RedoAction.excecute method?

Did this error exist before? (if yes, then could you elaborate the steps to reproduce it?)

Else, if this was triggered after your changes, submit a PR so that we can review and make suggestions.

yes this exist before, not beacufe of my changed. If you want you are try on your local.

@subhramit
Copy link
Collaborator Author

yes this exist before, not beacufe of my changed. If you want you are try on your local.

Yes, please write down the steps to reproduce the exception. It is also okay if you want to open another issue regarding it for more visibility, and then solve both the issues together.

@melisolmez
Copy link
Contributor

melisolmez commented Sep 25, 2024

yes this exist before, not beacufe of my changed. If you want you are try on your local.

Yes, please write down the steps to reproduce the exception. It is also okay if you want to open another issue regarding it for more visibility, and then solve both the issues together.

When I run jabref, the redo and undo buttons become active. When you press them directly without doing anything, this error occurs.
I can solve with this issue.

@Siedlerchr
Copy link
Member

The buttons should be disabled if no library is opened that is probably the issue here

@melisolmez
Copy link
Contributor

The buttons should be disabled if no library is opened that is probably the issue here

Yes but there are not disabled. If you want,You can open an issue about this error

@koppor
Copy link
Member

koppor commented Sep 25, 2024

The buttons should be disabled if no library is opened that is probably the issue here
Yes but there are not disabled. If you want,You can open an issue about this error

Do you refer to #463? Does this issue need to be reopened? If yes, please refine the issue. Make a screenshot and point out which actions should be disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FirstTimeCodeContribution Triggers GitHub Greeter Workflow good first issue An issue intended for project-newcomers. Varies in difficulty. type: enhancement
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants