[Glade-devel] small patch

[pborelli katamail com (Paolo Borelli)]

On Tue, 2004-01-13 at 14:10, Sridhar R wrote:
> Hi,

Hi Sridhar,
        I've not tried the patch (yet), but if I understand it correctly the
result is that, e.g. for a GtkButton, the GtkButton signals list node in
the tree is expanded.
Did I get it right? If yes, I like it :)
[if someone else doesn't feel free to voice your opinion]

Some comments (nitpicks mostly) on the patch follow:

1) it seems to me that your mailer ate the patch a bit (it breaks some
lines). If you use Evolution you can send the patch as an attachment and
it will be sent in plain text, don't know if other mail apps do the
same...

> I'm all new to collabarative developments.  Here is my
> (initial) small patch to CVS glade-signal-editor.c,
> regarding usability.  That is, it will now expand the
> first row of signal-list dialog, which is what any
> user would do more often.
>
> Here goes the diff (diff --context=2 . .)

please use diff -pu (the p option tells in which function the changes
are made, making the patch more readable. The default context (3 iirc is
just fine).
Beside also provide a ChangeLog entry.

> *** ../../glade3.orig/src/glade-signal-editor.c Mon
> Jan 12 21:38:09 2004
> --- glade-signal-editor.c       Mon Jan 12 22:04:06
> 2004
> ***************
> *** 158,161 ****
> --- 158,162 ----
>         GtkTreeIter *parent = NULL;
>         GList *list = NULL;
> +       GtkTreePath *path_first = NULL;
>         GladeWidgetClassSignal *signal;
>     

No need to initialize local vars to NULL as far as I can see, while you
are at it remove it also from the other vars; beside, not related to
your code, but usually a TreeIter is allocated on the stack: i.e.
GtkTreeIter iter;
gtk_tree_functio (..., &iter, ...);

> ***************
> *** 174,177 ****
> --- 175,182 ----
>                
> glade_signal_editor_dialog_append_signal (lst_model,
> signal->nam
> e, parent);
>         }
> +       /* Sridhar R: Expand the first row */
> +       path_first = gtk_tree_path_new_first();
> +       gtk_tree_view_expand_row(GTK_TREE_VIEW (view),
> path_first, FALSE);
> +       gtk_tree_path_free(path_first);
>   }
>   

We usually don't add comments like "paolo: did this" for each patch
because they would fastly clutter the code. In this particular case the
whole comment seems superflous, since it's just stating what the 3 line
under it do.

> ***************
> *** 183,186 ****
> --- 188,193 ----
>         gint response;
>   
> +       g_assert(editor);
> +       g_assert(editor->class);
>         g_return_if_fail (editor->class->signals !=
> NULL);
>   

Use g_return_if_fail instead of assert to check function args.

> ***************
> *** 663,665 ****
>         }
>   }
> - 
> --- 670,671 ----
> I have also attached the diff file.
>
> Well, what are all the stuffs that I can start working on?

If you want to continue with ui stuff on the signal editor there is a
detailed bug in bugzilla.gnome.org, suggesting some changes that can be
made. I don't have the bug number at hand, but since at the moment the
open bugs against glade3 are just 2, you should find it pretty easily :)

Thanks for your work, I'm sure that once you get started things will go
more smoothly!

ciao
        paolo