Skip to content
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

Fix leakage in Windows #27

Closed
wants to merge 3 commits into from
Closed

Conversation

hanjinliu
Copy link
Contributor

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

Copy link

codecov bot commented Jun 14, 2024

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 ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #27   +/-   ##
=======================================
  Coverage   81.92%   81.93%           
=======================================
  Files          13       13           
  Lines        1234     1251   +17     
=======================================
+ Hits         1011     1025   +14     
- Misses        223      226    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlambert03
Copy link
Member

Hey @hanjinliu, lovely seeing you here! 😀

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

Are you definitely updated on superqt?

@hanjinliu
Copy link
Contributor Author

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
Copy link
Member

Great! Good to hear :)

@tlambert03
Copy link
Member

tlambert03 commented Jun 14, 2024

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
Copy link
Contributor Author

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.

@hanjinliu hanjinliu closed this Jun 15, 2024
@tlambert03
Copy link
Member

Ok well thanks for popping in!

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.

2 participants