Skip to content

Conversation

@ruspl-afed
Copy link
Contributor

  • implement dispose for TmfPieChartViewer
  • use dispose properly

What 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 TmfStatisticsViewer should be present.

Follow-ups

I plan to fix similar problems for other views

Review checklist

  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

* implement dispose for `TmfPieChartViewer`
* use `dispose` properly
Copy link
Contributor

@bhufmann bhufmann left a 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!

@bhufmann bhufmann requested a review from MatthewKhouzam July 23, 2025 16:58
@ruspl-afed
Copy link
Contributor Author

Thank you for your review @bhufmann !
We need to get @MatthewKhouzam 's attention somehow to advance with PRs :)
I'll try to find something non-conflicting to fix while this PR is waiting.

@bhufmann bhufmann requested review from PatrickTasse and removed request for MatthewKhouzam August 20, 2025 14:00
}

@Override
public void dispose() {
Copy link
Contributor

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.

Copy link
Contributor

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().

Copy link
Contributor Author

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.

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