Skip to content

fix caret timeline memory leak#435

Merged
johanvos merged 3 commits intogluonhq:mainfrom
johanvos:434-caret
Mar 13, 2026
Merged

fix caret timeline memory leak#435
johanvos merged 3 commits intogluonhq:mainfrom
johanvos:434-caret

Conversation

@johanvos
Copy link
Copy Markdown
Contributor

Stop timeline (and remove it from the GC root) when the ParagraphTile is removed from the scenegraph

@eugener
Copy link
Copy Markdown
Collaborator

eugener commented Mar 13, 2026

Minor observation:
caretTimeline.play() in the newScene != null branch runs unconditionally, meaning all layers (not just the one with the caret) briefly start their timeline on scene attachment. This is harmless in practice because setCaretVisibility gates on !caretShape.getElements().isEmpty() (making the animation frames no-ops), and updateCaretPosition() will quickly stop the timeline for non-caret layers. A slightly more precise version would
guard with if (hasCaret()) before playing, but it's not worth complicating the fix for.

@johanvos
Copy link
Copy Markdown
Contributor Author

Minor observation: caretTimeline.play() in the newScene != null branch runs unconditionally, meaning all layers (not just the one with the caret) briefly start their timeline on scene attachment. This is harmless in practice because setCaretVisibility gates on !caretShape.getElements().isEmpty() (making the animation frames no-ops), and updateCaretPosition() will quickly stop the timeline for non-caret layers. A slightly more precise version would guard with if (hasCaret()) before playing, but it's not worth complicating the fix for.

Good remark.
I added the hasCaret() check. It is a non-intrusive say check, and I believe it's the right thing to do. I'm not sure about the cost (including object creation) of starting a timeline (even without actions). Since this code runs on the FX thread, every single instruction counts.

@johanvos johanvos merged commit 4d1eca8 into gluonhq:main Mar 13, 2026
3 checks passed
@johanvos johanvos deleted the 434-caret branch March 13, 2026 08:25
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.

2 participants