Created attachment 63512 [details] [review]
Use GOptionContext and GOptionGroup

Created attachment 63513 [details]
Wrapper for GOptionContext

Needed by the pygoption.diff patch.

Created attachment 63514 [details]
Wrapper for GOptionGroup

Needs pygoption.diff

Created attachment 63515 [details]
Example for GOptionWrapper and GOptionGroup

A tiny example how to use the GOptionGroup and GOptionContext wrapper.

These files add GOptionContext and GOptionGroup wrappers to pygobject. 

The GOptionEntry is mapped to an tuple, which contains instead of the variable pointer either an name or a function for the callback case. An option value can accessed by optiongroup[name].

The GOptionGroup can only accessed when the GOptionGroup was created via the python wrapper, otherwise the userdata and destroy callbacks are not set by the python wrapper.

Johannes, can you submit it as one big patch? (hint, use cvs add and diff -uN)

Created attachment 63517 [details]
Wrapper for GOptionGroup

Needs pygoption.diff

This is the second version, the PyGOptionGroup was holdding two references from the beginnig. Now the refcount only is increased when the PyGOptionGroup is added to the GOptionContext.

Created attachment 63519 [details] [review]
Wraps GOptionGroup and GOptionContext

Sorry, cvs add don't work (i dont have write access to cvs).

This patch should work with 
cd gnome-python
patch -p1 < pygobject-option.diff.

Created attachment 63697 [details] [review]
Wraps GOptionGroup and GOptionContext

Added test and example.

Added optparse interface (gobject.option) and simplified the c part of the wrapper.

Created attachment 63699 [details] [review]
Wraps GOptionGroup and GOptionContext

Adds tests and an example.

Simplifies the C part of the wrapper and adds a optparse interface (gobject.option).

(Sorry the last patch was reversed)

Comment on attachment 63699 [details] [review]
Wraps GOptionGroup and GOptionContext

Much better!
Less code, more done in python.

However, I found a couple of things that still needs to be improved:

>+++ pygobject/gobject/pygobject-private.h	2006-04-15 17:11:42.000000000 +0200

>+typedef struct {
>+    PyObject_HEAD
>+    GOptionGroup *group;
>+    gboolean other_owner, is_in_context;

>+    PyObject *callback,
>+             *pre_hook, *post_hook, *hooks_arg,
>+             *error_hook, *error_hook_arg,
>+             *translate_func, *translate_func_arg;

Don't store this here, see below.

>+++ pygobject/gobject/pygoptioncontext.c	2006-04-15 21:36:07.000000000 +0200

>+static void
>+pyg_option_context_dealloc(PyGOptionContext *self)
>+{
>+    Py_CLEAR(self->main_group);
>+    
>+    if(self->context != NULL)
        ^
Add a space here, same for for, while etc.

>+static PyObject *
>+pyg_option_context_parse(PyGOptionContext *self, PyObject *argv)
>+{
>+    PyObject *arg;
>+    PyObject *new_argv;
>+    gssize argv_length, pos;
>+    char **argv_content, **original;
>+    GError *error = NULL;
>+    gboolean result;
>+    
>+    argv_length = PySequence_Size(argv);
>+    if(argv_length == -1)
>+    {
>+        PyErr_SetString(PyExc_TypeError, 
>+                        "GOptionContext.parse expects a list of strings.");
>+        return NULL;
>+    }

This is inconsistent. It's reasonable to accept only a list and nothing else.
Follow the exception, so its point in using PySequence_*.
Don't to forget that it's actually a list (PyList_Check)

>+    argv_content = g_new(char*, PySequence_Size(argv) + 1);

reuse argv_length.

>+    original = g_memdup(argv_content, sizeof(char*) * (PySequence_Size(argv) + 1));

g_strdupv, reuse argv_length again.

>+    if(! result)
>+    {

if (!result)

>+    new_argv = PyList_New(0);
>+    for(pos = 0; pos < argv_length; pos++)
>+    {
>+        arg = PyString_FromString(argv_content[pos]);
>+        PyList_Append(new_argv, arg);
>+    }

You know the length, it's more effiecent to create a list of fixed size and
use PyList_SetItem afterwards. Be careful with the reference counting though.

>+static PyObject *
>+pyg_option_context_set_help_enabled(PyGOptionContext *self,
>+                                    PyObject *enabled)
>+{
>+    g_option_context_set_help_enabled(self->context, PyObject_IsTrue(enabled));
>+    Py_RETURN_NONE;

Is this not new in 2.4?

>+++ pygobject/gobject/pygoptiongroup.c	2006-04-15 22:09:44.000000000 +0200

>+static gboolean
>+call_parse_func(PyGOptionGroup *self,
>+                GOptionContext *context,
>+                PyObject *func,
>+                PyObject *arg,
>+                GError **error)
>+{
>+    PyObject *obj;
>+    PyGILState_STATE state;
>+   
>+    if (func == NULL) return TRUE;

This looks sort of strange, will it ever happen?

>+static int
>+pyg_option_group_init(PyGOptionGroup *self, PyObject *args)
>+{
>+    char *name, *description, *help_description;
>+    PyObject *callback;

Perhaps accept keywords too?

>+    if (! PyArg_ParseTuple(args, "zzzO:GOptionGroup.__init__", 
>+                           &name, &description, &help_description, &callback))
>+        return -1;

if (!PyArg...)

>+static gboolean 
>+arg_func(const gchar *option_name,

>+    if(value != NULL) 
>+        obj = PyObject_CallFunction(self->callback, "ssO", option_name, value, self);
>+    else
>+        obj = PyObject_CallFunction(self->callback, "sOO", option_name, Py_None, self);

Why not an empty string?

>+static PyObject *
>+pyg_option_group_add_entries(PyGOptionGroup *self, PyObject *sequence)
>+{

>+    entry_count = PySequence_Size(sequence);
>+    if(entry_count == -1)
>+    {
>+        PyErr_SetString(PyExc_TypeError, "GOptionGroup.add_entries expected a list of entries");
>+        return FALSE;
>+    }

Inconsistency again.

>+    entries = g_new0(GOptionEntry, entry_count + 1);
>+    for(pos = 0; pos < entry_count; pos++)
>+    {
>+        tuple = PySequence_GetItem(sequence, pos);

Perhaps check so it is a tuple first?

>+static PyObject *
>+pyg_option_group_set_translate_func(PyGOptionGroup *self, PyObject *args)
>+{

>+    Py_CLEAR(self->translate_func_arg);
>+    self->translate_func_arg = arg;
>+    Py_XINCREF(self->translate_func_arg);

This is not threadsafe. You should pass in this as an argument.
See how it's done in the rest of pygobject and pygtk.

>+GOptionGroup *
>+pyg_option_group_transfer_group(PyGOptionGroup *self)

Is this really required, is there no way around it?

>+++ pygobject/examples/option.py	2006-04-15 13:52:56.000000000 +0200
>@@ -0,0 +1,37 @@
>+#!/usr/bin/env python
>+# gnome-python/pygobject/examples/option.py
>+
>+from gobject.goption import *

Don't encourage *-imports.

Created attachment 64333 [details] [review]
Wraps GOptionGroup and GOptionContext

Sorry for the long delay. Here is the next version of the GOption(Context|Group)-Wrapper. 

(In reply to comment #11)
> (From update of attachment 63699 [details] [review] [edit])
> Much better!
> Less code, more done in python.
> 
> However, I found a couple of things that still needs to be improved:

Here only the parts of the review i commented, every thing else are part of the new patch
 
> >+++ pygobject/gobject/pygobject-private.h	2006-04-15 17:11:42.000000000 +0200
> 
> >+typedef struct {
> >+    PyObject_HEAD
> >+    GOptionGroup *group;
> >+    gboolean other_owner, is_in_context;
> 
> >+    PyObject *callback,
> >+             *pre_hook, *post_hook, *hooks_arg,
> >+             *error_hook, *error_hook_arg,
> >+             *translate_func, *translate_func_arg;
> 
> Don't store this here, see below.

Changed all this into tuples containing a callable and an argument.
 
 
> >+static PyObject *
> >+pyg_option_context_set_help_enabled(PyGOptionContext *self,
> >+                                    PyObject *enabled)
> >+{
> >+    g_option_context_set_help_enabled(self->context, PyObject_IsTrue(enabled));
> >+    Py_RETURN_NONE;
> 
> Is this not new in 2.4?

Yes, I changed all ocurrences of Py_RETURN_NONE to Py_INCREF(Py_None); return Py_None; . Or is it okay to depend on 2.4 ?

> >+++ pygobject/gobject/pygoptiongroup.c	2006-04-15 22:09:44.000000000 +0200
> 
> >+static gboolean
> >+call_parse_func(PyGOptionGroup *self,
> >+                GOptionContext *context,
> >+                PyObject *func,
> >+                PyObject *arg,
> >+                GError **error)
> >+{
> >+    PyObject *obj;
> >+    PyGILState_STATE state;
> >+   
> >+    if (func == NULL) return TRUE;
> 
> This looks sort of strange, will it ever happen?

In the old version this could happen, since then i changed the behaviour.
 
> >+static gboolean 
> >+arg_func(const gchar *option_name,
> 
> >+    if(value != NULL) 
> >+        obj = PyObject_CallFunction(self->callback, "ssO", option_name, value, self);
> >+    else
> >+        obj = PyObject_CallFunction(self->callback, "sOO", option_name, Py_None, self);
> 
> Why not an empty string?

There is the flag G_OPTION_FLAG_OPTIONAL_ARG, so the option_value can either be "" (a empty string was passed as argument) or NULL, no argument was supplied.
 
> >+static PyObject *
> >+pyg_option_group_set_translate_func(PyGOptionGroup *self, PyObject *args)
> >+{
> 
> >+    Py_CLEAR(self->translate_func_arg);
> >+    self->translate_func_arg = arg;
> >+    Py_XINCREF(self->translate_func_arg);
> 
> This is not threadsafe. You should pass in this as an argument.
> See how it's done in the rest of pygobject and pygtk.

Checked and changed all such assignments to if (x) { Py_INCREF(x); self->x = x; }.

> >+GOptionGroup *
> >+pyg_option_group_transfer_group(PyGOptionGroup *self)
> 
> Is this really required, is there no way around it?

I think it is required. The GOptionContext frees every GOptionGroup it has (added with g_option_context_add_group, _set_main_group) when the context is freed. So the owner of the GOptionGroup is now the GOptionContext, and the Python-Object only holds something like a weak-reference. But the callback functions have a pointer to PyGOptionGroup, so PyGOptionGroup shouldn't destroied before the GOptionGroup. So i INCREF the OptionGroup on transfer of the ownership of GOptionGroup from PyGOptionGroup to GOptionContext, and decref it in destroy_g_group(). Which i thought was a good idea to capsulate into _transfer_group().

In addition to this also some bugs are fixed. Most notably, There is now a GSList
of strings which are added with the entries, since these are not copied by the
g_option_group_add_entires function.

> > >+    PyObject *callback,
> > >+             *pre_hook, *post_hook, *hooks_arg,
> > >+             *error_hook, *error_hook_arg,
> > >+             *translate_func, *translate_func_arg;
> > 
> > Don't store this here, see below.
> 
> Changed all this into tuples containing a callable and an argument.

This is still not done properly, what you need to do is this, pseudo
python/C code:

error_marshal(carg1, ..., data):
  func, arg = data
  pyarg = ...(carg1)
  PyObject_CallObject(func, "(...)", pyarg1, ...)
  ...

_wrap_error(self, args, kwargs):
  ....
  data = Py_BuildValue("(..)", pyfunc, pyargs)
  error(..., error_marshal, data)
  ...

The function and the argument are sent as user data to the marshaller, you're not saving any state inside the object itself, this will completely avoid
concurrency issues.
Look at any marshaller in pygobject or pygtk, all of them are implemented this way.

Created attachment 64488 [details] [review]
Wraps GOptionGroup and GOptionContext (without the pre, post, error and translation hooks)

The same patch, but without the pre, post, error hook.

For the argument callback shouldn't be a race condition, because the callback (and  the destroy notify) get the PyGOptionGroup as user data, and the PyGOptionGroup lives always longer than the GOptionGroup.

Also this patch has a small bugfix, the string list is now correctly freed and the PyGOptionGroup is not leaked any more.

Comment on attachment 64488 [details] [review]
Wraps GOptionGroup and GOptionContext (without the pre, post, error and translation hooks)

In general this looks good.
I'm not so familiar with GOption or OptParse that I can make a really good review. But I think it can go into CVS. Do you have an account Johannes?

(In reply to comment #15)
> (From update of attachment 64488 [details] [review] [edit])
> In general this looks good.
> I'm not so familiar with GOption or OptParse that I can make a really good
> review. But I think it can go into CVS. Do you have an account Johannes?

No, sorry.

Okay, I'll commit it when I can find a little bit of time.

Checking in ChangeLog;
/cvs/gnome/gnome-python/pygobject/ChangeLog,v  <--  ChangeLog
new revision: 1.37; previous revision: 1.36
done
Checking in examples/Makefile.am;
/cvs/gnome/gnome-python/pygobject/examples/Makefile.am,v  <--  Makefile.am
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvs/gnome/gnome-python/pygobject/examples/option.py,v
done
Checking in examples/option.py;
/cvs/gnome/gnome-python/pygobject/examples/option.py,v  <--  option.py
initial revision: 1.1
done
Checking in gobject/Makefile.am;
/cvs/gnome/gnome-python/pygobject/gobject/Makefile.am,v  <--  Makefile.am
new revision: 1.9; previous revision: 1.8
done
Checking in gobject/gobjectmodule.c;
/cvs/gnome/gnome-python/pygobject/gobject/gobjectmodule.c,v  <--  gobjectmodule.c
new revision: 1.216; previous revision: 1.215
done
RCS file: /cvs/gnome/gnome-python/pygobject/gobject/option.py,v
done
Checking in gobject/option.py;
/cvs/gnome/gnome-python/pygobject/gobject/option.py,v  <--  option.py
initial revision: 1.1
done
Checking in gobject/pygobject-private.h;
/cvs/gnome/gnome-python/pygobject/gobject/pygobject-private.h,v  <--  pygobject-private.h
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvs/gnome/gnome-python/pygobject/gobject/pygoptioncontext.c,v
done
Checking in gobject/pygoptioncontext.c;
/cvs/gnome/gnome-python/pygobject/gobject/pygoptioncontext.c,v  <--  pygoptioncontext.c
initial revision: 1.1
done
RCS file: /cvs/gnome/gnome-python/pygobject/gobject/pygoptiongroup.c,v
done
Checking in gobject/pygoptiongroup.c;
/cvs/gnome/gnome-python/pygobject/gobject/pygoptiongroup.c,v  <--  pygoptiongroup.c
initial revision: 1.1
done
RCS file: /cvs/gnome/gnome-python/pygobject/tests/test_option.py,v
done
Checking in tests/test_option.py;
/cvs/gnome/gnome-python/pygobject/tests/test_option.py,v  <--  test_option.py
initial revision: 1.1
done

This stuff has memory problems.  Running "make check"

..................*** glibc detected *** free(): invalid next size (fast): 0x08199930 ***

gdb backtrace:

+ Trace 68075

This fixed the problem:

--- gobject/pygoptioncontext.c  29 Apr 2006 18:58:20 -0000      1.1
+++ gobject/pygoptioncontext.c  7 May 2006 15:23:31 -0000
@@ -272,7 +272,7 @@ PyTypeObject PyGOptionContext_Type = {
     PyObject_HEAD_INIT(NULL)
     0,
     "gobject.OptionContext",
-    sizeof(PyGMainContext),
+    sizeof(PyGOptionContext),
     0,
     /* methods */
     (destructor)pyg_option_context_dealloc,

Created attachment 66211 [details] [review]
Adds exception handling to GOption

Adds the function pyg_set_gerror_from_exception() and support exception handling support to GOption.