Commit 901c1b6e authored by Daniel Drake's avatar Daniel Drake Committed by Martin Pitt
Browse files

Fix property lookup in class hierarchy

Commit 4bfe7972 introduced a bug where
a Python subclass of a gi-provided base class overrides a property from the
base class.

The new behaviour in the above commit causes pygobject to seek the property
in the base class and try to read it from there (resulting in confusion)
rather than noticing that the property is overridden and present in the
Python object instance.

To provide a nicer solution here, we can exploit the fact that
g_object_class_find_property() will traverse the hierarchy in order to
find the right GParamSpec, and the returned GParamSpec can tell us exactly
which GType introduces the property. The strategy is:

 1. Find pspec with g_object_class_find_property()
 2. Find the class that owns that property (pspec->owner_type)
 3. See if girepository owns that class.
 3a. If yes, get property from there.
 3b. If not, get property "directly"

And the same for property setting.

Now that _pygi_lookup_property_from_g_type is always passed the type that
implements the property, it no longer has to go recursing through parent
classes, which was the original cause of confusion.

https://bugzilla.gnome.org/show_bug.cgi?id=686942
parent efcb0f9f
......@@ -51,6 +51,25 @@ GQuark pygobject_wrapper_key;
GQuark pygobject_has_updated_constructor_key;
GQuark pygobject_instance_data_key;
/* Copied from glib. gobject uses hyphens in property names, but in Python
* we can only represent hyphens as underscores. Convert underscores to
* hyphens for glib compatibility. */
static void
canonicalize_key (gchar *key)
{
gchar *p;
for (p = key; *p != 0; p++)
{
gchar c = *p;
if (c != '-' &&
(c < '0' || c > '9') &&
(c < 'A' || c > 'Z') &&
(c < 'a' || c > 'z'))
*p = '-';
}
}
/* -------------- class <-> wrapper manipulation --------------- */
......@@ -237,7 +256,7 @@ build_parameter_list(GObjectClass *class)
static PyObject*
PyGProps_getattro(PyGProps *self, PyObject *attr)
{
char *attr_name;
char *attr_name, *property_name;
GObjectClass *class;
GParamSpec *pspec;
GValue value = { 0, };
......@@ -257,15 +276,13 @@ PyGProps_getattro(PyGProps *self, PyObject *attr)
return ret;
}
if (self->pygobject != NULL) {
ret = pygi_get_property_value (self->pygobject, attr_name);
if (ret != NULL) {
g_type_class_unref(class);
return ret;
}
}
pspec = g_object_class_find_property(class, attr_name);
/* g_object_class_find_property recurses through the class hierarchy,
* so the resulting pspec tells us the owner_type that owns the property
* we're dealing with. */
property_name = g_strdup(attr_name);
canonicalize_key(property_name);
pspec = g_object_class_find_property(class, property_name);
g_free(property_name);
g_type_class_unref(class);
if (!pspec) {
......@@ -278,14 +295,23 @@ PyGProps_getattro(PyGProps *self, PyObject *attr)
return NULL;
}
/* If we're doing it without an instance, return a GParamSpec */
if (!self->pygobject) {
/* If we're doing it without an instance, return a GParamSpec */
return pyg_param_spec_new(pspec);
}
/* See if the property's class is from the gi repository. If so,
* use gi to correctly read the property value. */
ret = pygi_get_property_value (self->pygobject, pspec);
if (ret != NULL) {
return ret;
}
/* If we reach here, it must be a property defined outside of gi.
* Just do a straightforward read. */
g_value_init(&value, G_PARAM_SPEC_VALUE_TYPE(pspec));
pyg_begin_allow_threads;
g_object_get_property(self->pygobject->obj, attr_name, &value);
g_object_get_property(self->pygobject->obj, pspec->name, &value);
pyg_end_allow_threads;
ret = pyg_param_gvalue_as_pyobject(&value, TRUE, pspec);
g_value_unset(&value);
......@@ -295,7 +321,6 @@ PyGProps_getattro(PyGProps *self, PyObject *attr)
static gboolean
set_property_from_pspec(GObject *obj,
char *attr_name,
GParamSpec *pspec,
PyObject *pvalue)
{
......@@ -304,13 +329,13 @@ set_property_from_pspec(GObject *obj,
if (pspec->flags & G_PARAM_CONSTRUCT_ONLY) {
PyErr_Format(PyExc_TypeError,
"property '%s' can only be set in constructor",
attr_name);
pspec->name);
return FALSE;
}
if (!(pspec->flags & G_PARAM_WRITABLE)) {
PyErr_Format(PyExc_TypeError,
"property '%s' is not writable", attr_name);
"property '%s' is not writable", pspec->name);
return FALSE;
}
......@@ -322,7 +347,7 @@ set_property_from_pspec(GObject *obj,
}
pyg_begin_allow_threads;
g_object_set_property(obj, attr_name, &value);
g_object_set_property(obj, pspec->name, &value);
pyg_end_allow_threads;
g_value_unset(&value);
......@@ -336,7 +361,7 @@ static int
PyGProps_setattro(PyGProps *self, PyObject *attr, PyObject *pvalue)
{
GParamSpec *pspec;
char *attr_name;
char *attr_name, *property_name;
GObject *obj;
int ret = -1;
......@@ -358,20 +383,33 @@ PyGProps_setattro(PyGProps *self, PyObject *attr, PyObject *pvalue)
return -1;
}
ret = pygi_set_property_value (self->pygobject, attr_name, pvalue);
obj = self->pygobject->obj;
property_name = g_strdup(attr_name);
canonicalize_key(property_name);
/* g_object_class_find_property recurses through the class hierarchy,
* so the resulting pspec tells us the owner_type that owns the property
* we're dealing with. */
pspec = g_object_class_find_property(G_OBJECT_GET_CLASS(obj),
property_name);
g_free(property_name);
if (!pspec) {
return PyObject_GenericSetAttr((PyObject *)self, attr, pvalue);
}
/* See if the property's class is from the gi repository. If so,
* use gi to correctly read the property value. */
ret = pygi_set_property_value (self->pygobject, pspec, pvalue);
if (ret == 0)
return 0;
else if (ret == -1)
if (PyErr_Occurred())
return -1;
obj = self->pygobject->obj;
pspec = g_object_class_find_property(G_OBJECT_GET_CLASS(obj), attr_name);
if (!pspec) {
return PyObject_GenericSetAttr((PyObject *)self, attr, pvalue);
}
if (!set_property_from_pspec(obj, attr_name, pspec, pvalue))
/* If we reach here, it must be a property defined outside of gi.
* Just do a straightforward set. */
if (!set_property_from_pspec(obj, pspec, pvalue))
return -1;
return 0;
......@@ -1458,7 +1496,7 @@ pygobject_set_property(PyGObject *self, PyObject *args)
return NULL;
}
if (!set_property_from_pspec(self->obj, param_name, pspec, pvalue))
if (!set_property_from_pspec(self->obj, pspec, pvalue))
return NULL;
Py_INCREF(Py_None);
......@@ -1496,7 +1534,7 @@ pygobject_set_properties(PyGObject *self, PyObject *args, PyObject *kwargs)
goto exit;
}
if (!set_property_from_pspec(G_OBJECT(self->obj), key_str, pspec, value))
if (!set_property_from_pspec(G_OBJECT(self->obj), pspec, value))
goto exit;
}
......
......@@ -25,92 +25,95 @@
#include <girepository.h>
/* Copied from glib */
static void
canonicalize_key (gchar *key)
static GIPropertyInfo *
lookup_property_from_object_info (GIObjectInfo *info, const gchar *attr_name)
{
gchar *p;
gssize n_infos;
gssize i;
for (p = key; *p != 0; p++)
{
gchar c = *p;
n_infos = g_object_info_get_n_properties (info);
for (i = 0; i < n_infos; i++) {
GIPropertyInfo *property_info;
if (c != '-' &&
(c < '0' || c > '9') &&
(c < 'A' || c > 'Z') &&
(c < 'a' || c > 'z'))
*p = '-';
property_info = g_object_info_get_property (info, i);
g_assert (info != NULL);
if (strcmp (attr_name, g_base_info_get_name (property_info)) == 0) {
return property_info;
}
g_base_info_unref (property_info);
}
return NULL;
}
static GIPropertyInfo *
_pygi_lookup_property_from_g_type (GType g_type, const gchar *attr_name)
lookup_property_from_interface_info (GIInterfaceInfo *info,
const gchar *attr_name)
{
GIRepository *repository;
GIBaseInfo *info;
gssize n_infos;
gssize i;
GType parent;
repository = g_irepository_get_default();
info = g_irepository_find_by_gtype (repository, g_type);
if (info != NULL) {
n_infos = g_interface_info_get_n_properties (info);
for (i = 0; i < n_infos; i++) {
GIPropertyInfo *property_info;
n_infos = g_object_info_get_n_properties ( (GIObjectInfo *) info);
for (i = 0; i < n_infos; i++) {
GIPropertyInfo *property_info;
property_info = g_interface_info_get_property (info, i);
g_assert (info != NULL);
property_info = g_object_info_get_property ( (GIObjectInfo *) info,
i);
g_assert (info != NULL);
if (strcmp (attr_name, g_base_info_get_name (property_info)) == 0) {
g_base_info_unref (info);
return property_info;
}
g_base_info_unref (property_info);
if (strcmp (attr_name, g_base_info_get_name (property_info)) == 0) {
return property_info;
}
g_base_info_unref (info);
g_base_info_unref (property_info);
}
parent = g_type_parent (g_type);
if (parent > 0)
return _pygi_lookup_property_from_g_type (parent, attr_name);
return NULL;
}
static GIPropertyInfo *
_pygi_lookup_property_from_g_type (GType g_type, const gchar *attr_name)
{
GIPropertyInfo *ret = NULL;
GIRepository *repository;
GIBaseInfo *info;
repository = g_irepository_get_default();
info = g_irepository_find_by_gtype (repository, g_type);
if (info == NULL)
return NULL;
if (GI_IS_OBJECT_INFO (info))
ret = lookup_property_from_object_info ((GIObjectInfo *) info,
attr_name);
else if (GI_IS_INTERFACE_INFO (info))
ret = lookup_property_from_interface_info ((GIInterfaceInfo *) info,
attr_name);
g_base_info_unref (info);
return ret;
}
PyObject *
pygi_get_property_value_real (PyGObject *instance,
const gchar *attr_name)
pygi_get_property_value_real (PyGObject *instance, GParamSpec *pspec)
{
GType g_type;
GIPropertyInfo *property_info = NULL;
char *property_name = g_strdup (attr_name);
GParamSpec *pspec = NULL;
GValue value = { 0, };
GIArgument arg = { 0, };
PyObject *py_value = NULL;
GITypeInfo *type_info = NULL;
GITransfer transfer;
canonicalize_key (property_name);
g_type = pyg_type_from_object ((PyObject *)instance);
property_info = _pygi_lookup_property_from_g_type (g_type, property_name);
/* The owner_type of the pspec gives us the exact type that introduced the
* property, even if it is a parent class of the instance in question. */
property_info = _pygi_lookup_property_from_g_type (pspec->owner_type, pspec->name);
if (property_info == NULL)
goto out;
pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (instance->obj),
attr_name);
if (pspec == NULL)
goto out;
g_value_init (&value, G_PARAM_SPEC_VALUE_TYPE (pspec));
g_object_get_property (instance->obj, attr_name, &value);
g_object_get_property (instance->obj, pspec->name, &value);
type_info = g_property_info_get_type (property_info);
transfer = g_property_info_get_ownership_transfer (property_info);
......@@ -243,7 +246,6 @@ pygi_get_property_value_real (PyGObject *instance,
py_value = _pygi_argument_to_object (&arg, type_info, transfer);
out:
g_free (property_name);
if (property_info != NULL)
g_base_info_unref (property_info);
if (type_info != NULL)
......@@ -254,33 +256,24 @@ out:
gint
pygi_set_property_value_real (PyGObject *instance,
const gchar *attr_name,
GParamSpec *pspec,
PyObject *py_value)
{
GType g_type;
GIPropertyInfo *property_info = NULL;
char *property_name = g_strdup (attr_name);
GITypeInfo *type_info = NULL;
GITypeTag type_tag;
GITransfer transfer;
GValue value = { 0, };
GIArgument arg = { 0, };
GParamSpec *pspec = NULL;
gint ret_value = -1;
canonicalize_key (property_name);
g_type = pyg_type_from_object ((PyObject *)instance);
property_info = _pygi_lookup_property_from_g_type (g_type, property_name);
/* The owner_type of the pspec gives us the exact type that introduced the
* property, even if it is a parent class of the instance in question. */
property_info = _pygi_lookup_property_from_g_type (pspec->owner_type,
pspec->name);
if (property_info == NULL)
goto out;
pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (instance->obj),
attr_name);
if (pspec == NULL)
goto out;
if (! (pspec->flags & G_PARAM_WRITABLE))
goto out;
......@@ -413,12 +406,11 @@ pygi_set_property_value_real (PyGObject *instance,
goto out;
}
g_object_set_property (instance->obj, attr_name, &value);
g_object_set_property (instance->obj, pspec->name, &value);
ret_value = 0;
out:
g_free (property_name);
if (property_info != NULL)
g_base_info_unref (property_info);
if (type_info != NULL)
......
......@@ -30,10 +30,10 @@
#include "pygi.h"
PyObject *pygi_get_property_value_real (PyGObject *instance,
const gchar *attr_name);
GParamSpec *pspec);
gint pygi_set_property_value_real (PyGObject *instance,
const gchar *attr_name,
GParamSpec *pspec,
PyObject *py_value);
#endif /* __PYGI_PROPERTY_H__ */
......@@ -77,9 +77,9 @@ typedef PyObject * (*PyGIArgOverrideReleaseFunc) (GITypeInfo *type_info,
struct PyGI_API {
PyObject* (*type_import_by_g_type) (GType g_type);
PyObject* (*get_property_value) (PyGObject *instance,
const gchar *attr_name);
GParamSpec *pspec);
gint (*set_property_value) (PyGObject *instance,
const gchar *attr_name,
GParamSpec *pspec,
PyObject *value);
GClosure * (*signal_closure_new) (PyGObject *instance,
const gchar *sig_name,
......@@ -124,23 +124,23 @@ pygi_type_import_by_g_type (GType g_type)
static inline PyObject *
pygi_get_property_value (PyGObject *instance,
const gchar *attr_name)
GParamSpec *pspec)
{
if (_pygi_import() < 0) {
return NULL;
}
return PyGI_API->get_property_value(instance, attr_name);
return PyGI_API->get_property_value(instance, pspec);
}
static inline gint
pygi_set_property_value (PyGObject *instance,
const gchar *attr_name,
GParamSpec *pspec,
PyObject *value)
{
if (_pygi_import() < 0) {
return -1;
}
return PyGI_API->set_property_value(instance, attr_name, value);
return PyGI_API->set_property_value(instance, pspec, value);
}
static inline GClosure *
......
......@@ -20,6 +20,7 @@ from gi.repository.GObject import \
from gi.repository import Gio
from gi.repository import GLib
from gi.repository import Regress
from gi.repository import GIMarshallingTests
from gi._gobject import propertyhelper
......@@ -61,6 +62,35 @@ class PropertyObject(GObject.GObject):
type=TYPE_STRV, flags=PARAM_READWRITE | PARAM_CONSTRUCT)
class PropertyInheritanceObject(Regress.TestObj):
# override property from the base class, with a different type
string = GObject.Property(type=int)
# a property entirely defined at the Python level
python_prop = GObject.Property(type=str)
class PropertySubClassObject(PropertyInheritanceObject):
# override property from the base class, with a different type
python_prop = GObject.Property(type=int)
class TestPropertyInheritanceObject(unittest.TestCase):
def test_override_gi_property(self):
self.assertNotEqual(Regress.TestObj.props.string.value_type,
PropertyInheritanceObject.props.string.value_type)
obj = PropertyInheritanceObject()
self.assertEqual(type(obj.props.string), int)
obj.props.string = 4
self.assertEqual(obj.props.string, 4)
def test_override_python_property(self):
obj = PropertySubClassObject()
self.assertEqual(type(obj.props.python_prop), int)
obj.props.python_prop = 5
self.assertEqual(obj.props.python_prop, 5)
class TestPropertyObject(unittest.TestCase):
def test_get_set(self):
obj = PropertyObject()
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment