[pygobject] Move property and signal creation into _class_init()

[Martin Pitt <martinpitt src gnome org>]

commit efcb0f9fda65e24ae98438d61487d06db9eac1b1
Author: Martin Pitt <martinpitt gnome org>
Date:   Sat Nov 3 16:14:01 2012 +0100

    Move property and signal creation into _class_init()
    
    We must not add class interfaces after g_type_class_ref() has been called the
    first time. Move signal and property creation from pyg_type_register() into
    pyg_object_class_init(), and drop the hack of registering interfaces twice.
    
    This changed class initialization order now exposes GLib's warning about
    unknown signals, so adjust test_signal.TestGSignalsError.test_invalid_name() to
    not abort on that.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=686149

 gi/_gobject/gobjectmodule.c |  177 +++++++++++++++++--------------------------
 tests/test_signal.py        |    6 +-
 2 files changed, 75 insertions(+), 108 deletions(-)
---
diff --git a/gi/_gobject/gobjectmodule.c b/gi/_gobject/gobjectmodule.c
index 0d62a42..6c3aa5f 100644
--- a/gi/_gobject/gobjectmodule.c
+++ b/gi/_gobject/gobjectmodule.c
@@ -225,13 +225,6 @@ pyg_object_get_property (GObject *object, guint property_id,
     pyglib_gil_state_release(state);
 }
 
-static void
-pyg_object_class_init(GObjectClass *class, PyObject *py_class)
-{
-    class->set_property = pyg_object_set_property;
-    class->get_property = pyg_object_get_property;
-}
-
 typedef struct _PyGSignalAccumulatorData {
     PyObject *callable;
     PyObject *user_data;
@@ -397,15 +390,14 @@ override_signal(GType instance_type, const gchar *signal_name)
 }
 
 static PyObject *
-add_signals (GType instance_type, PyObject *signals)
+add_signals (GObjectClass *klass, PyObject *signals)
 {
     gboolean ret = TRUE;
-    GObjectClass *oclass;
     Py_ssize_t pos = 0;
     PyObject *key, *value, *overridden_signals = NULL;
+    GType instance_type = G_OBJECT_CLASS_TYPE (klass);
 
     overridden_signals = PyDict_New();
-    oclass = g_type_class_ref(instance_type);
     while (PyDict_Next(signals, &pos, &key, &value)) {
 	const gchar *signal_name;
         gchar *signal_name_canon, *c;
@@ -443,7 +435,6 @@ add_signals (GType instance_type, PyObject *signals)
 	if (!ret)
 	    break;
     }
-    g_type_class_unref(oclass);
     if (ret)
         return overridden_signals;
     else {
@@ -716,14 +707,12 @@ pyg_param_spec_from_object (PyObject *tuple)
 }
 
 static gboolean
-add_properties (GType instance_type, PyObject *properties)
+add_properties (GObjectClass *klass, PyObject *properties)
 {
     gboolean ret = TRUE;
-    GObjectClass *oclass;
     Py_ssize_t pos = 0;
     PyObject *key, *value;
 
-    oclass = g_type_class_ref(instance_type);
     while (PyDict_Next(properties, &pos, &key, &value)) {
 	const gchar *prop_name;
 	GType prop_type;
@@ -789,7 +778,7 @@ add_properties (GType instance_type, PyObject *properties)
 	Py_DECREF(slice);
 
 	if (pspec) {
-	    g_object_class_install_property(oclass, 1, pspec);
+	    g_object_class_install_property(klass, 1, pspec);
 	} else {
             PyObject *type, *value, *traceback;
 	    ret = FALSE;
@@ -799,7 +788,7 @@ add_properties (GType instance_type, PyObject *properties)
                 g_snprintf(msg, 256,
 			   "%s (while registering property '%s' for GType '%s')",
                PYGLIB_PyUnicode_AsString(value),
-			   prop_name, g_type_name(instance_type));
+			   prop_name, G_OBJECT_CLASS_NAME(klass));
                 Py_DECREF(value);
                 value = PYGLIB_PyUnicode_FromString(msg);
             }
@@ -808,11 +797,63 @@ add_properties (GType instance_type, PyObject *properties)
 	}
     }
 
-    g_type_class_unref(oclass);
     return ret;
 }
 
 static void
+pyg_object_class_init(GObjectClass *class, PyObject *py_class)
+{
+    PyObject *gproperties, *gsignals, *overridden_signals;
+    PyObject *class_dict = ((PyTypeObject*) py_class)->tp_dict;
+
+    class->set_property = pyg_object_set_property;
+    class->get_property = pyg_object_get_property;
+
+    /* install signals */
+    /* we look this up in the instance dictionary, so we don't
+     * accidentally get a parent type's __gsignals__ attribute. */
+    gsignals = PyDict_GetItemString(class_dict, "__gsignals__");
+    if (gsignals) {
+	if (!PyDict_Check(gsignals)) {
+	    PyErr_SetString(PyExc_TypeError,
+			    "__gsignals__ attribute not a dict!");
+	    return;
+	}
+	if (!(overridden_signals = add_signals(class, gsignals))) {
+	    return;
+	}
+        if (PyDict_SetItemString(class_dict, "__gsignals__",
+				 overridden_signals)) {
+            return;
+        }
+        Py_DECREF(overridden_signals);
+
+        PyDict_DelItemString(class_dict, "__gsignals__");
+    } else {
+	PyErr_Clear();
+    }
+
+    /* install properties */
+    /* we look this up in the instance dictionary, so we don't
+     * accidentally get a parent type's __gproperties__ attribute. */
+    gproperties = PyDict_GetItemString(class_dict, "__gproperties__");
+    if (gproperties) {
+	if (!PyDict_Check(gproperties)) {
+	    PyErr_SetString(PyExc_TypeError,
+			    "__gproperties__ attribute not a dict!");
+	    return;
+	}
+	if (!add_properties(class, gproperties)) {
+	    return;
+	}
+	PyDict_DelItemString(class_dict, "__gproperties__");
+	/* Borrowed reference. Py_DECREF(gproperties); */
+    } else {
+	PyErr_Clear();
+    }
+}
+
+static void
 pyg_register_class_init(GType gtype, PyGClassInitFunc class_init)
 {
     GSList *list;
@@ -1011,7 +1052,7 @@ pygobject__g_instance_init(GTypeInstance   *instance,
  */
 static void
 pyg_type_add_interfaces(PyTypeObject *class, GType instance_type,
-                        PyObject *bases, gboolean new_interfaces,
+                        PyObject *bases,
                         GType *parent_interfaces, guint n_parent_interfaces)
 {
     int i;
@@ -1025,7 +1066,6 @@ pyg_type_add_interfaces(PyTypeObject *class, GType instance_type,
         guint k;
         PyObject *base = PyTuple_GET_ITEM(bases, i);
         GType itype;
-        gboolean is_new = TRUE;
         const GInterfaceInfo *iinfo;
         GInterfaceInfo iinfo_copy;
 
@@ -1042,16 +1082,6 @@ pyg_type_add_interfaces(PyTypeObject *class, GType instance_type,
         if (!G_TYPE_IS_INTERFACE(itype))
             continue;
 
-        for (k = 0; k < n_parent_interfaces; ++k) {
-            if (parent_interfaces[k] == itype) {
-                is_new = FALSE;
-                break;
-            }
-        }
-
-        if ((new_interfaces && !is_new) || (!new_interfaces && is_new))
-            continue;
-
         iinfo = pyg_lookup_interface_info(itype);
         if (!iinfo) {
             gchar *error;
@@ -1072,7 +1102,7 @@ pyg_type_add_interfaces(PyTypeObject *class, GType instance_type,
 int
 pyg_type_register(PyTypeObject *class, const char *type_name)
 {
-    PyObject *gtype, *gsignals, *gproperties, *overridden_signals;
+    PyObject *gtype;
     GType parent_type, instance_type;
     GType *parent_interfaces;
     guint n_parent_interfaces;
@@ -1150,88 +1180,22 @@ pyg_type_register(PyTypeObject *class, const char *type_name)
     }
 
     /*
-     * Note: Interfaces to be implemented are searched twice.  First
-     * we register interfaces that are already implemented by a parent
-     * type.  The second time, the remaining interfaces are
-     * registered, i.e. the ones that are not implemented by a parent
-     * type.  In between these two loops, properties and signals are
-     * registered.  It has to be done this way, in two steps,
-     * otherwise glib will complain.  If registering all interfaces
-     * always before properties, you get an error like:
-     *
-     *    ../gobject:121: Warning: Object class
-     *    test_interface+MyObject doesn't implement property
-     *    'some-property' from interface 'TestInterface'
-     *
-     * If, on the other hand, you register interfaces after
-     * registering the properties, you get something like:
-     *
-     *     ../gobject:121: Warning: cannot add interface type
-     *    `TestInterface' to type `test_interface+MyUnknown', since
-     *    type `test_interface+MyUnknown' already conforms to
-     *    interface
-     *
-     * This looks like a GLib quirk, but no bug has been filed
-     * upstream.  However we have a unit test for this particular
-     * problem, which can be found in test_interfaces.py, class
-     * TestInterfaceImpl.
+     * Note, all interfaces need to be registered before the first
+     * g_type_class_ref(), see bug #686149.
      *
      * See also comment above pyg_type_add_interfaces().
      */
-    pyg_type_add_interfaces(class, instance_type, class->tp_bases, FALSE,
+    pyg_type_add_interfaces(class, instance_type, class->tp_bases,
                             parent_interfaces, n_parent_interfaces);
 
-    /* we look this up in the instance dictionary, so we don't
-     * accidentally get a parent type's __gsignals__ attribute. */
-    gsignals = PyDict_GetItemString(class->tp_dict, "__gsignals__");
-    if (gsignals) {
-	if (!PyDict_Check(gsignals)) {
-	    PyErr_SetString(PyExc_TypeError,
-			    "__gsignals__ attribute not a dict!");
-            g_free(parent_interfaces);
-	    return -1;
-	}
-	if (!(overridden_signals = add_signals(instance_type, gsignals))) {
-            g_free(parent_interfaces);
-	    return -1;
-	}
-        if (PyDict_SetItemString(class->tp_dict, "__gsignals__",
-				 overridden_signals)) {
-            g_free(parent_interfaces);
-            return -1;
-        }
-        Py_DECREF(overridden_signals);
-    } else {
-	PyErr_Clear();
-    }
 
-    /* we look this up in the instance dictionary, so we don't
-     * accidentally get a parent type's __gsignals__ attribute. */
-    gproperties = PyDict_GetItemString(class->tp_dict, "__gproperties__");
-    if (gproperties) {
-	if (!PyDict_Check(gproperties)) {
-	    PyErr_SetString(PyExc_TypeError,
-			    "__gproperties__ attribute not a dict!");
-            g_free(parent_interfaces);
-	    return -1;
-	}
-	if (!add_properties(instance_type, gproperties)) {
-            g_free(parent_interfaces);
-	    return -1;
-	}
-	PyDict_DelItemString(class->tp_dict, "__gproperties__");
-	/* Borrowed reference. Py_DECREF(gproperties); */
-    } else {
-	PyErr_Clear();
+    gclass = g_type_class_ref(instance_type);
+    if (PyErr_Occurred() != NULL) {
+        g_type_class_unref(gclass);
+        g_free(parent_interfaces);
+        return -1;
     }
 
-    /* Register new interfaces, that are _not_ already defined by
-     * the parent type.  FIXME: See above.
-     */
-    pyg_type_add_interfaces(class, instance_type, class->tp_bases, TRUE,
-                            parent_interfaces, n_parent_interfaces);
-
-    gclass = g_type_class_ref(instance_type);
     if (pyg_run_class_init(instance_type, gclass, class)) {
         g_type_class_unref(gclass);
         g_free(parent_interfaces);
@@ -1240,9 +1204,8 @@ pyg_type_register(PyTypeObject *class, const char *type_name)
     g_type_class_unref(gclass);
     g_free(parent_interfaces);
 
-    if (gsignals)
-        PyDict_DelItemString(class->tp_dict, "__gsignals__");
-
+    if (PyErr_Occurred() != NULL)
+        return -1;
     return 0;
 }
 
diff --git a/tests/test_signal.py b/tests/test_signal.py
index e24f97f..ec58336 100644
--- a/tests/test_signal.py
+++ b/tests/test_signal.py
@@ -4,7 +4,7 @@ import gc
 import unittest
 import sys
 
-from gi.repository import GObject
+from gi.repository import GObject, GLib
 from gi._gobject import signalhelper
 import testhelper
 from compathelper import _long
@@ -74,7 +74,11 @@ class TestGSignalsError(unittest.TestCase):
         def foo():
             class Foo(GObject.GObject):
                 __gsignals__ = {'not-exists': 'override'}
+        # do not stumble over the warning thrown by GLib
+        old_mask = GLib.log_set_always_fatal(GLib.LogLevelFlags.LEVEL_CRITICAL |
+                                             GLib.LogLevelFlags.LEVEL_ERROR)
         self.assertRaises(TypeError, foo)
+        GLib.log_set_always_fatal(old_mask)
         gc.collect()