GNOME Bugzilla – Bug 663763
Some contact/invidiual misc patches
Last modified: 2011-11-16 14:50:49 UTC
This is the first part of bug #663387 containing the simple, uncontroverisal patches.
Created attachment 201130 [details] [review] details_update_show: skip empty field
Created attachment 201131 [details] [review] update_weak_contact: use a greater or equal comparaison That way we'll pick at least one TpContact if there is only one contact in the individual and he doesn't have any presence (IRC for example).
Created attachment 201132 [details] [review] factor out empathy_contact_info_create_channel_list_label() Move it to empathy-contactinfo-utils so we'll be able to re-use it in empathy-individual-widget as well.
Created attachment 201133 [details] [review] individual-widget: factor out add_row()
Created attachment 201134 [details] [review] individual-widget: display channels list if available This will be needed when using this widget in MUC.
Created attachment 201135 [details] [review] individual-store: use self->priv pattern https://bugzilla.gnome.org/show_bug.cgi?id=663387
Created attachment 201136 [details] [review] individual-view: remove explicit boolean comparaisons https://bugzilla.gnome.org/show_bug.cgi?id=663387
Created attachment 201137 [details] [review] individual-view: add an option to disable uninteresting filtering This is needed when being used in a muc. https://bugzilla.gnome.org/show_bug.cgi?id=663387
Created attachment 201138 [details] [review] individual-view: don't display menu if empathy_folks_individual_contains_contact() fails The individual menu already asserts that's the case. And there is no point displaying a menu anyway. https://bugzilla.gnome.org/show_bug.cgi?id=663387
Created attachment 201139 [details] [review] empathy_individual_store_remove_individual: use EMPATHY_INDIVIDUAL_STORE_COL_NAME https://bugzilla.gnome.org/show_bug.cgi?id=663387
Created attachment 201140 [details] [review] main-window: use the EmpathyIndividual flavor of some types We switched to EmpathyIndividualView a while ago... https://bugzilla.gnome.org/show_bug.cgi?id=663387
Branch is http://cgit.collabora.com/git/user/cassidy/empathy/log/?h=muc-individual-trivia-663763
Review of attachment 201130 [details] [review]: Looks good to me.
Review of attachment 201131 [details] [review]: ::: libempathy-gtk/empathy-individual-widget.c @@ +195,2 @@ if (folks_presence_details_typecmp ( + presence_type_cur, presence_type) >= 0) Wouldn't it be a bit clearer to instead do: if (tp_contact == NULL || folks_presence_details_typecmp (presence_type_cur, presence_type) > 0) and drop the comment? This would also ensure we don't change the current behaviour of the code for non-IRC cases.
Review of attachment 201132 [details] [review]: ::: libempathy-gtk/empathy-contactinfo-utils.c @@ +313,3 @@ + * ensure it's actually a valid URI. g_string_append_uri_escaped() + * escapes way more than we actually need to; so we're just using + * g_markup_escape_text directly. g_string_append_uri_escaped() is definitely wrong to use here, since it's for escaping URI parameters to be appended to a URI, rather than HTML to be appended to a markup string. @@ +324,3 @@ + g_free (escaped); + } + Looks like channels is not getting unreffed after the loop.
Review of attachment 201133 [details] [review]: Looks good to me.
Review of attachment 201134 [details] [review]: Also looks good to me.
Review of attachment 201135 [details] [review]: Looks good to me.
Review of attachment 201136 [details] [review]: Eww. I guess this is in the Telepathy code style though. :-(
Review of attachment 201137 [details] [review]: This looks OK, but what about the call to empathy_folks_persona_is_interesting() in got_avatar()? ::: libempathy-gtk/empathy-individual-view.c @@ +2216,3 @@ + PROP_SHOW_UNINTERESTING, + g_param_spec_boolean ("show-uninteresting", + "Show Unteresting Individuals", s/Unteresting/Uninteresting/ @@ +2217,3 @@ + g_param_spec_boolean ("show-uninteresting", + "Show Unteresting Individuals", + "Whether the view should filter out individuals using " s/should/should not/?
Review of attachment 201138 [details] [review]: ::: libempathy-gtk/empathy-individual-view.c @@ +2619,3 @@ + if (!empathy_folks_individual_contains_contact (individual)) + return NULL; Looks like individual should be unreffed before returning.
Review of attachment 201139 [details] [review]: Good catch!
Review of attachment 201140 [details] [review]: Looks good to me.
Attachment 201130 [details] pushed as e6e284c - details_update_show: skip empty field Attachment 201133 [details] pushed as 0a04322 - individual-widget: factor out add_row() Attachment 201135 [details] pushed as 8a831cc - individual-store: use self->priv pattern Attachment 201136 [details] pushed as c85a456 - individual-view: remove explicit boolean comparaisons Attachment 201139 [details] pushed as bd386d1 - empathy_individual_store_remove_individual: use EMPATHY_INDIVIDUAL_STORE_COL_NAME Attachment 201140 [details] pushed as 65d16f7 - main-window: use the EmpathyIndividual flavor of some types
Review of attachment 201131 [details] [review]: ::: libempathy-gtk/empathy-individual-widget.c @@ +195,2 @@ if (folks_presence_details_typecmp ( + presence_type_cur, presence_type) >= 0) You're totally right. Changed.
Review of attachment 201132 [details] [review]: ::: libempathy-gtk/empathy-contactinfo-utils.c @@ +313,3 @@ + * ensure it's actually a valid URI. g_string_append_uri_escaped() + * escapes way more than we actually need to; so we're just using + /* Is there channels? */ I didn't change this code, but it actually uses g_markup_escape_text(). @@ +324,3 @@ + g_free (escaped); + } + Good point; fixed.
Review of attachment 201137 [details] [review]: I added the check to got_avatar() as well. ::: libempathy-gtk/empathy-individual-view.c @@ +2216,3 @@ + PROP_SHOW_UNINTERESTING, + g_param_spec_boolean ("show-uninteresting", + "Show Unteresting Individuals", fixed. @@ +2217,3 @@ + g_param_spec_boolean ("show-uninteresting", + "Show Unteresting Individuals", + "Whether the view should filter out individuals using " fixed.
Review of attachment 201138 [details] [review]: ::: libempathy-gtk/empathy-individual-view.c @@ +2619,3 @@ + if (!empathy_folks_individual_contains_contact (individual)) + return NULL; fixed.
Created attachment 201350 [details] [review] update_weak_contact: use a greater or equal comparaison That way we'll pick at least one TpContact if there is only one contact in the individual and he doesn't have any presence (IRC for example).
Created attachment 201351 [details] [review] factor out empathy_contact_info_create_channel_list_label() Move it to empathy-contactinfo-utils so we'll be able to re-use it in empathy-individual-widget as well.
Created attachment 201352 [details] [review] individual-view: add an option to disable uninteresting filtering This is needed when being used in a muc. https://bugzilla.gnome.org/show_bug.cgi?id=663387
Created attachment 201353 [details] [review] individual-view: don't display menu if empathy_folks_individual_contains_contact() fails The individual menu already asserts that's the case. And there is no point displaying a menu anyway. https://bugzilla.gnome.org/show_bug.cgi?id=663387
Review of attachment 201350 [details] [review]: Great.
Review of attachment 201351 [details] [review]: ::: libempathy-gtk/empathy-contactinfo-utils.c @@ +303,3 @@ + { + g_ptr_array_unref (channels); + return NULL; Just noticed that label_markup isn't freed in this case. @@ +314,3 @@ + * escapes way more than we actually need to; so we're just using + * g_markup_escape_text directly. + */ My point here was that the comment is wrong to suggest that g_string_append_uri_escaped() would ever be correct to use here.
Review of attachment 201352 [details] [review]: ::: libempathy-gtk/empathy-individual-view.c @@ +2510,3 @@ + if (!priv->show_uninteresting && + empathy_folks_persona_is_interesting (persona)) Shouldn't this be (priv->show_uninteresting || empathy_folks_persona_is_interesting (persona))? If we're showing uninteresting personas, we want to consider all of them when counting how many personas are in the individual. In fact, in the case that (priv->show_uninteresting == TRUE), you could just skip doing that entire loop, and set persona_count = gee_collection_get_size (personas); instead.
Review of attachment 201353 [details] [review]: Looks good to me.
Review of attachment 201351 [details] [review]: ::: libempathy-gtk/empathy-contactinfo-utils.c @@ +303,3 @@ + { + g_ptr_array_unref (channels); + GString *label_markup = g_string_new (""); Good catch; fixed. @@ +314,3 @@ + * escapes way more than we actually need to; so we're just using + * g_markup_escape_text directly. + channels = g_ptr_array_new (); Ahh ok. I removed that part of the comment then.
Created attachment 201517 [details] [review] factor out empathy_contact_info_create_channel_list_label() Move it to empathy-contactinfo-utils so we'll be able to re-use it in empathy-individual-widget as well.
Review of attachment 201352 [details] [review]: ::: libempathy-gtk/empathy-individual-view.c @@ +2510,3 @@ + if (!priv->show_uninteresting && + empathy_folks_persona_is_interesting (persona)) Good point. Fixed.
Created attachment 201518 [details] [review] individual-view: add an option to disable uninteresting filtering This is needed when being used in a muc.
Review of attachment 201517 [details] [review]: Make it so!
Review of attachment 201518 [details] [review]: Looks good to me.
Attachment 201131 [details] pushed as 7c3389c - update_weak_contact: use a greater or equal comparaison Attachment 201134 [details] pushed as 0366e51 - individual-widget: display channels list if available Attachment 201138 [details] pushed as 20772a7 - individual-view: don't display menu if empathy_folks_individual_contains_contact() fails Attachment 201350 [details] pushed as 7c3389c - update_weak_contact: use a greater or equal comparaison Attachment 201353 [details] pushed as 20772a7 - individual-view: don't display menu if empathy_folks_individual_contains_contact() fails Attachment 201517 [details] pushed as caae76c - factor out empathy_contact_info_create_channel_list_label() Attachment 201518 [details] pushed as 9882c0f - individual-view: add an option to disable uninteresting filtering
All merged; thanks!
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.