Using the current GParamSpec wrapper for this would be an interesting
idea.  It would just involve adding tp_descr_set and tp_descr_get
methods to the wrapper.

mass reassign of open pygtk and gnome-python bugs.

*** Bug 105514 has been marked as a duplicate of this bug. ***

What does "using descriptors" mean to the end-user? Will they be
attributes for the instances or not?

kiko: a descriptor is an attribute of a class that can control how it
is get or set in instances of the class.  For instance:
  class MyDescriptor(object):
      '''foo!'''
      def __get__(self, instance, type):
          if not instance: return self
          return instance._foo
      def __set__(self, instance, value):
          instance._foo = value
  class MyClass(object):
      foo = MyDescriptor()

  c = MyClass()
  c.foo = 1
  print c.foo
  print MyClass.foo

The benefits over the old __getattr__ scheme is that descriptors get
inheritted by subclasses automatically (no need to worry about
chaining up __getattr__ implementations or anything like that), and
get listed in help() (in the above example, you would see "foo!" as
the documentation on MyClass's foo attribute).

I have been trying to minimise the cases where a __getattr__ impl is
needed in the 1.99.x series of pygtk.

Created attachment 19788 [details] [review]
Patch implementing this feature

Adding the PATCH keyword and marking Priority=High.

Over to patch author.

Do we want to expose GObject properties as attributes?  I thought pygtk didn't
because people can (& arguably should) use the set_ / get_ methods.  I could go
either way on the issue.

It's certainly less convenient to use set_/get_foo than normal attributes, but:

  - is there a risk of breaking existing applications?
  - is there a risk of conflicts with "normal" attributes?
  - is there a *future* risk of conflicts with "normal" attributes?
  - how hard is it to do this?

Any subclass that defined an attribute with the same name as a property would
have problems, e.g. (this is unchecked pseudo code, so the names may be wrong):

class CustomView(gtk.TreeView):
  def __init__(self):
    gtk.TreeModel.__init__(self)
    self.model = 'string description of the model'

It's valid code now, but would probably throw an exception with the patch. 
Also, there would be problems if a name of a property was the same name as a
method defined in the gtk type, but it may be more of theoretical than a real
problem.

As for implementation, my initial take is that the attached patch should work.

I don't think we want to do this for the upcoming release; the question is
whether this should stay on the list of things to implement eventually.

The example should be:

class CustomView(gtk.TreeView):
  def __init__(self):
    gtk.TreeView.__init__(self)
    self.model = 'string description of the model'

Well. Nothing says that we can't prefix or suffix the comments, so something
like prop_ + name would probably be the way to go.

But we can't just remove the setter/getter.

It's 2.6 material anyway.

Post 2.6 now.

The patch as it is cannot be applied, it will break existing applications.
Some sort of prefix needs to be added. Or a separate property container needs to
be added.

OK.  It seems the current consensus, after discussion in pygtk list, is to
implement a "properties container" object.  Example:
    label.props.text = "foo"
or:
    label.properties.text = "foo"

Iñaki, if you want to implement this (your patch is near), go ahead; you'll get
full credit and be praised forever :-)  Otherwise I'll eventually do it.  Let's
keep in mind the API freeze is at July 11th.

  The name properties/props is not decided, but that's trivial to change later.
 We need a single descriptor on GObject which returns an object containing (I
think using tp_getattro and tp_setattro uses less memory here) the properties
for the particular GObject.

PS: the PATCH keyword in bugzilla is redundant and deprecated.

Created attachment 48453 [details] [review]
Updated patch

Who can resist being praised forever :-)?

The attached patch should do what you suggest. A couple of things that could be
very much improved (it should be a 10 minute cut'n'paste job) is the location
of the code (probably it does not belong logically to the place in pygobject.c
it is in) and the GProps type initialization code, which is quite ugly right
now.

I think the patch needs just a bit of work:

1- I don't think we need to call pygobject_add_props from
pygobject_register_class; just adding the props descriptor once to the base
PyGObject should be enough, then it gets inherited automatically;

2- init_props should go away; just initialize the python type in gobjectmodule.c

3- GIL calls in the descriptor are not needed; since python code calls our
descriptor, we already have the python lock;

4- getattro/setattro must PyString_Check(attr) before proceeding (conceptually,
you could have a unicode string, for example, although the python source code
parser doesn't allow it yet)

5- I also think PyGProps should contain a reference to the GObject, not
PyGObject if we reference the PyGObject, then we'd need to implement cyclic GC
methods (tp_traverse, tp_clear)... [( hm.. thinking again, we might need to
implement those anyway, and in that case reference to PyGObject is simpler )]

I might just fix the patch myself...

Created attachment 48552 [details] [review]
Improved patch, based on Iñaki's

This patch fixes all those things I mentioned, including adding a tp_traverse
function; It also creates a custom descriptor type, instead of using
PyDescr_NewGetSet(), so that we can list properties from classes, like this:
dir(gtk.Label.props).

Created attachment 48559 [details] [review]
unit test

Comment on attachment 48552 [details] [review]
Improved patch, based on Iñaki's

>+build_parameter_list(GType gtype)
[..]
>+    props = g_object_class_list_properties(class, &n_props);
[..]
>+    g_free (props);

Will this work correctly for objects without properties?
Eg, is props NULL or not?

>+PyGProps_getattro(PyGProps *gprops, PyObject *attr)
[..]
>+    if (PyString_Check(attr))

Isn't attr always a string?
Also in setattro

[..]
>+    if (!pspec) {
>+	return PyObject_GenericGetAttr((PyObject *)gprops, attr);
>+    }

There are two identical exit points in this function, can it be refactored to
only have one?
Even this in setattro

>+PyGProps_setattro(PyGProps *gprops, PyObject *attr, PyObject *pvalue)
[..]
>+    if (!gprops->pygobject) {
>+        PyErr_SetString(PyExc_TypeError, "cannot set GOject properties without an instance");
>+        return -1;
>+    }

When will this be true?

>+    "The properties of the GObject accessible as "
>+    "Python attributes.",      /* tp_doc */

Can we improve the doc string and generate a better description on the fly?

>+    gprops = PyObject_GC_New(PyGProps, &PyGProps_Type);
>+    if (obj == NULL || obj == Py_None) {
>+        gprops->pygobject = NULL;
>+        gprops->gtype = pyg_type_from_object(type);
>+    } else {
>+        if (!PyObject_IsInstance(obj, (PyObject *) &PyGObject_Type)) {
>+            PyErr_SetString(PyExc_TypeError, "cannot use GObject property"
>+                            " descriptor on non-GObject instances");

Isn't this redundant?

>+        Py_INCREF(obj);
>+        gprops->pygobject = (PyGObject *) obj;

Do you need the PyObject, why not just save the GObject directly?

>+PyTypeObject PyGPropsDescr_Type = {

I think we need to use NULL for a couple of fields.
I'm pretty sure some compilers will complain otherwise.

>+	PyObject_HEAD_INIT(NULL)
>+	sizeof(PyObject),			/* ob_size */
>+	"gobject.GPropsDescr",			/* tp_name */
>+	0,					/* tp_basicsize */
>+	0,					/* tp_itemsize */
>+	0,	 				/* tp_dealloc */
>+	0,					/* tp_print */
>+	0,					/* tp_getattr */
>+	0,					/* tp_setattr */
>+	0,					/* tp_compare */
>+	0,					/* tp_repr */
>+	0,					/* tp_as_number */
>+	0,					/* tp_as_sequence */

__len__ ?
__contains__?

if name in obj.props: easily wins of hasattr(obj.props, name)

>+	0,		       			/* tp_as_mapping */
>+	0,					/* tp_hash */
>+	0,					/* tp_call */
>+	0,					/* tp_str */

dict like perhaps?

 - Fixed g_free with NULL props

 - in tp_getattro/tp_setattro, attr should normally be a string, but it may not
be a string (eg. could  be a unicode string, eg. setattr(myobj, u"Olá", 123))

 - About the multiple exit points, so what? Sometimes multiple exit points makes
the code clearer, especially when there is only one 'normal' return point and
other 'early exit' points to deal with error conditions

 - gprops->pygobject is NULL when we are trying to access properties in a class
instead of an instance; eg. dir(gtk.Label.props)

 - What to you suggest to improve the doc string of GObject.props?  Generate a
list of properties with names and descriptions?

 - The "PyObject_IsInstance(obj, (PyObject *) &PyGObject_Type)" check is a
precaution against foul play, although I don't really know how one can trigger
this error.  Maybe we can remove this check..

 - > Do you need the PyObject, why not just save the GObject directly?
   Actually keeping the PyGObject is better because it allows a simpler and more
efficient tp_traverse implementation;

 - Regarding NULL vs 0, we are already abusing it in so many places that if it
were really a problem it would have been already reported.

 - Regarding the all other mappings, let's choose one mapping and stick to it! 
"There's only one good way to do it" !!

>  - in tp_getattro/tp_setattro, attr should normally be a string, but it may  
>    not be a string (eg. could  be a unicode string, eg. 
>      setattr(myobj, u"Olá", 123))

PyString_AsString handles that conversion no? Is there not a function which can
do that for us?

>  - What to you suggest to improve the doc string of GObject.props?  Generate a
> list of properties with names and descriptions?

Yes, but I suppose it can wait.

> - > Do you need the PyObject, why not just save the GObject directly?
>   Actually keeping the PyGObject is better because it allows a simpler and 
> more efficient tp_traverse implementation;

Okay, fair enough.

>  - Regarding the all other mappings, let's choose one mapping and stick to it! 
> "There's only one good way to do it" !!

As discussed on IRC, we should have at least an iterator interface, so
  for prop in label.props 
will work, which is a valid usecase :)

Created attachment 48712 [details] [review]
adds iterator, and a couple of other fixes..

Checked in the patch
I modified Gustavos patch in attachment 48712 [details] [review] slightly:
* Cleaned up getattro
* Implemented len()
* Modified getattr(klass, name) to return the paramspec.
* Improved unittests

Checking in ChangeLog;
/cvs/gnome/gnome-python/pygtk/ChangeLog,v  <--  ChangeLog
new revision: 1.1236; previous revision: 1.1235
done
Checking in gobject/gobjectmodule.c;
/cvs/gnome/gnome-python/pygtk/gobject/gobjectmodule.c,v  <--  gobjectmodule.c
new revision: 1.180; previous revision: 1.179
done
Checking in gobject/pygobject-private.h;
/cvs/gnome/gnome-python/pygtk/gobject/pygobject-private.h,v  <-- 
pygobject-private.h
new revision: 1.36; previous revision: 1.35
done
Checking in gobject/pygobject.c;
/cvs/gnome/gnome-python/pygtk/gobject/pygobject.c,v  <--  pygobject.c
new revision: 1.48; previous revision: 1.47
done
Checking in tests/Makefile.am;
/cvs/gnome/gnome-python/pygtk/tests/Makefile.am,v  <--  Makefile.am
new revision: 1.18; previous revision: 1.17
done

*** Bug 325453 has been marked as a duplicate of this bug. ***