Bug 52011 - TpBaseContactList announces Group channels twice
Summary: TpBaseContactList announces Group channels twice
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://cgit.freedesktop.org/~smcv/tel...
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2012-07-12 12:52 UTC by Jonny Lamb
Modified: 2013-06-10 14:03 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
patch (2.72 KB, patch)
2012-07-12 12:52 UTC, Jonny Lamb
Details | Splinter Review
tp_base_contact_list_set_list_received: don't re-announce groups (1.84 KB, patch)
2013-01-03 11:15 UTC, Simon McVittie
Details | Splinter Review
contact-lists test: add a regression test for fd.o #52011 (4.73 KB, patch)
2013-01-03 11:41 UTC, Simon McVittie
Details | Splinter Review

Description Jonny Lamb 2012-07-12 12:52:59 UTC
Created attachment 64139 [details] [review]
patch

This seems to have happened since forever?
Comment 1 Jonny Lamb 2012-07-12 12:54:30 UTC
I didn't mean to press submit that quickly.

So yeah, it announces $conn.object_path/Group/i channels twice because both groups_created and set_list_received emit NewChannels for them.

I've attached a patch which fixes the problem but it feels clunky. What do you think?
Comment 2 Simon McVittie 2012-07-18 18:14:59 UTC
(In reply to comment #1)
> So yeah, it announces $conn.object_path/Group/i channels twice because both
> groups_created and set_list_received emit NewChannels for them.

I was about to ask why set_list_received needs to emit NewChannels at all. It looks as though my goal was probably to signal the creation of the channels with their members already present, rather than signal them empty and then add all the members afterwards.

If this has never actually worked, because tp_base_contact_list_groups_created signals the channels' creation, then we might as well just drop the tp_base_contact_list_announce_channel calls from set_list_received?
Comment 3 Simon McVittie 2012-07-18 18:15:57 UTC
(Rationale for accepting this less-than-ideal behaviour: any legacy apps that are broken by it are already broken, and non-legacy apps shouldn't be using these channels anyway.)
Comment 4 Simon McVittie 2013-01-03 11:15:55 UTC
Created attachment 72438 [details] [review]
tp_base_contact_list_set_list_received: don't re-announce groups

We already announced each group from tp_base_contact_list_groups_created
a few lines ago; we don't need to do it again.
    
Ideally we'd add each channel's members before announcing the
channel itself, so that the channel is created "fully-formed"; but
we've never actually done that, and keeping the first NewChannels
instead of the second seems less likely to break applications.
These channels are only for legacy code anyway: any modern client
should be using the ContactGroups interface.

---

Here's my suggestion for how to fix this. Thoughts?

I've verified with dbus-monitor that test-contact-lists creates each group once; currently adding a regression test, just to make sure.
Comment 5 Simon McVittie 2013-01-03 11:41:13 UTC
Created attachment 72442 [details] [review]
contact-lists test: add a regression test for fd.o #52011
Comment 6 Xavier Claessens 2013-01-03 12:51:45 UTC
that looks correct to me. +1
Comment 7 Simon McVittie 2013-06-10 14:03:55 UTC
Fixed in git for 0.20.3, 0.21.1. Thanks!


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.