Converter: Only use PyObjectRef, add tests

The converter shouldn't need to deal with PyObject * itself, but use
only PyObjectRef, which takes care of reference already.

Make the GIL state management easier to read.

Add a small test that shows how to pass Python objects to/from QML.

src/converter.h

@@ -19,7 +19,7 @@
 #ifndef PYOTHERSIDE_CONVERTER_H
 #define PYOTHERSIDE_CONVERTER_H
 
-typedef struct _object PyObject;
+#include "pyobject_ref.h"
 
 struct ConverterDate {
     ConverterDate(int y, int m, int d)
@@ -117,8 +117,7 @@ class Converter {
         virtual ConverterDate date(V&) = 0;
         virtual ConverterTime time(V&) = 0;
         virtual ConverterDateTime dateTime(V&) = 0;
-        // `pyObject` must keep the object's reference count constant.
-        virtual PyObject *pyObject(V&) = 0;
+        virtual PyObjectRef pyObject(V&) = 0;
 
         virtual V fromInteger(long long v) = 0;
         virtual V fromFloating(double v) = 0;
@@ -127,8 +126,7 @@ class Converter {
         virtual V fromDate(ConverterDate date) = 0;
         virtual V fromTime(ConverterTime time) = 0;
         virtual V fromDateTime(ConverterDateTime dateTime) = 0;
-        // `fromPyObject` must return a new reference to the object.
-        virtual V fromPyObject(PyObject *pyobj) = 0;
+        virtual V fromPyObject(const PyObjectRef &pyobj) = 0;
         virtual ListBuilder<V> *newList() = 0;
         virtual DictBuilder<V> *newDict() = 0;
         virtual V none() = 0;

src/pyobject_converter.h

@@ -202,7 +202,7 @@ class PyObjectConverter : public Converter<PyObject *> {
                     PyDateTime_DATE_GET_SECOND(o),
                     PyDateTime_DATE_GET_MICROSECOND(o) / 1000);
         }
-        virtual PyObject *pyObject(PyObject *&o) { return o; }
+        virtual PyObjectRef pyObject(PyObject *&o) { return PyObjectRef(o); }
 
         virtual PyObject * fromInteger(long long v) { return PyLong_FromLong((long)v); }
         virtual PyObject * fromFloating(double v) { return PyFloat_FromDouble(v); }
@@ -213,10 +213,7 @@ class PyObjectConverter : public Converter<PyObject *> {
         virtual PyObject * fromDateTime(ConverterDateTime v) {
             return PyDateTime_FromDateAndTime(v.y, v.m, v.d, v.time.h, v.time.m, v.time.s, v.time.ms * 1000);
         }
-        virtual PyObject * fromPyObject(PyObject *pyobj) {
-            Py_XINCREF(pyobj);
-            return pyobj;
-        }
+        virtual PyObject * fromPyObject(const PyObjectRef &pyobj) { return pyobj.newRef(); }
         virtual ListBuilder<PyObject *> *newList() { return new PyObjectListBuilder(); }
         virtual DictBuilder<PyObject *> *newDict() { return new PyObjectDictBuilder(); }
         virtual PyObject * none() { Py_RETURN_NONE; }

src/pyobject_ref.cpp

@@ -2,6 +2,7 @@
 /**
  * PyOtherSide: Asynchronous Python 3 Bindings for Qt 5
  * Copyright (c) 2014, Felix Krull <f_krull@gmx.de>
+ * Copyright (c) 2014, Thomas Perl <m@thp.io>
  *
  * Permission to use, copy, modify, and/or distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -18,44 +19,50 @@
 
 #include "pyobject_ref.h"
 
-class PyGIL {
+class EnsureGILState {
     public:
-        PyGIL(bool condition = true) : acquired(condition) {
-            if (acquired) { gil_state = PyGILState_Ensure(); }
-        }
-        ~PyGIL() {
-            if (acquired) { PyGILState_Release(gil_state); }
-        }
+        EnsureGILState() : gil_state(PyGILState_Ensure()) { }
+        ~EnsureGILState() { PyGILState_Release(gil_state); }
+
     private:
         PyGILState_STATE gil_state;
-        bool acquired;
 };
 
-PyObjectRef::PyObjectRef(PyObject *obj) : pyobject(obj) {
-    PyGIL gil(pyobject != NULL);
-    Q_UNUSED(gil);
-    Py_XINCREF(pyobject);
+#define ENSURE_GIL_STATE EnsureGILState _ensure; Q_UNUSED(_ensure)
+
+PyObjectRef::PyObjectRef(PyObject *obj)
+    : pyobject(obj)
+{
+    if (pyobject) {
+        ENSURE_GIL_STATE;
+        Py_INCREF(pyobject);
+    }
 }
 
-PyObjectRef::PyObjectRef(const PyObjectRef &other) : pyobject(other.pyobject) {
-    PyGIL gil(pyobject != NULL);
-    Q_UNUSED(gil);
-    Py_XINCREF(pyobject);
+PyObjectRef::PyObjectRef(const PyObjectRef &other)
+    : pyobject(other.pyobject)
+{
+    if (pyobject) {
+        ENSURE_GIL_STATE;
+        Py_INCREF(pyobject);
+    }
 }
 
-PyObjectRef::~PyObjectRef() {
-    PyGIL gil(pyobject != NULL);
-    Q_UNUSED(gil);
-    Py_CLEAR(pyobject);
+PyObjectRef::~PyObjectRef()
+{
+    if (pyobject) {
+        ENSURE_GIL_STATE;
+        Py_CLEAR(pyobject);
+    }
 }
 
-PyObject *PyObjectRef::newRef() const {
-    PyGIL gil(pyobject != NULL);
-    Q_UNUSED(gil);
-    Py_XINCREF(pyobject);
-    return pyobject;
-}
-
-PyObject *PyObjectRef::getPyObject() const {
+PyObject *
+PyObjectRef::newRef() const
+{
+    if (pyobject) {
+        ENSURE_GIL_STATE;
+        Py_INCREF(pyobject);
+    }
+
     return pyobject;
 }

src/pyobject_ref.h

@@ -2,6 +2,7 @@
 /**
  * PyOtherSide: Asynchronous Python 3 Bindings for Qt 5
  * Copyright (c) 2014, Felix Krull <f_krull@gmx.de>
+ * Copyright (c) 2014, Thomas Perl <m@thp.io>
  *
  * Permission to use, copy, modify, and/or distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -25,11 +26,12 @@
 
 class PyObjectRef {
     public:
-        explicit PyObjectRef(PyObject *obj = NULL);
+        explicit PyObjectRef(PyObject *obj=0);
         PyObjectRef(const PyObjectRef &other);
         virtual ~PyObjectRef();
+
         PyObject *newRef() const;
-        PyObject *getPyObject() const;
+
     private:
         PyObject *pyobject;
 };

src/qvariant_converter.h

@@ -20,7 +20,6 @@
 #define PYOTHERSIDE_QVARIANT_CONVERTER_H
 
 #include "converter.h"
-#include "pyobject_ref.h"
 
 #include <QVariant>
 #include <QTime>
@@ -193,8 +192,8 @@ class QVariantConverter : public Converter<QVariant> {
             return stringstorage.constData();
         }
 
-        virtual PyObject *pyObject(QVariant &v) {
-            return v.value<PyObjectRef>().getPyObject();
+        virtual PyObjectRef pyObject(QVariant &v) {
+            return v.value<PyObjectRef>();
         }
 
         virtual ListBuilder<QVariant> *newList() {
@@ -216,8 +215,8 @@ class QVariantConverter : public Converter<QVariant> {
             QTime t(v.time.h, v.time.m, v.time.s, v.time.ms);
             return QVariant(QDateTime(d, t));
         }
-        virtual QVariant fromPyObject(PyObject *pyobj) {
-            return QVariant::fromValue(PyObjectRef(pyobj));
+        virtual QVariant fromPyObject(const PyObjectRef &pyobj) {
+            return QVariant::fromValue(pyobj);
         }
         virtual QVariant none() { return QVariant(); };
 

tests/test_wrapped/test_wrapped.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+
+class Foo(object):
+    def __init__(self, name):
+        print('new Foo(', name, ')')
+        self.name = name
+
+    def __del__(self):
+        print('__del__ called on', self.name)
+
+
+def get_foo():
+    print('get_foo()')
+    return Foo('Hello World!')
+
+def set_foo(foo):
+    print('set_foo(', foo, ')')
+    return foo.name
+

tests/test_wrapped/test_wrapped.qml

@@ -0,0 +1,28 @@
+import QtQuick 2.0
+import io.thp.pyotherside 1.0
+
+Rectangle {
+    id: page
+    width: 300
+    height: 300
+
+    Python {
+        Component.onCompleted: {
+            addImportPath(Qt.resolvedUrl('.'));
+
+            importModule('test_wrapped', function () {
+                console.log('"test_wrapped" imported successfully');
+
+                var foo = call_sync('test_wrapped.get_foo', []);
+                console.log('got foo: ' + foo);
+
+                var result = call_sync('test_wrapped.set_foo', [foo]);
+                console.log('got result: ' + result);
+            });
+        }
+
+        onError: {
+            console.log('Received error: ' + traceback);
+        }
+    }
+}

tests/tests.cpp

@@ -95,9 +95,14 @@ test_converter_for(Converter<V> *conv)
 
     /* Convert from/to generic PyObject */
     PyObject *obj = PyCapsule_New(conv, "test", NULL);
-    v = conv->fromPyObject(obj);
+    v = conv->fromPyObject(PyObjectRef(obj));
     QVERIFY(conv->type(v) == Converter<V>::PYOBJECT);
-    QVERIFY(conv->pyObject(v) == obj);
+
+    // Check if getting a new reference works
+    PyObject *o = conv->pyObject(v).newRef();
+    QVERIFY(o == obj);
+    Py_DECREF(o);
+
     Py_CLEAR(obj);
 
     delete conv;
@@ -123,6 +128,7 @@ TestPyOtherSide::testPyObjectRefRoundTrip()
     QVariant v = convertPyObjectToQVariant(o);
 
     // Decrement refcount and pass QVariant to QML.
+    QVERIFY(o->ob_refcnt == 2);
     Py_DECREF(o);
     QVERIFY(o->ob_refcnt == 1);
 
@@ -131,14 +137,20 @@ TestPyOtherSide::testPyObjectRefRoundTrip()
     PyObject *o2 = convertQVariantToPyObject(v);
     QVERIFY(o->ob_refcnt == 2);
 
-    // Do something with the object, then decrement its refcount.
-    Py_DECREF(o2);
-    QVERIFY(o->ob_refcnt == 1);
-
     // The QVariant is deleted, i.e. by a JS variable falling out of scope.
     // This deletes the PyObjectRef and thus decrements the object's refcount.
     v = QVariant();
 
+    // At this point, we only have one reference (the one from o2)
+    QVERIFY(o->ob_refcnt == 1);
+
+    // There's still a reference, so the destructor must not have been called
+    QVERIFY(!destructor_called);
+
+    // Now, at this point, the last remaining reference is removed, which
+    // will cause the destructor to be called
+    Py_DECREF(o2);
+
     // There are no references left, so the capsule's destructor is called.
     QVERIFY(destructor_called);
 }