Hi, thanks for your patch!

I'm not convinced by the current UI, it adds too much new options in the "View" menu and takes too much space in the contact list.
What we could do is to add a "Display protocol" checkbox in the "View" menu so only one item would be added.

A nice way to display the protocol icon could be to display it near the status icon by composing both image. Something like that: http://people.collabora.co.uk/~cassidy/status-protocol.png

*** Bug 595047 has been marked as a duplicate of this bug. ***

Hi Guillaume,

Thanks for the feedback.

The new patch creates a new checkbox under "Offline Contact".

Regarding the icon thing, are there any available? Or should one image overlay the other?

Created attachment 149480 [details] [review]
patch v2

Review of attachment 149480 [details] [review]:

Thanks for your patch.

Could you please rebase it on top of the new master? Also, it would be cool if you could use git format-patch so you'll be credited for the commits. Or just upload your branch to a repo if you prefer.

Your patch introduced some traiing spaces and coding style regressions. Use 'make check' to detect them.

Indeed, you should compose the icon by overlaying the protocol on top of the status icon. You should probably keep them in memory so you don't have to compose the image each time.

::: libempathy-gtk/empathy-contact-list-view.c
@@ +1239,3 @@
 
+
+	

Why did you add blank lines here?

::: libempathy-gtk/empathy-ui-utils.c
@@ +503,3 @@
+
+	account = empathy_contact_get_account (contact);
+	filename = empathy_filename_from_icon_name ( tp_account_get_icon_name( account ), GTK_ICON_SIZE_DIALOG );

should be empathy_filename_from_icon_name (tp_account_get_icon_name (account), GTK_ICON_SIZE_DIALOG);

@@ +511,3 @@
+	return pixbuf;
+}
+

Remove one blank line

Created attachment 149665 [details] [review]
protocol icon overlay

* protocol icon overlay status icon
* icons are cached in a hash table

Review of attachment 149665 [details] [review]:

Thanks for your patch. I inlined my comments.

Also, your patch increased the size of the status icon. Please keep it the same.

::: libempathy-gtk/empathy-contact-list-store.c
@@ +308,3 @@
 						      (GSourceFunc) contact_list_store_inibit_active_cb,
 						      store);
+	priv->status_icons = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref );

Should be:  ..., g_object_unref);

@@ +1671,2 @@
 	gtk_tree_store_set (GTK_TREE_STORE (store), iter,
+					EMPATHY_CONTACT_LIST_STORE_COL_ICON_STATUS, pixbuf_status,

Indentation is wrong.

@@ +1699,3 @@
+	if (show_protocol) {
+		protocol_name = empathy_protocol_name_for_contact (contact);
+		icon_name = g_strdup_printf ("%s-%s-%d", status_icon_name, protocol_name, status_img_size);

Few lines in this function are > 80 chars; please wrap them.

@@ +1703,3 @@
+		icon_name = g_strdup_printf ("%s-%d", status_icon_name, status_img_size);
+	}
+	pixbuf_status = (GdkPixbuf *) g_hash_table_lookup (priv->status_icons, icon_name);

No need to cast, g_hash_table_lookup returns a gpointer.

@@ +1704,3 @@
+	}
+	pixbuf_status = (GdkPixbuf *) g_hash_table_lookup (priv->status_icons, icon_name);
+	if (!pixbuf_status) {

Please use if (ptr == NULL)

@@ +1706,3 @@
+	if (!pixbuf_status) {
+		pixbuf_status = empathy_pixbuf_icon_for_contact_scaled (contact, show_protocol, status_img_size, status_img_size);
+		if (pixbuf_status) {

if (ptr != NULL)

@@ +1721,3 @@
+					EmpathyContact *contact,
+					const gchar *status_icon_name)
+{

Please wrap long lines.

@@ +1734,3 @@
+		show_protocol = TRUE;
+	}
+	status_img_size = priv->is_compact ? 16 : 24 ;

Most of this code has been copied from contact_list_store_get_contact_status_icon; please factor it out.

::: libempathy-gtk/empathy-ui-utils.c
@@ +238,3 @@
+	g_return_val_if_fail (EMPATHY_IS_CONTACT (contact), NULL);
+	account = empathy_contact_get_account (contact);
+	if (!account) {

if (ptr == NULL)

@@ +509,3 @@
+						gboolean       show_protocol,
+					  gint           width,
+					  gint           height)

Identation of args is wrong.

@@ +513,3 @@
+	GdkPixbuf *pix_status;
+	GdkPixbuf *pix_protocol;
+	const gchar     *icon_name;

Don't add space.

@@ +517,3 @@
+	g_return_val_if_fail (EMPATHY_IS_CONTACT (contact), NULL);
+
+	icon_name = empathy_filename_from_icon_name (empathy_icon_name_for_contact (contact), GTK_ICON_SIZE_DIALOG);

This line is too long.
icon_name is leaked.

@@ +530,3 @@
+	}
+	gdk_pixbuf_composite (pix_protocol, pix_status,
+												0, height - height*2/3,

add space around '*'.
Indentation is wrong.

@@ +591,3 @@
+	account = empathy_contact_get_account (contact);
+	filename = empathy_filename_from_icon_name (tp_account_get_icon_name (account), GTK_ICON_SIZE_DIALOG);
+	if ( filename ) {

if (filename) {

::: src/empathy-main-window.c
@@ +801,3 @@
+	empathy_conf_set_bool (empathy_conf_get (),
+							EMPATHY_PREFS_UI_SHOW_PROTOCOLS,
+							 value == TRUE);

Indentation is wrong

@@ +1436,3 @@
+						 EMPATHY_PREFS_UI_SHOW_PROTOCOLS,
+						 window);
+	//gtk_toggle_action_set_active (show_protocol_widget, window->show_protocol);

Why did you comment this?

@@ +1438,3 @@
+	//gtk_toggle_action_set_active (show_protocol_widget, window->show_protocol);
+
+

No need to add extra blank lines.

Created attachment 149723 [details] [review]
protocol icon overlay2

Hi Guillaume,

That should correct coding style, bugs and factor 2 functions.

I am a bit confused with tabs and ident.
This is what I am using:
set tw=78 ts=2 sts=2 sw=2 noet
which I got from telepathy coding style.

Regarding the icon of the status icon, I am not sure what was used when the column G_TYPE_STRING, so I defaulted to 32 in normal mode, to 24 in compact mode.

Review of attachment 149723 [details] [review]:

Thanks.

Your patch still introduce trailing spaces. Please use "make check".

Coding style are a bit of a mess atm. Old files are using the Gossip coding style but new ones the Telepathy one. See http://live.gnome.org/Empathy#Developers.

The default size of the icon (in the current code) is GTK_ICON_SIZE_MENU. You should continue to use this size. Also, the size of the icon doesn't change in "compat" mode.

Created attachment 149794 [details] [review]
protocol icon overlay3

Hi,

Sorry for the trailing space...

Here is the last change made:
    Changing size of status icon to GTK_ICON_SIZE_MENU
    
    * libempathy-gtk/empathy-ui-utils.c:
      - use numerator/dominator variables to scale protocol icon
      - scale protocol icon to a ratio of 3/4 instead of 2/3
    * Some more conding style cleanup
    * use explicit comparison for pointers

Hopefully, coding style is right this time :)

Review of attachment 149794 [details] [review]:

Great! I added some last style comments. After that I think we'll be good. :)

::: libempathy-gtk/empathy-ui-utils.c
@@ +235,3 @@
+{
+	TpAccount     *account;
+	g_return_val_if_fail (EMPATHY_IS_CONTACT (contact), NULL);

add some space (\n) here.

@@ +554,3 @@
+	}
+
+	if (!show_protocol) {

No need for { } in one-line early returns.

@@ +594,3 @@
+	filename = empathy_filename_from_icon_name (tp_account_get_icon_name (account),
+						    GTK_ICON_SIZE_MENU);
+	if ( filename != NULL ) {

if (filename != NULL) {

::: libempathy-gtk/empathy-ui-utils.h
@@ +83,3 @@
+GdkPixbuf *   empathy_pixbuf_contact_status_icon_with_icon_name (EmpathyContact   *contact,
+							 const gchar       *icon_name,
+               gboolean          show_protocol);

args are miss-aligned.

::: src/empathy-main-window.c
@@ +762,3 @@
+					EmpathyMainWindow *window)
+{
+	gboolean     value;

gboolean value;

Created attachment 149838 [details] [review]
protocol_icon-20091216 git format-patch

Hi Guillaume,

really sorry for wasting your time on these silly coding style issues.

Well, I fixed the errors you mentioned in your last comment and I scrolled over the global patch protocol_icon-20091216-diffformat.patch and it seems correct... so did it yesterday so I dont guarantee that my fingers did not put extra spaces anywhere :p

Also attached is protocol_icon-20091216-formatpatch.patch which is the mail formatted patch.

Cheers

Created attachment 149839 [details] [review]
diff formatted

Merged to master. \o/
Thanks a lot for your great work.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.

I like to have the protocol icons since there is no way to have meta-contacts yet. But in my opinion we should find a better way to do this.

some thoughts:
- the icons get very confusing if the status is not "available".
- "show protocols" should be off by default, not on
- why is there no protocol icon in compact view?
- the icons are used with protocol in contact view, without it in accounts-dialog-infobar. imho we should be more consistent.

I will create a quick mockup...

Created attachment 150442 [details]
mockup

this change does make it look ugly. especially the protocol icons are not really clear.
don't know how to make it better though.

what do you mean by the protocol icons are not really clear? do you mean the images are not clear? thats because its a mockup...I took the images from somewhere else and scaled them.

the mockup should just show how it *could* be done. this is not a screenshot of the implementation...

i was refering to the way it is now in 2.2.9.4 ie how it is currently merged in master

ah sorry =) I thought you were talking about my mockup...

(In reply to comment #15)
> - the icons get very confusing if the status is not "available".

We decided to do it that way to not add an extra column in the contact list which is already too crowded.

> - "show protocols" should be off by default, not on

Fixed.

> - why is there no protocol icon in compact view?

Good point. Can you please open a bug about that?

> - the icons are used with protocol in contact view, without it in
> accounts-dialog-infobar. imho we should be more consistent.

Not sure. The protocol is pretty obvious in that case.

Created attachment 151169 [details]
how protocol icons should be

What about this? This is how CarrierIM (based on pidgin) is. This is how gAIM used to be. I find this method to be completely perfect. No wasted space. No useless green orbs or triangles. We get the protocol information as well as an overlay of away status.