in this branch I use gtkinfobar instead of the mix it was before. I also added a retry button for bug #587216, at the moment all 3 buttons are shown, this could be improved (discussion at bug #587216)

http://git.collabora.co.uk/?p=user/kaserf/empathy.git;a=shortlog;h=refs/heads/infobar

Thanks for your patch Felix. Here is my review.

• The info bar doesn't have a close button as the old one did. Do we have API to add a 'cross' closing the info bar? If not, I guess you should add an 'Ignore' button.
• Your git commit message should contain the bug number: "(#599176)"
• We tend to make atomic commit when posible. For example, this branch should have be splitted in different commits:
∘ one replacing old code with GtkInfoBar
∘ one adding the 'retry' button
∘ one adding the 'disable' button
• You added trailing spaces/tabes (line 335 for example), please remove them. git show should show them in red. "make check" should detect those too. As a general rule, you should always run "make check" before proposing a branch.
• GtkInfoBar has been added in GTK+ 2.18 but we still depend on 2.16; you should bump this dep in configure.ac
• main_window_error_display: I'd rename "child" to "info_bar"
• Shouldn't you call gtk_info_bar_set_message_type (GTK_MESSAGE_WARNING)?
• Wouldn't it be easier to use gtk_info_bar_new_with_buttons?
• Why not create the label after g_markup_printf_escaped and pass the str to gtk_label_new ?
• Why are you setting the label as data? It doesn't seem used after.
• I'd rename EMPATHY_RESPONSE_* to INFO_BAR_RESPONSE_* for extra clarity
• I'm wondering if we shouldn't display the icon of the account instead of GTK_STOCK_DIALOG_WARNING. It could be useful for user to quickly identified the faulty account. Furthemore, the background of the GtkInfoBar is already a hint that the msg is a warning. What do you think?

Currently 17 Empathy tickets are set as GNOME 2.30 blockers, hence mass-removing.
Guillaume: Please use normal Target Milestones instead. If you really think that this specific issue here is a 2.30 blocker then please restore the GNOME target and set corresponding importance values.

Thanks for the review. Here is my review. :)

* I substituted the "close" button with the disable button because it seemed more clear to me. I will check to add the cross, but why do we need a close button? does close mean ignore? or keep trying?
* I already experimented with set_message_type, but somehow I decided that it looks best the way it is...I guess you're right ;) its better to set the message type.
* the label is used to alter the error message in the first if in main_window_error_display. that code comes from the old implementation
* I agree on showing the image (icon) of the account...where do I get those images?

for the rest: all fixed in the branch! I will keep the general things in mind for the next time...

(In reply to comment #4)
> * I substituted the "close" button with the disable button because it seemed
> more clear to me. I will check to add the cross, but why do we need a close
> button? does close mean ignore? or keep trying?

Close means "Ignore" indeed, so "Ignore" is probably a better label.

> * I agree on showing the image (icon) of the account...where do I get those
> images?

I guess you should call empathy_account_manager_get_account_for_connection () and then use empathy_account_get_icon_name(). Probably best to check with Jonny though.

> for the rest: all fixed in the branch! I will keep the general things in mind
> for the next time...

Awesome; thanks!

+       str = g_markup_printf_escaped ("<b>%s</b>\n\n%s",
                                       empathy_account_get_display_name (account),
                                       message);
+       label = gtk_label_new ("");


you should pass str to g_label_new. You should probably call gtk_label_set_use_markup() then.

(In reply to comment #6)
> +       str = g_markup_printf_escaped ("<b>%s</b>\n\n%s",
>                                        empathy_account_get_display_name
> (account),
>                                        message);
> +       label = gtk_label_new ("");
> 
> 
> you should pass str to g_label_new. You should probably call
> gtk_label_set_use_markup() then.

that doesn't change much...

your version:
str = g_markup_printf_escaped ("<b>%s</b>\n\n%s",
	       empathy_account_get_display_name (account),
	       message);
label = gtk_label_new (str);
gtk_label_set_use_markup (GTK_LABEL (label), TRUE);
gtk_widget_show (label);

my version:
str = g_markup_printf_escaped ("<b>%s</b>\n\n%s",
	       empathy_account_get_display_name (account),
	       message);
label = gtk_label_new ("");
gtk_label_set_markup (GTK_LABEL (label), str);
gtk_widget_show (label);

http://git.collabora.co.uk/?p=user/kaserf/empathy.git;a=shortlog;h=refs/heads/infobar

updated branch

Looks good to me. Please merge.

fixed in master

One query is that in some distributions like Ubuntu with Karmic using messaging indicator and also with the default manner that empathy shows its icon in the taskbar, if i dont have empathy open ie the contact list visible i wont get to know that an error has occured and i will think the accounts are all OK. maybe a notification should also be shown?

what about the way pidgin solves this: they have a different tray icon if one account tries to connect. Instead of the current status icon we could display something else (maybe the status icon with some modification?)

A notification is nice...but it is useless if you're do not see it when it pops up (when you're afk or something)

anyways...I think this issue should get its own bug!

bug filed https://bugzilla.gnome.org/show_bug.cgi?id=601500