Hi all,
I have observed the same in the past.
This explanation sounds very reasonable, Martin.
Are you convinced that the proper solution will be a /ReleaseReference/
annotation?
I would assume that the current information is sufficient for sip /
sipify to generate proper bindings without a new annotation.
From what I understand, sip creates an internal subclass that forwards
virtual methods to their python implementations. I would assume that the
destructor of this subclass can be used to trigger the garbage
collection / decreasing the ref counter and the reference counter should
be increased when the object is sent through a /Transfer/ annotated
function parameter.
If that works, there's still work to be done, but the fix wouldn't
affect the QGIS source itself.
Matthias
On 07.01.19 12:11, Martin Dobias wrote:
Hi Nyall
I have seen that too with other QGIS registries, but I am not sure if
it is actually broken in SIP or if this should be simply handled in a
different way.
My understanding is that SIP for an object has the "c++ part" and
"python part". If we do not deal with subclasses, things seem to work
nicely: if you pass object of type X to function "myfunc(X* x
/Transfer/)" then the object gets ownership passed to C++, and then
when in python the object gets out of scope, its "python part" get
deleted because it is not necessary anymore and only the "c++ part"
keeps on living (and will be deleted in c++ code. The trouble is when
passing derived classes with /Transfer/ where the "python part" is
actually important, but it still gets deleted.
SIP also has /KeepReference/ annotation, but it's probably not that
useful here, because it is meant to keep reference to one python
object passed through API (if I had a function addCheck with
/KeepReference/ arg annotation then the first call to addCheck(a) will
keep the reference, but the second call addCheck(b) would drop
reference to "a"). The trouble is that if we want SIP hold reference
to the python object, we would need to give it extra information about
the object's life cycle (when is it safe to release the reference). So
we would probably need a /KeepReference/ variant with a matching
/ReleaseReference/ annotation for some other function - like this SIP
could track of multiple objects (which is the case of registries) and
handle their life cycle correctly. Maybe this registry pattern is not
used anywhere in Qt so SIP does not include support for it...
A simple "fix" is to just keep the reference to the object in the
python code. Not the nicest solution I know...
Cheers
Martin
On Thu, Jan 3, 2019 at 8:19 AM Nyall Dawson <nyall.daw...@gmail.com> wrote:
Hi list,
I'm at my wits end here, fighting with a sip issue I can't solve. And
honestly, I'm starting to think maybe there's something completely
broken in sip. (Or at least, in the version 4.19.7 provided by my
distro).
Here's what I'm currently hitting:
The validity check registry added this week has a method "addCheck",
which transfers ownership of the check to c++:
void addCheck( QgsAbstractValidityCheck *check SIP_TRANSFER );
https://github.com/qgis/QGIS/blob/master/src/core/validity/qgsvaliditycheckregistry.h#L65
But if I make a Python subclass of QgsAbstractValidityCheck, and add
it to the registry, it's immediately garbage collected by Python!
My test code looks like this:
class LayoutMapCrsCheck(QgsAbstractValidityCheck):
def __del__(self):
print('WTFFFFFFF!!!')
def id(self):
return 'map_crs_check'
def checkType(self):
return QgsAbstractValidityCheck.TypeLayoutCheck
def prepareCheck(self,context, feedback):
return True
def runCheck(self,context, feedback):
return []
def add_check():
QgsApplication.validityCheckRegistry().addCheck(LayoutMapCrsCheck())
add_check()
So I create a Python subclass of the interface, and then add it to the
registry. And boom - it's immediately deleted.
If I make a global copy of the check, and then add to the registry,
everything works -- the check isn't garbage collected.
check_instance = LayoutMapCrsCheck()
QgsApplication.validityCheckRegistry().addCheck(check_instance)
But this leads me to believe that sip is broken somewhere, and is
ignoring the /Transfer/ annotation.
Indeed - I've also seen this in other classes. Specifically
QgsTaskManager.addTask() -- Python tasks must be stored globally in
order to avoid them being immediately deleted too.
Can anyone else reproduce using my above code? Does anyone have any
ideas what's happening here?
Nyall
_______________________________________________
QGIS-Developer mailing list
QGIS-Developer@lists.osgeo.org
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
_______________________________________________
QGIS-Developer mailing list
QGIS-Developer@lists.osgeo.org
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
_______________________________________________
QGIS-Developer mailing list
QGIS-Developer@lists.osgeo.org
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
