Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #468 +/- ##
===========================================
- Coverage 36.07% 31.07% -5.01%
===========================================
Files 198 196 -2
Lines 27621 27220 -401
Branches 4582 4548 -34
===========================================
- Hits 9964 8458 -1506
- Misses 16779 17947 +1168
+ Partials 878 815 -63
Continue to review full report at Codecov.
|
campagnola
reviewed
May 1, 2017
|
|
||
| ## Convenience functions for command-line use | ||
|
|
||
| plots = [] |
Member
There was a problem hiding this comment.
These references should be necessary to ensure the plots stay alive as long as they the user keeps them open. Perhaps we can have the references automatically removed when the window is closed?
Contributor
Author
There was a problem hiding this comment.
Alright, I'll try to keep the references, and remove them on window close.
1659f5f to
d243cce
Compare
There is no need to wrap PlotView/ImageView into QMainWindow, since only purpose of the QMainWindow is some default menu toolbar & menu handling, that is not used by PyQtGraph anyway. Moreover, every parent-less Qt widget can become window, so this change just use PlotView/ImageView as windows, removing extra complexity, eg. method forwarding, self.win property. Another benefit of this change, it that these windows get initial dimensions and titles as they were designed in .ui file.
We should not close explicitly child widgets or clear scene, otherwise Qt will deallocate children views, and cause "wrapped C/C++ object of type ImageItem has been deleted" error next time we call close() and/or some other methods. All children, including self.ui.roiPlot, self.ui.graphicsView will be closed together with its parent, so there is no need to close them explicitly. So the purpose of close it to reclaim the memory, but not to make the existing ImageView object dysfunctional.
PyQtGraph images and plots module list variables are currently holding references to all plots and image windows returned directly from main module. This does not seem to be documented however, and causes the Qt windows to be not released from memory, even if user releases all own references. This change removes the references from images/plots list once window is closed, so when there is no other reference, window and all related memory is reclaimed.
d243cce to
d81bf21
Compare
Contributor
Author
|
@campagnola Hi I have updated my PR, but I have no idea why Travis fails with |
Member
|
Hi @nanoant sorry it's taken us so long to get to this. I fixed a merge conflict, this looks good to me. I'll be merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR reduces number of references to PyQtGraph windows implicitly hold by the package. Also removes some memory leak in
ImageView.close()not taking care ofself.imageItem.This PR is related to #466