Bug 40933 - Too many MembersChanged(Detailed) signals when connecting
Summary: Too many MembersChanged(Detailed) signals when connecting
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Marco Barisione
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/ba...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-09-16 03:49 UTC by Marco Barisione
Modified: 2011-09-16 13:35 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Barisione 2011-09-16 03:49:32 UTC
Currently telepathy-glib emits one MembersChanged signal and one MembersChangedDetailed signal per contact in the subscribe list.
Comment 1 Marco Barisione 2011-09-16 03:53:56 UTC
This branch fixes the problem by distinguishing when a contact appears in our contact list because it's already there when we connect (so we group the contacts and emit the signal with actor=0) and when contacts appear later because they accepted our subscription request.

Another possible solution is to not emit the signal at all when connecting. I looked into that, but I think that the fix looks more like a hack with little gain (emitting one MembersChanged and one MembersChangedDetailed signal is not a problem).
Comment 2 Simon McVittie 2011-09-16 04:09:51 UTC
> + tp_group_mixin_change_members (sub_chan, "", sub, NULL, NULL, NULL, 100,
> + TP_CHANNEL_GROUP_CHANGE_REASON_NONE);

s/100/0/ (the actor)

> + if (interface == TP_IFACE_QUARK_CHANNEL_INTERFACE_GROUP &&
> + msg_type == DBUS_MESSAGE_TYPE_SIGNAL &&
> + g_strcmp0 (member, "MembersChanged") == 0)

dbus_message_is_signal() is ideal for low-level filters like this

Other than that, looks good to me.
Comment 3 Will Thompson 2011-09-16 04:15:41 UTC
+  tp_group_mixin_change_members (sub_chan, "", sub, NULL, NULL, NULL, 100,

Actor 100? :O

The rest of this is just nit-picking

+  closure = g_slice_new0 (MembersChangedClosure);

You don't need to slice-allocate the struct, you could just stick it on the stack. I don't really care though.

+      g_assert (lookup_contact (self, members[i]) == NULL);

g_assert_cmpstr for future reference. :)

+  /* Now put it online and wait for contact list state move to success */

“wait for the contact list state to move”
Comment 4 Marco Barisione 2011-09-16 13:35:30 UTC
100 was an interesting typo :-O

> +      g_assert (lookup_contact (self, members[i]) == NULL);
> 
> g_assert_cmpstr for future reference. :)

Er... it's not a string :)

I fixed the things you suggested to change and pushed to master.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.