Issue #777 (Regression test and fix proposal)

[m-kuhn]

This test fails, but will pass, as soon as issue 777 gets fixed.
I'm not sure if we want to merge it already, but I guess a failing test will remind us of this issue.
http://hub.qgis.org/issues/777

[m-kuhn]

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)

Edit:
To complete the confusion:
I just noticed, that setGeometryAndOwnership takes a WKB string as input, while geometryAndOwnership returns a QgsGeometry.

[wonder-sk]

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.

[m-kuhn]

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 [1] 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?

[1]:

from PyQt4.QtCore import *

class A:
    def __init__(self):
        o = QObject()
        self.child = QObject(o)

a = A()
a.child.objectName()

[wonder-sk]

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.

[m-kuhn]

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).
I imagine something like a check in the destructor, if there are any python references left to this geometry. In case there are, make a copy of the geometry, update existing references and register with the garbage collector. Something like "copy on write" it would be a "copy on delete". Would that work?

Surely QObject adds some overhead. And as deleteLater() also only solves part of the problem it's probably not worth it.