Bug 31729

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

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.

(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

(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.

(In reply to comment #3)
> Branch updated

Looks good now, thanks for picking this up!

Cool, merged.

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.

(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

(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.