Bug 31729 - use TpBaseContactList in Salut
Summary: use TpBaseContactList in Salut
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: salut (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Xavier Claessens
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/xc...
Whiteboard: review+
Keywords: patch
Depends on: 31728
Blocks: 41357
  Show dependency treegraph
 
Reported: 2010-11-18 06:06 UTC by Simon McVittie
Modified: 2012-03-15 12:54 UTC (History)
1 user (show)

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 Simon McVittie 2010-11-18 06:06:17 UTC
Salut should use TpBaseContactList for the contact list. Blocked by Bug #31728.
Comment 1 Xavier Claessens 2011-09-29 03:50:32 UTC
This is really needed now that tp-glib client-side uses ContactList spec.

The only issue I've observed, is empathy let user to modify the contact groups, but that's not salut's fault.

Here it is:
http://cgit.collabora.com/git/user/xclaesse/telepathy-salut.git/log/?h=contact-list
Comment 2 Simon McVittie 2011-09-29 04:17:04 UTC
I'm not really keen on "priv->roster" as the name of the set of contacts: the roster is an XMPP construct that link-local XMPP explicitly doesn't use, and someone carefully avoided calling this object SalutRoster. priv->contacts? priv->nearby?

> +static gboolean
> +contact_manager_get_contact_list_persists (TpBaseContactList *self)
> +{
> + return FALSE;

You could use tp_base_contact_list_false_func instead of reimplementing it.

> + base_connection = tp_base_contact_list_get_connection (
> + (TpBaseContactList *) self, NULL);
> + self->connection = SALUT_CONNECTION (base_connection);

Borrowed reference. Can you ref it, and release the ref on disconnection? (It might not be safe to do so - it isn't in Gabble, due to internal assumptions that it's safe to use the Roster right until the bitter end - in which case you should comment it instead.)

> + /* Not really already received, but we cannot know when initial set
> + * is fetched */

The Avahi backend, at least, ought to be able to signal received when Avahi says ALL_FOR_NOW?

> + /* No variant taking a GError*, really? */
> + tp_base_contact_list_set_list_failed ((TpBaseContactList *) self,
> + err->domain, err->code, err->message);

Add one to telepathy-glib taking a const GError * if you really want to, but I don't think this is really any harder to use.
Comment 3 Xavier Claessens 2011-10-03 08:17:12 UTC
(In reply to comment #2)
> I'm not really keen on "priv->roster" as the name of the set of contacts: the
> roster is an XMPP construct that link-local XMPP explicitly doesn't use, and
> someone carefully avoided calling this object SalutRoster. priv->contacts?
> priv->nearby?

there is already manager->contacts, I'll just call it priv->handles, which is what it is...

> > +static gboolean
> > +contact_manager_get_contact_list_persists (TpBaseContactList *self)
> > +{
> > + return FALSE;
> 
> You could use tp_base_contact_list_false_func instead of reimplementing it.

good, didn't see that one.

> > + base_connection = tp_base_contact_list_get_connection (
> > + (TpBaseContactList *) self, NULL);
> > + self->connection = SALUT_CONNECTION (base_connection);
> 
> Borrowed reference. Can you ref it, and release the ref on disconnection? (It
> might not be safe to do so - it isn't in Gabble, due to internal assumptions
> that it's safe to use the Roster right until the bitter end - in which case you
> should comment it instead.)

TpBaseContactList already keeps a strong ref until status goes to DISCONNECTED if I understand correctly, so I could do the same in the subclass.

Note that this is not different with my patch, it is just that new the property in in the base class instead of in SalutContactManager.

> > + /* Not really already received, but we cannot know when initial set
> > + * is fetched */
> 
> The Avahi backend, at least, ought to be able to signal received when Avahi
> says ALL_FOR_NOW?

I've no idea.

> > + /* No variant taking a GError*, really? */
> > + tp_base_contact_list_set_list_failed ((TpBaseContactList *) self,
> > + err->domain, err->code, err->message);
> 
> Add one to telepathy-glib taking a const GError * if you really want to, but I
> don't think this is really any harder to use.

doesn't matter tbh. removed the comment :)



Branch updated
Comment 4 Xavier Claessens 2011-10-03 08:49:40 UTC
(In reply to comment #2)
> > + /* Not really already received, but we cannot know when initial set
> > + * is fetched */
> 
> The Avahi backend, at least, ought to be able to signal received when Avahi
> says ALL_FOR_NOW?

Ok, changed to propagate that signal and call tp_base_contact_list_set_list_received() only when avahi says it's all for now. If we ever have other backends, they'll have to emit the signal after a timeout probably.
Comment 5 Simon McVittie 2011-10-05 04:45:23 UTC
(In reply to comment #3)
> Branch updated

Looks good now, thanks for picking this up!
Comment 6 Xavier Claessens 2011-10-05 04:55:39 UTC
Cool, merged.
Comment 7 Olli Salli 2012-03-15 04:41:43 UTC
Umm...

http://cgit.freedesktop.org/telepathy/telepathy-salut/tree/src/connection.c#n724

doesn't include TP_CONNECTION_INTERFACE_CONTACT_LIST, so nothing will actually use the ContactList implementation enabled here. This is evident by running the tp-glib contact list example client, which requires this interface:

http://paste.debian.net/159801/

We'll try adding the CONTACT_LIST interface to the announced ones - expect a report in how that fared shortly. That'd mean completely unexercised code works out of the box though so I'm not really holding my breath for that, but it's a start.
Comment 8 Siraj Razick 2012-03-15 12:48:10 UTC
(In reply to comment #7)
> Umm...
> 
> http://cgit.freedesktop.org/telepathy/telepathy-salut/tree/src/connection.c#n724
> 
> doesn't include TP_CONNECTION_INTERFACE_CONTACT_LIST, so nothing will actually
> use the ContactList implementation enabled here. This is evident by running the
> tp-glib contact list example client, which requires this interface:
> 
> http://paste.debian.net/159801/
> 
> We'll try adding the CONTACT_LIST interface to the announced ones - expect a
> report in how that fared shortly. That'd mean completely unexercised code works
> out of the box though so I'm not really holding my breath for that, but it's a
> start.

I was able to get it working with the following one line patch

http://cgit.collabora.com/git/user/siraj/telepathy-salut.git/commit/?h=contact-list-fix&id=b2523f75fda1d6620f03a39329fa8945ce4f379c
Comment 9 Olli Salli 2012-03-15 12:54:08 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Umm...
> > 
> > http://cgit.freedesktop.org/telepathy/telepathy-salut/tree/src/connection.c#n724
> > 
> > doesn't include TP_CONNECTION_INTERFACE_CONTACT_LIST, so nothing will actually
> > use the ContactList implementation enabled here. This is evident by running the
> > tp-glib contact list example client, which requires this interface:
> > 
> > http://paste.debian.net/159801/
> > 
> > We'll try adding the CONTACT_LIST interface to the announced ones - expect a
> > report in how that fared shortly. That'd mean completely unexercised code works
> > out of the box though so I'm not really holding my breath for that, but it's a
> > start.
> 
> I was able to get it working with the following one line patch
> 
> http://cgit.collabora.com/git/user/siraj/telepathy-salut.git/commit/?h=contact-list-fix&id=b2523f75fda1d6620f03a39329fa8945ce4f379c

Woo!

Merged.

Against some heavy odds, I suppose this is OK again.


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.