Re: [patch] emblem sidebar

[Dave Camp <dave ximian com>]

On Fri, 2002-09-27 at 21:33, James Willcox wrote:
> > (it's a little inaccurate...the "erase" icon is actually at the end)

Maybe it should be at the front?  The "Reset" tiles in the background
dialog are at the front, and this is basically the same idea.

> I've attached a new patch that implements the sidebar as a bonobo
> component (so it can now be disabled, etc.).

Looks good. 

It would be nice if it grabbed from the icon theme - no sense checking
in code that is already obsolete.

Some nit-picky comments:

>	if (string_to_strip == NULL)
>		return NULL;

We require braces around all blocks, even one-liners.

>	GtkWidget *emblems_table, *image, *scroller;
>	GtkWidget *erase_widget = NULL;
>	char *emblem_name;

The coding style requires that variables not be initialized in the
declaration.

>	while (nautilus_customization_data_get_next_element_for_display
(customization_data, &emblem_name, &pixbuf, &label) == GNOME_VFS_OK) {	
>		GdkPixbuf *prelight_pixbuf;
>		gboolean is_erase_widget = FALSE;
>		gchar *keyword;

The coding style requires that variables be declared at the top of
functions, and we use 'char' and 'int' rather than 'gchar' and 'gint'.

>	if (emblem_view->details) {
>		g_free (emblem_view->details);
>		emblem_view->details = NULL;
>	}

The details struct is usually freed in finalize, which doesn't need
multiple-call protection.

Most of these rules are mentioned in nautilus/docs/style-guide.html.

Thanks,
-dave

-- 
Dave Camp <dave ximian com>