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

[#1318] Fix leaking fragments #1319

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mswokk
Copy link

@mswokk mswokk commented Mar 16, 2021

Fix leaking Fragments.

Remove windowProperty() listener before undocking.

And some refactoring for listeners registering for dock/undock.
Move listener holders from properties (map) to backing field by lazy() kotlin to reduce indent and lines.

Even the fixes pass the test cases, I couldn't find a way to check memory leaking.
It should be verified manually using jmap histogram dump.

Please review the code.

And some refactoring for listeners registering.
Move listener holders from properties map to backing field by lazy()
kotlin to reduce indent and lines.
@mswokk
Copy link
Author

mswokk commented Mar 17, 2021

Thank you for review @SKeeneCode

The rootParentChangeListener instance is still hold by the property rootParentChangeListener
I just moved the reference from Component.properties to property rootParentChangeListener explicitly.
So it doesn't break the bug fix for docking.

@SKeeneCode
Copy link

SKeeneCode commented Mar 31, 2021

I think this looks good - could you apply this bugfix to the tornadofx2 repo and then I can modify the reference in the workspace (or you could do it).

I actually think the way I went about fixing the bug was a poor way. I think instead of removing and re-adding the listener I can just do muteDocking = false. Perhaps we can add that correction onto the pull request.

@guizmaii
Copy link

guizmaii commented Aug 5, 2021

Hi @SKeeneCode,

I've done what you're asking here: edvin/tornadofx2#40

edvin added a commit to edvin/tornadofx2 that referenced this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants