-
Notifications
You must be signed in to change notification settings - Fork 27
Resolve "SWT Resource was not properly disposed" for "Statistics" #297
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
base: master
Are you sure you want to change the base?
Resolve "SWT Resource was not properly disposed" for "Statistics" #297
Conversation
* implement dispose for `TmfPieChartViewer` * use `dispose` properly
bhufmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Test and no more such exceptions. Thank!
|
Thank you for your review @bhufmann ! |
| } | ||
|
|
||
| @Override | ||
| public void dispose() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually not the intended SWT design to require dispose() to also dispose the children.
The method Widget.dispose() is not a cleanup method, it's a trigger for the application to remove a widget (and all its children). Internally it's the Widget.release() method that gets called recursively, automatically.
If a widget has cleanup to do, it should be done in a DisposeListener. This will always be called when the widget is removed, while it's not guaranteed that dispose() will ever be called, it might only be called on one of the widget's parents.
The real source of the problem is the implementation in org.eclipse.swtchart.Chart. It holds some system resources (e.g. Font in the AxisTitle) that are only released if Chart.dispose() is called, which is not normally called. Because of this implementation, we have to make sure to call Chart.dispose() on our child pie charts.
However, to avoid repeating the mistake, we should do this in a DisposeListener added in the constructor. There is already one, but it should be added to 'this', not to the parent. The disposal of the two pie charts can be moved to that listener. Then this overload of dispose() is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a clarification, it works currently since TmfPieChartViewer is only a child of TmfStatisticsViewer, but I want to protect in case it is ever put as a child of any other Composite that might not call dispose().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very grateful for your comments @PatrickTasse
Please give me some time to revisit SWT API and find better formulation for this change.
TmfPieChartViewerdisposeproperlyWhat it does
The PR introduces proper handling of SWT resources for "Statistics" view
How to test
Open "Statistics" view, do something, close it: the console should have no "SWT Resource was not properly disposed" initiated from
TmfStatisticsViewershould be present.Follow-ups
I plan to fix similar problems for other views
Review checklist