Hi Jonny, I read your branch, some comments:

- you should initialize all the GTK+ packing and signals inside empathy_contact_selector_init and drop the _constructor implementation.
- when you connect to the GtkComboBox signals, there's no need to cast the object with GTK_COMBO_BOX, just use g_signal_connect (foo, ...). Also, there's no need to cast the user_data to gpointer, and you can use EmpathyContactSelector * instead of GtkComboBox * in the callbacks prototypes to avoid a macro.
- as _dispose can be called multiple times from some bindings, you should have a dispose_run variable (usually inside priv) which is set to TRUE after the first dispose has run; you can have a look at EmpathyAccountManager for an example of this.
- in contact_selector_get_number_online_contacts (), and contact_selector_get_iter_for_blank_contact (), there's no need to go through a GtkTreePath to get the first iter, just use gtk_tree_model_get_iter_first ().
- in contact_selector_get_iter_for_blank_contact (), why do you go through all the list to see if there's a blank contact? Isn't it always saved in the first position if present? If I'm not overlooking something, you could have a boolean variable is_blank_present and just remove the first element if the variable is TRUE.
- when you need the private structure but not the EmpathyContactSelector itself, (e.g. in _set_property and _get_property), you can use GET_PRIV directly without casting the object to EmpathyContactSelector, as GET_PRIV already does the cast for you.

Cosimo, thanks for your review!

(In reply to comment #1)
> - you should initialize all the GTK+ packing and signals inside
> empathy_contact_selector_init and drop the _constructor implementation.

So, I'm a bit of a noob with GObjects, but surely the constructor is required as the store (priv->store) property isn't set in _init, whereas it is in _constructor. Feel free to tell me I'm wrong though.

> - when you connect to the GtkComboBox signals, there's no need to cast the
> object with GTK_COMBO_BOX, just use g_signal_connect (foo, ...). Also, there's
> no need to cast the user_data to gpointer, and you can use
> EmpathyContactSelector * instead of GtkComboBox * in the callbacks prototypes
> to avoid a macro.

Fixed.

> - as _dispose can be called multiple times from some bindings, you should have
> a dispose_run variable (usually inside priv) which is set to TRUE after the
> first dispose has run; you can have a look at EmpathyAccountManager for an
> example of this.

Done.

> - in contact_selector_get_number_online_contacts (), and
> contact_selector_get_iter_for_blank_contact (), there's no need to go through a
> GtkTreePath to get the first iter, just use gtk_tree_model_get_iter_first ().

Done.

> - in contact_selector_get_iter_for_blank_contact (), why do you go through all
> the list to see if there's a blank contact? Isn't it always saved in the first
> position if present? If I'm not overlooking something, you could have a boolean
> variable is_blank_present and just remove the first element if the variable is
> TRUE.

So, after a tiny bit of playing with this, it appears the store is ordered before you can just remove the first element, so as a result "Select a contact" remains present, but "Aaron Ardvark" gets removed..

> - when you need the private structure but not the EmpathyContactSelector
> itself, (e.g. in _set_property and _get_property), you can use GET_PRIV
> directly without casting the object to EmpathyContactSelector, as GET_PRIV
> already does the cast for you.

Done.

P.S. These changes are in my "selector" branch of 
git://git.collabora.co.uk/git/user/jonny/empathy.git

Cosimo has the latest branch:

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

Some comments:

 - empathy_contact_selector_get_selected() --> it returns a ref so it should be _dup_selected() and I'm sure there is places where the contact is leaked.

+  /* This is required otherwise the dispatcher isn't ref'd, and so it
+   * disappears by the time the callback gets called. It's deliberately not
+   * freed otherwise it segfaults... sigh */
+  empathy_dispatcher_dup_singleton ();
+  empathy_dispatcher_chat_with_contact (contact, chat_cb, NULL);
--> We should really fix that now instead of duplicating such hack everywhere and then forget to fix them. It is bad to leak empathy singletons in all processes.

 - empetit.c, destroy_cb() --> you can give gtk_main_quit directly as callback, no need for that function.

 - empathy_contact_selector_new() --> should return a GtkWidget* I think. GTK always return a GtkWidget* for constructors of widget subclasses. This should be fixed in other places in libempathy-gtk's API I guess...

 - instead of getting a EmpathyContactListStore as construct param and then change its properties, which the user could change later... I think we should get a EmpathyContactList iface as construct param and keep the store completely private. Especially since we add a blank contact in the store!

 - contact_selector_add_blank_contact: You can use gtk_tree_store_insert_with_values.

 - contact_selector_get_iter_for_blank_contact: tmp_contact is leaked. You can simplify a bit the code with a for loop:
for (ok = gtk_tree_model_get_iter_first(); ok; ok = gtk_tree_model_iter_next())

 - contact_selector_get_number_online_contacts: same as above.

 - contact_selector_manage_sensitivity: avoid calling function in var declaration. That FIXME should get fixed, no?

 - contact_selector_notify_popup_shown_cb, contact_selector_changed_cb and contact_selector_store_row_changed_cb --> they can be removed if you use g_signal_connect_swapped and give directly the function called from those cb as callback.

Otherwise it seems fine, thanks for the work!

Oh, forgot a little comment:

+  icon_path = g_build_path (G_DIR_SEPARATOR_S, PKGDATADIR, "icons", NULL);
+  gtk_icon_theme_append_search_path (gtk_icon_theme_get_default (), icon_path);
+  g_free (icon_path);

--> that can be replaced by empathy_gtk_init().

(In reply to comment #5)
>  - empathy_contact_selector_get_selected() --> it returns a ref so it should be
> _dup_selected() and I'm sure there is places where the contact is leaked.
> 
> +  /* This is required otherwise the dispatcher isn't ref'd, and so it
> +   * disappears by the time the callback gets called. It's deliberately not
> +   * freed otherwise it segfaults... sigh */
> +  empathy_dispatcher_dup_singleton ();
> +  empathy_dispatcher_chat_with_contact (contact, chat_cb, NULL);
> --> We should really fix that now instead of duplicating such hack everywhere
> and then forget to fix them. It is bad to leak empathy singletons in all
> processes.

Removed this comment and the ref. I fixed the ref problem in another branch that (at the time of writing) has not been merged. Therefore, the selector should obviously wait until that is fixed.

>  - empetit.c, destroy_cb() --> you can give gtk_main_quit directly as callback,
> no need for that function.

Done.

>  - empathy_contact_selector_new() --> should return a GtkWidget* I think. GTK
> always return a GtkWidget* for constructors of widget subclasses. This should
> be fixed in other places in libempathy-gtk's API I guess...

Done.

>  - instead of getting a EmpathyContactListStore as construct param and then
> change its properties, which the user could change later... I think we should
> get a EmpathyContactList iface as construct param and keep the store completely
> private. Especially since we add a blank contact in the store!

The store is now created in empathy_contact_selector_new. Is this okay or do you want the EmpathyContactList to be set as a GObject param?

>  - contact_selector_add_blank_contact: You can use
> gtk_tree_store_insert_with_values.

Done.

>  - contact_selector_get_iter_for_blank_contact: tmp_contact is leaked. You can
> simplify a bit the code with a for loop:
> for (ok = gtk_tree_model_get_iter_first(); ok; ok = gtk_tree_model_iter_next())
>  - contact_selector_get_number_online_contacts: same as above.

Cool, done.

>  - contact_selector_manage_sensitivity: avoid calling function in var
> declaration. That FIXME should get fixed, no?

Done.

>  - contact_selector_notify_popup_shown_cb, contact_selector_changed_cb and
> contact_selector_store_row_changed_cb --> they can be removed if you use
> g_signal_connect_swapped and give directly the function called from those cb as
> callback.

Done.

(In reply to comment #6)
> +  icon_path = g_build_path (G_DIR_SEPARATOR_S, PKGDATADIR, "icons", NULL);
> +  gtk_icon_theme_append_search_path (gtk_icon_theme_get_default (),
> icon_path);
> +  g_free (icon_path);
> 
> --> that can be replaced by empathy_gtk_init().

Done.

My branch is at: http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/selector

Merged today.