GNOME Bugzilla – Bug 663387
Use the individual manager to display muc members
Last modified: 2011-11-24 11:30:07 UTC
MUCs are the last user of EmpathyContactListView and EmpathyContactListStore. Porting it to the IndividualManager/View would allow us to get rid of a lot of old code.
Created attachment 200997 [details] [review] individual-store: use self->priv pattern
Created attachment 200998 [details] [review] individual-store: expose some attributes and methods as 'protected' This will be needed when abstracting EmpathyIndividualStore.
Created attachment 200999 [details] [review] Abstract the individual store We now have EmpathyIndividualStoreManager which implements the store using EmpathyIndividualManager as its contact source.
Created attachment 201000 [details] [review] factor out empathy_create_individual_from_tp_contact()
Created attachment 201001 [details] [review] add EMPATHY_INDIVIDUAL_FEATURE_ADD_CONTACT
Created attachment 201002 [details] [review] Add individual-store-channel This will allow us to use the individual view to display muc members.
Created attachment 201003 [details] [review] chat: use the individual view/store rather than the contact one The great unification ! EmpathyChat was the last user of the contact store/view, everything now use the individual ones.
Created attachment 201004 [details] [review] remove EmpathyContactManager's test We want to get rid of it any way.
Created attachment 201005 [details] [review] empathy_individual_store_remove_individual: use EMPATHY_INDIVIDUAL_STORE_COL_NAME
Created attachment 201006 [details] [review] main-window: use the EmpathyIndividual flavor of some types We switched to EmpathyIndividualView a while ago...
Created attachment 201007 [details] [review] Remove obsolete contact-list-{store,view} Hourrah \o/
Humm don't review this yet. I found some issues in the branch.
One of my bug is actually in Gtk+: bug #663694
Created attachment 201067 [details] [review] individual-store: use self->priv pattern
Created attachment 201068 [details] [review] individual-store: expose some attributes and methods as 'protected' This will be needed when abstracting EmpathyIndividualStore.
Created attachment 201069 [details] [review] Abstract the individual store We now have EmpathyIndividualStoreManager which implements the store using EmpathyIndividualManager as its contact source.
Created attachment 201070 [details] [review] factor out empathy_create_individual_from_tp_contact()
Created attachment 201071 [details] [review] add EMPATHY_INDIVIDUAL_FEATURE_ADD_CONTACT
Created attachment 201072 [details] [review] Add individual-store-channel This will allow us to use the individual view to display muc members.
Created attachment 201073 [details] [review] individual-view: remove explicit boolean comparaisons
Created attachment 201074 [details] [review] individual-view: add an option to disable uninteresting filtering This is needed when being used in a muc.
Created attachment 201075 [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.
Created attachment 201076 [details] [review] chat: use the individual view/store rather than the contact one The great unification ! EmpathyChat was the last user of the contact store/view, everything now use the individual ones.
Created attachment 201077 [details] [review] remove EmpathyContactManager's test We want to get rid of it any way.
Created attachment 201078 [details] [review] empathy_individual_store_remove_individual: use EMPATHY_INDIVIDUAL_STORE_COL_NAME
Created attachment 201079 [details] [review] main-window: use the EmpathyIndividual flavor of some types We switched to EmpathyIndividualView a while ago...
Created attachment 201080 [details] [review] Remove obsolete contact-list-{store,view} Hourrah \o/
This branch is getting pretty big. Let's split it a bit to make review and merge easier. First part is bug #663763
The first patches have been merged, here is the interesting stuff:
Created attachment 201534 [details] [review] individual-store: expose some attributes and methods as 'protected' This will be needed when abstracting EmpathyIndividualStore.
Created attachment 201535 [details] [review] Abstract the individual store We now have EmpathyIndividualStoreManager which implements the store using EmpathyIndividualManager as its contact source.
Created attachment 201536 [details] [review] factor out empathy_create_individual_from_tp_contact()
Created attachment 201537 [details] [review] add EMPATHY_INDIVIDUAL_FEATURE_ADD_CONTACT
Created attachment 201538 [details] [review] Add individual-store-channel This will allow us to use the individual view to display muc members.
Created attachment 201539 [details] [review] chat: use the individual view/store rather than the contact one The great unification ! EmpathyChat was the last user of the contact store/view, everything now use the individual ones.
Created attachment 201540 [details] [review] remove EmpathyContactManager's test We want to get rid of it any way.
Created attachment 201541 [details] [review] Remove obsolete contact-list-{store,view} Hourrah \o/
Review of attachment 201536 [details] [review]: Looks good to me.
Review of attachment 201537 [details] [review]: ::: libempathy-gtk/empathy-individual-menu.c @@ +629,3 @@ break; + case PROP_STORE: + priv->store = g_value_dup_object (value); Might be useful to have a comment here that the property's read only (and hence this isn't a memory leak). @@ +705,3 @@ { g_return_val_if_fail (FOLKS_IS_INDIVIDUAL (individual), NULL); g_return_val_if_fail (features != EMPATHY_INDIVIDUAL_FEATURE_NONE, NULL); Need to add a precondition for store. @@ +1494,3 @@ + while (gee_iterator_next (iter)) + { + TpfPersona *persona = gee_iterator_get (iter); Need to unref the persona at various points. (gee_iterator_get() returns a ref IIRC.) @@ +1544,3 @@ + to_add = contact; + break; + } Need to unref the iter.
Review of attachment 201534 [details] [review]: ::: libempathy-gtk/empathy-individual-store.h @@ +83,3 @@ + /* protected */ + gboolean show_active; + guint setup_idle_id; Yuck. Is there no better way to do this? show_active is only used by individual_store_manager_groups_changed_cb() to prevent removing and re-adding an individual from showing up as activity. Why not add a protected empathy_individual_store_refresh_individual() method which does this, but which keeps show_active internal to EmpathyIndividualStore? setup_idle_id is only used in empathy_individual_store_set_show_groups(). There must be a way to tidy up the logic there so that we don't expose it in this struct.
Review of attachment 201535 [details] [review]: ::: libempathy-gtk/empathy-individual-store-manager.c @@ +86,3 @@ + + individual_store_remove_individual_and_disconnect (store, l->data); + } As below, I suggest the removals be done before the additions. @@ +110,3 @@ + empathy_individual_store_remove_individual (store, individual); + empathy_individual_store_add_individual (store, individual); + store->show_active = show_active; See my comments on the previous patch about show_active. @@ +138,3 @@ + individual_store_manager_members_changed_cb (self->priv->manager, "initial add", + individuals, NULL, 0, self); + g_list_free (individuals); If the first list element isn't an individual (why would this ever happen?) the list will be leaked. @@ +152,3 @@ + EmpathyIndividualStore *store = EMPATHY_INDIVIDUAL_STORE (self); + + self->priv->manager = g_object_ref (manager); Might be useful to put a comment or assertion here that the property's read-only, meaning this won't leak. @@ +177,3 @@ + + /* remove old contact */ + individual_store_remove_individual_and_disconnect (store, old_individual); I think it would be better to remove the individual before adding the new one here. In the case that old_individual == new_individual, doing the removal second will probably break things. (This has caused bugs in folks before.) @@ +181,3 @@ + +static void +individual_store_manager_dispose (GObject *object) If setup_idle_id is non-zero in dispose() you should cancel the idle callback before it runs after the object has been destroyed and explodes. @@ +283,3 @@ + PROP_INDIVIDUAL_MANAGER, + g_param_spec_object ("individual-manager", + "The individual manager", s/The//
Review of attachment 201538 [details] [review]: ::: libempathy-gtk/empathy-individual-store-channel.c @@ +82,3 @@ + continue; + + individual = empathy_create_individual_from_tp_contact (contact); Looks like a ref to this individual gets leaked at the end of the loop iteration. @@ +133,3 @@ + + add_members (self, added); + remove_members (self, removed); Should remove members before adding them, as mentioned in one of the patch reviews above. @@ +171,3 @@ + TpChannel *channel) +{ + GQuark features[] = { TP_CHANNEL_FEATURE_CONTACTS }; Shouldn't this array be 0-terminated? @@ +173,3 @@ + GQuark features[] = { TP_CHANNEL_FEATURE_CONTACTS }; + + self->priv->channel = g_object_ref (channel); Might be useful to add a comment here explaining that the channel is construct-only, and so this isn't a memory leak. @@ +198,3 @@ + + G_OBJECT_CLASS (empathy_individual_store_channel_parent_class)->dispose ( + object); Looks like you're leaking a ref to priv->channel here. @@ +247,3 @@ + GPtrArray *members; + + g_hash_table_remove_all (self->priv->individuals); Why doesn't this call remove_members()? Doesn't this mean we're leaking the signal handlers connected to those individuals? @@ +274,3 @@ + PROP_CHANNEL, + g_param_spec_object ("individual-channel", + "The individual channel", s/The//
Review of attachment 201539 [details] [review]: Looks good to me, with one tiny comment. ::: libempathy-gtk/empathy-chat.c @@ +2751,3 @@ + store = EMPATHY_INDIVIDUAL_STORE ( + empathy_individual_store_channel_new ((TpChannel *) priv->tp_chat)); Out of interest, why isn't this typecast using the TP_CHANNEL() macro?
Review of attachment 201540 [details] [review]: Die, code. Die.
Review of attachment 201541 [details] [review]: Yay! \o/
Review of attachment 201537 [details] [review]: ::: libempathy-gtk/empathy-individual-menu.c @@ +629,3 @@ break; + case PROP_STORE: + priv->store = g_value_dup_object (value); added. @@ +705,3 @@ { g_return_val_if_fail (FOLKS_IS_INDIVIDUAL (individual), NULL); g_return_val_if_fail (features != EMPATHY_INDIVIDUAL_FEATURE_NONE, NULL); added @@ +1494,3 @@ + while (gee_iterator_next (iter)) + { + g_object_unref (contact); You're right; fixed. @@ +1544,3 @@ + to_add = contact; + break; + Rah shit. Fixed. I suspect more empathy code does this error, I'll double check.
Review of attachment 201535 [details] [review]: ::: libempathy-gtk/empathy-individual-store-manager.c @@ +86,3 @@ + + individual_store_remove_individual_and_disconnect (store, l->data); + } changed. @@ +138,3 @@ + individual_store_manager_members_changed_cb (self->priv->manager, "initial add", + individuals, NULL, 0, self); + g_list_free (individuals); I don't see any reason why. I removed the test. @@ +152,3 @@ + EmpathyIndividualStore *store = EMPATHY_INDIVIDUAL_STORE (self); + + self->priv->manager = g_object_ref (manager); Good idea; done. @@ +177,3 @@ + + /* remove old contact */ + individual_store_remove_individual_and_disconnect (store, old_individual); Changed. @@ +181,3 @@ + +static void +individual_store_manager_dispose (GObject *object) That's done in individual_store_dispose(). But I agree, we should avoid exposing that in the class. I'll take a look. @@ +283,3 @@ + PROP_INDIVIDUAL_MANAGER, + g_param_spec_object ("individual-manager", + "The individual manager", fixed.
Review of attachment 201538 [details] [review]: ::: libempathy-gtk/empathy-individual-store-channel.c @@ +82,3 @@ + continue; + + individual = empathy_create_individual_from_tp_contact (contact); Good point; fixed. @@ +133,3 @@ + + add_members (self, added); + remove_members (self, removed); changed. @@ +171,3 @@ + TpChannel *channel) +{ + GQuark features[] = { TP_CHANNEL_FEATURE_CONTACTS }; Yes; fixed. @@ +173,3 @@ + GQuark features[] = { TP_CHANNEL_FEATURE_CONTACTS }; + + self->priv->channel = g_object_ref (channel); done. @@ +198,3 @@ + + G_OBJECT_CLASS (empathy_individual_store_channel_parent_class)->dispose ( + object); nice catch; fixed. @@ +247,3 @@ + GPtrArray *members; + + g_hash_table_remove_all (self->priv->individuals); You're right. Fixed. @@ +274,3 @@ + PROP_CHANNEL, + g_param_spec_object ("individual-channel", + "The individual channel", fixed.
Review of attachment 201539 [details] [review]: I've been told that this kind of cast aren't that fast because of GLib's type system checks (but maybe that's a legend?). So I sometimes avoid to use them when casting from a well known type (ie not from a gpointer).
Review of attachment 201534 [details] [review]: ::: libempathy-gtk/empathy-individual-store.h @@ +83,3 @@ + /* protected */ + gboolean show_active; + guint setup_idle_id; Agreed, it's not great. I did some protected and virtual methods magic and cleaned it up a bit (in new commits).
Created attachment 201904 [details] [review] individual-store: expose some attributes and methods as 'protected' This will be needed when abstracting EmpathyIndividualStore.
Created attachment 201905 [details] [review] Abstract the individual store We now have EmpathyIndividualStoreManager which implements the store using EmpathyIndividualManager as its contact source.
Created attachment 201906 [details] [review] Add individual-store-channel This will allow us to use the individual view to display muc members.
Created attachment 201907 [details] [review] add empathy_individual_store_refresh_individual() as a protected method This allows us to remove show_active as a protected variable.
Created attachment 201908 [details] [review] add initial_loading() as a virtual method This allows us to remove setup_idle_id as a protected variable.
*** Bug 653217 has been marked as a duplicate of this bug. ***
Review of attachment 201907 [details] [review]: Looks much cleaner to me.
Review of attachment 201908 [details] [review]: Nice!
Review of attachment 201904 [details] [review]: Looks OK with the addition of attachment #201907 [details] and attachment #201908 [details].
Review of attachment 201905 [details] [review]: With attachment #201907 [details] to fix up the show_active stuff, this looks good to me. ::: libempathy-gtk/empathy-individual-store-manager.c @@ +285,3 @@ + PROP_INDIVIDUAL_MANAGER, + g_param_spec_object ("individual-manager", + "individual manager", s/individual/Individual/, if I'm allowed to be picky. (No need for another iteration of the patch, though.)
Review of attachment 201906 [details] [review]: Looks good to me. ::: libempathy-gtk/empathy-individual-store-channel.c @@ +293,3 @@ + PROP_CHANNEL, + g_param_spec_object ("individual-channel", + "individual channel", s/individual/Individual/ if I'm allowed to be picky. (Doesn't require another iteration of the patch, though. Just commit with this change.)
(In reply to comment #49) > Review of attachment 201539 [details] [review]: > > I've been told that this kind of cast aren't that fast because of GLib's type > system checks (but maybe that's a legend?). So I sometimes avoid to use them > when casting from a well known type (ie not from a gpointer). They do add an overhead, but IIRC there was some work to optimise this a few months ago. I think the official line is that the performance impact is negligible unless you're in performance-critical code. I'd prefer the added safety guarantees of using the macro type checks over normal casts, but I guess it's a matter of personal preference.
Review of attachment 201905 [details] [review]: ::: libempathy-gtk/empathy-individual-store-manager.c @@ +285,3 @@ + PROP_INDIVIDUAL_MANAGER, + g_param_spec_object ("individual-manager", + "individual manager", fixed.
Review of attachment 201906 [details] [review]: ::: libempathy-gtk/empathy-individual-store-channel.c @@ +293,3 @@ + PROP_CHANNEL, + g_param_spec_object ("individual-channel", + "individual channel", fixed.
Attachment 201536 [details] pushed as ef0d55e - factor out empathy_create_individual_from_tp_contact() Attachment 201537 [details] pushed as cbeb879 - add EMPATHY_INDIVIDUAL_FEATURE_ADD_CONTACT Attachment 201539 [details] pushed as bcc2359 - chat: use the individual view/store rather than the contact one Attachment 201540 [details] pushed as 1c51f65 - remove EmpathyContactManager's test Attachment 201541 [details] pushed as 4f470e2 - Remove obsolete contact-list-{store,view} Attachment 201904 [details] pushed as 0c7f25a - individual-store: expose some attributes and methods as 'protected' Attachment 201905 [details] pushed as edf8805 - Abstract the individual store Attachment 201906 [details] pushed as 2a03f6f - Add individual-store-channel Attachment 201907 [details] pushed as f13e0c1 - add empathy_individual_store_refresh_individual() as a protected method Attachment 201908 [details] pushed as 70ec5df - add initial_loading() as a virtual method