Skip to content

Fix leaky QT widgets #108

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 6 commits into from
May 18, 2023
Merged

Fix leaky QT widgets #108

merged 6 commits into from
May 18, 2023

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented May 15, 2023

This PR enables a check for leaky QT widgets. This is done in conftest.py by setting the "NAPARI_STRICT_QT" environment variable during the tests.

To make the the widgets watertight I had to do two things:

  • Consistently use the parent keyword argument to make sure all widgets have a parent widget. This is important, because parent widgets will pro-actively close any of their children when they themselves are closed.
  • Re-write the features scatter widget to not use magicgui. Previously there was a circular reference that prevented the widget and one of the menus magicgui created from being cleaned up. I think the new code is a bit more readable if anything, and makes it clear where callbacks are between the combo boxes and the graph drawing. To test this run examples/features_scatter.py and have a play around with the widget.

Fixes #109

@dstansby dstansby force-pushed the leaks branch 2 times, most recently from 1123512 to 92186d1 Compare May 15, 2023 09:16
@dstansby
Copy link
Member Author

I've managed to fix them all, apart from FeaturesScatterWidget. Next step is to work out what's special about that widget that's causing it to fail.

@dstansby dstansby mentioned this pull request May 15, 2023
@dstansby dstansby force-pushed the leaks branch 4 times, most recently from 215e185 to 221e479 Compare May 17, 2023 12:47
@dstansby dstansby force-pushed the leaks branch 3 times, most recently from fda8647 to 1a7324d Compare May 17, 2023 14:42
@dstansby dstansby marked this pull request as ready for review May 17, 2023 15:27
@dstansby dstansby changed the title Check for leaky QT widgets Fix leaky QT widgets May 17, 2023
@dstansby dstansby requested review from ruaridhg and samcunliffe and removed request for ruaridhg May 17, 2023 15:34
@dstansby dstansby added the Tests label May 18, 2023
Copy link
Collaborator

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

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

LGTM

@ruaridhg ruaridhg added this pull request to the merge queue May 18, 2023
Merged via the queue into matplotlib:main with commit fe24d4f May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix leaking QT widgets
2 participants