Re: [GnomeMeeting-devel-list] [PATCH] druid reorganisation

[PUYDT Julien <julien puydt laposte net>]

On ven, 2004-05-07 at 15:41, Damien Sandras wrote:
> OK after reading it, it only moves a bit of code around here and there,
> but there are 2 things that must absolutely change :

As the subject says: it is a reorganisation, not a full rewrite ;-)

> - your naming of the functions is worse than what it was. 

Well...

> If you create the druid with gm_druid_new, then all your callbacks and
> functions related to the gm_druid should be prefixed by gm_druid.

I can change that with a lot of C-y...

> - your callbacks are internal callbacks for the gm_druid object, so
> that's perfectly rational to pass the internal structure as parameter.
> In the case where your callbacks would be shared by several parts of the
> code, it is better to keep the pointer parameter for other uses to pass
> real parameters and not constant ones. However, I would prefix the
> callbacks by gm_druid.

All of them are internal, and can hardly get the internal structure from
a g_object_get_data... and since the gpointer would be unused, passing
the internal structure through it seems reasonable.

> - You have split a callback in many pieces, the advantage is more than
> discussable, but if you like it, that's ok as long as I don't dislike
> it. But don't forget documentation !!

Well, check the original callback: it chooses its execution path from a
GPOINTER_TO_INT (data) using "if else if else if ..." with no code
shared between all paths! (and the GPOINTER_TO_INT was compared to
explicit values, not nice named enum!)

The advantages are clear:
* it is more readable;
* it is more understandable;
* adding a page won't break it.

> - Your static functions should take the GtkWidget *druid as argument,
> not the private data, that's a matter of consistency. Perhaps we should
> add in misc.cpp a function named gm_get_object_from_widget.

True. I'll change that.

Snark