Skip to content

ParagraphText: immediately remove listeners from selections and carets on disposal#791

Merged
Jugen merged 5 commits intoFXMisc:masterfrom
JFormDesigner:dispose-paragraph-text-listeners
Dec 16, 2018
Merged

ParagraphText: immediately remove listeners from selections and carets on disposal#791
Jugen merged 5 commits intoFXMisc:masterfrom
JFormDesigner:dispose-paragraph-text-listeners

Conversation

@JFormDesigner
Copy link
Copy Markdown
Contributor

This PR immediately removes listeners (in class ParagraphText) from SelectionPath and Caret on disposal instead of waiting for GC (this improves PR #779, issue #627). Without this change, disposed ParagraphText objects may still listen to selection and caret and do some useless stuff.

Since the weak listeners added in PR #779 are no longer necessary, I've removed the listener classes again and moved selection and caret listeners code back to constructor (saves 100 lines of code).

…ret on dispose instead of waiting for GC (this improves PR FXMisc#779, issue FXMisc#627)

without this change, disposed ParagraphText objects still listen to selection and caret and do some useless stuff
…ion and caret listeners, remove listener classes again and move listener code to constructor (saves 100 lines of code)
…ay both return true when replacing an item. So check both and handle wasRemoved() before wasAdded().

(don't know whether this may occur in RichTextFX, but want to be on the safe side)
@JFormDesigner
Copy link
Copy Markdown
Contributor Author

Forgot to mention that I've debugged the change and also used VisualVM to check whether the change still fixes the old memory leak (that was fixed in PR #779).

@JonathanMarchand would you be so kind and also test this PR? Thx

@Jugen Jugen merged commit 7be6dc4 into FXMisc:master Dec 16, 2018
@JFormDesigner JFormDesigner deleted the dispose-paragraph-text-listeners branch March 12, 2019 10:22
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.

3 participants