pyapp-kit / ndv

Simple, fast-loading, n-dimensional array viewer with minimal dependencies.
BSD 3-Clause "New" or "Revised" License
29 stars 4 forks source link

Fix leakage in Windows #27

Closed hanjinliu closed 1 month ago

hanjinliu commented 1 month ago

Maybe related to #8. I found that qthrottled holds the reference to the widget instance (via method.__self__?) and this prevented proper cleanups in Windows. This problem can be solved by using an weak reference inside a local function as is done in this PR, but probably should be somehow handled on superqt side (although I'm not sure if it's possible).

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (e606d53) to head (4cf3f81).

Files Patch % Lines
src/ndv/viewer/_backends/_vispy.py 66.66% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #27 +/- ## ======================================= Coverage 81.92% 81.93% ======================================= Files 13 13 Lines 1234 1251 +17 ======================================= + Hits 1011 1025 +14 - Misses 223 226 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tlambert03 commented 1 month ago

Hey @hanjinliu, lovely seeing you here! 😀

I saw this as well and added https://github.com/pyapp-kit/superqt/issues/247 to superqt (which is in the latest release)... but I haven't tested on windows yet!

Are you definitely updated on superqt?

hanjinliu commented 1 month ago

Hi @tlambert03, I'm very excited about this package! Sorry for not carefully checking the update... I was using superqt=0.6.6 and the latest version did solve the issue with my local windows laptop.

tlambert03 commented 1 month ago

Great! Good to hear :)

tlambert03 commented 1 month ago

Shall we still turn this PR into fixing whatever else is left for windows tests?

(It's ok if we need to make some leak concessions, as you'll see I've already done with Linux)

hanjinliu commented 1 month ago

Other fixes will be completely different from what I did here anyway. I think we can just close this PR and make new one later when the solution is found.

tlambert03 commented 1 month ago

Ok well thanks for popping in!