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



> 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.
> 

The rules on the wiki say to pass the GtkWidget corresponding to the
gm_druid in that case and not the internal structure. What do you think?
Basically none is better than the other, but we have a choice to make. I
wouldn't use internal data as much as possible as parameters.

> > - 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!)
> 

Who said that to have a switch statement you should have shared code
between the paths? I still prefer the old way of doing things... But I
won't spend hours trying to convince you because it is not worth it.
Generally my advice is that such code changes are unuseful.

Personally I think having 1 function handling the "prepare" signal and
taking the page number as argument is better than having 20 functions if
you have 20 pages. Having 20 functions reacting to the same signal makes
the code harder to read than having one unique function with a switch or
a correctly structured if.

The only reason why you have changed that is because you wanted to pass
the GmDruid * as gpointer. That is *very* arguable. I don't care much
about the druid as they will change the API and put it at the GTK level,
but I wouldn't accept it for any other part of the code ;)

> The advantages are clear:
> * it is more readable;
> * it is more understandable;

I have to disagree here. An explosion of functions makes C code less
readable than having one unique function for a given task.

> * adding a page won't break it.
> 

Same with an if.

> > - 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
> 
> _______________________________________________
> Gnomemeeting-devel-list mailing list
> Gnomemeeting-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/gnomemeeting-devel-list

Attachment: signature.asc
Description: Ceci est une partie de message =?ISO-8859-1?Q?num=E9riquement?= =?ISO-8859-1?Q?_sign=E9e?=



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]