8000 Plug leaking Stroke in StrokeHandler::strokeRecognizerDetected by bhennion · Pull Request #4258 · xournalpp/xournalpp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Plug leaking Stroke in StrokeHandler::strokeRecognizerDetected #4258

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

Merged
merged 1 commit into from
Oct 2, 2022

Conversation

bhennion
Copy link
Collaborator

This PR plugs a memory leak happening when the stroke recognizer replaces the stroke.
Esssentially, StrokeHandler::strokeRecognizerDetected clones the recognized stroke (i.e. the result of the recognition process) and adds the clone to the layer. The recognized stroke (not the clone) is then never deleted, and is no longer referenced anywhere once the StrokeRecognizerResult gets deleted.

This PR just removes the clone and directly adds the recognized stroke to the layer (which then owns it).

@bhennion bhennion requested a review from rolandlo September 29, 2022 18:23
Copy link
Member
@rolandlo rolandlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bhennion
Copy link
Collaborator Author
bhennion commented Oct 1, 2022

Thanks @rolandlo for the review!
Merging in 24 hours unless someone objects.

@bhennion bhennion added this to the v1.1.2 milestone Oct 1, 2022
@bhennion bhennion added the merge proposed Merge was proposed by maintainer label Oct 1, 2022
@bhennion bhennion merged commit f90c478 into xournalpp:release-1.1 Oct 2, 2022
@bhennion bhennion deleted the pr/plugMemLeaks branch October 2, 2022 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge proposed Merge was proposed 47AA by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0