Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Issue #777 (Regression test and fix proposal) #436
I added also a fix.
When QgsFeature::geometry is called from python, by default, a copy of the geometry is returned and ownership is transferred to python, so it gets garbage collected, and the qgis application keeps no reference to this copy.
IMHO, geometryAndOwnership should be deprecated as it seems to me, it was originally introduced as a work around for this problem, and it can lead to problems (untested problem description: call it once from python without assigning the return value: the geometry gets garbage collected, call it a second time and you will get a reference to a deleted object)
I don't really like this fix - it fixes just one particular instance of a more general error with object ownership. It was suggested by sip/pyqt developers to use virtual destructors for these issues - python wrapper should be notified on destruction and raise an exception saying that underlying c++ object does not exist anymore.
It seems to me, that raising an exception does not really solve the issue, but just covers it a little more beautiful.
Calling a method directly on a returned object should be a valid thing to do.
I see, that the sip/pyqt developers "solve" their problem the way you mentioned as in this demo  based on qt ownership. I personally just don't consider this a solution.
Maybe there are other approaches such as mixed ref-counting (C++ and python references) or using deleteLater(). Using deleteLater would only solve the one-line issue, while it still lets C++ silently delete child objects.
I would still opt for the solution I proposed here. Or are you afraid of memory problems when the geometry is always copied rather than referenced?
Right, that does not solve the problem, it just lets you know about such problem. I agree that it would be nice to have such one-liner working, but we would need a generic solution for that. That's why I don't like that solution: we would have to do such thing for all cases where an instance gives access to a contained object. Second problem is that copying will break things that work, e.g. feature.geometry().moveVertex(...) would update only a local copy and then throw it away. Third problem are likely crashes if someone decides to use feature.geometry(false) because of double deletions.
How would you imagine mixed ref-counting between c++ and python? I guess this would require us to use smart pointers everywhere and these smart pointers would include python incref and decref calls - not something I would like to see.
For deleteLater() this would mean that all classes would have to be derived from QObject - again that looks like a rather big hammer.
We will have to do a change for all cases where an instance gives access to a contained object in every case. Be it copying or implementing a virtual destructor.
The second problem is indeed a hard-to-notice API change, which will break code without even letting the developer know that something changed. But at least it would only apply for python code, as the default param for C++ code will still be to get a pointer to the feature geometry and not to a copy.
The third problem you mentioned could be worked around with some sip code I guess?
I don't really have an idea which I can guarantee will work concerning the ref-counting (I'm not a sip expert).
Surely QObject adds some overhead. And as deleteLater() also only solves part of the problem it's probably not worth it.