Re: [Epiphany] mega-patch

[Marco Pesenti Gritti <marco it gnome org>]

On Fri, 2003-02-14 at 05:42, James Willcox wrote:
> Hi Marco,
> 
> Here's a patch that does 2 things:  adds load feedback to tabs, and adds
> a context menu to the bookmark editor. 
> http://www.cs.indiana.edu/~jwillcox/epiphany-loading-and-menu.png
> 
> It really should be 2 separate patches, but I am lame and didn't want to
> separate it.  Of course, if you want one and not the other I will go
> ahead and do it. :)
> 
> Thanks,
> James
> ----
> 

Cool :) Review follow.

>  	epiphany-viewsource.png \
> -	epiphany-send-link.png	
> +	epiphany-send-link.png	\
> +	epiphany-tab-loading.gif

I dont like gifs but until we have mng support I guess there is no
choice :(

>  			       GtkWidget *child,
>  			       EphyNotebookPageLoadStatus status)
>  {
> +	GtkWidget *tab, *label, *image;
> +
> +	g_return_if_fail (nb != NULL);
> +
> +	if (status == nb->priv->current_status)
> +		return;
> +
> +	tab = gtk_notebook_get_tab_label (GTK_NOTEBOOK (nb), child);
> +
> +	g_return_if_fail (tab != NULL);
> +
> +	label  = g_object_get_data (G_OBJECT (tab), "label");
> +	image  = g_object_get_data (G_OBJECT (tab), "loading-image");
> +
> +	g_return_if_fail (label != NULL);
> +	g_return_if_fail (image != NULL);

Am I missing something or are you unnecessarily getting the label here ?
It doesnt appear to be used.

> +	/* setup load feedback image */
> +	loading_pixbuf = gdk_pixbuf_animation_new_from_file (ephy_file ("epiphany-tab-loading.gif"), NULL);
> +	loading_image = gtk_image_new_from_animation (loading_pixbuf);
> +	g_object_unref (loading_pixbuf);
> +	gtk_box_pack_start (GTK_BOX (hbox), loading_image, FALSE, FALSE, 0);
> +

I'd prefer if we register this as a stock icon. Have a look at
lib/ephy-stock-icons.c. So it's themable ...

> +
> +	if (selection)
> +		g_list_free (selection);
> +}

Silly nitpick. Can you use {} for the if ? Some style rule we have ...
(this code occur 2 times in your patch).

>  
> +GList		    *ephy_bookmarks_editor_get_selected_bookmarks (EphyBookmarksEditor *editor);
> +
> +void		     ephy_bookmarks_editor_remove_selected (EphyBookmarksEditor *editor);
> +
>  G_END_DECLS

I'd prefer to not have these public, can you please put the callbacks
inside the bookmarks editor code and keep these static ?

Thanks a lot. I love this patch :)

Marco