Bug 30088 - TpBaseProtocol: add Presence
Summary: TpBaseProtocol: add Presence
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://git.collabora.co.uk/?p=user/sm...
Whiteboard: review+
Keywords: patch
Depends on: 31102
Blocks: 31106 31107 31108
  Show dependency treegraph
 
Reported: 2010-09-08 06:37 UTC by Will Thompson
Modified: 2010-11-05 06:47 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Will Thompson 2010-09-08 06:37:37 UTC
These should be pretty easy.
Comment 1 Vivek Dasmohapatra 2010-10-05 06:59:56 UTC
splitting avatars off into a separate bug
Comment 3 Simon McVittie 2010-10-05 07:56:16 UTC
The telepathy-glib branch shouldn't be merged as-is, because it edits spec/, but this approach is OK as a trial implementation.

> +              /* assuming the string arg of a status is a message */

It'd be better to look for an argument called "message", like the mixin does.

I'm not sure whether non-core interfaces should be part of the TpBaseProtocolClass. One alternative would be to have a TpPresenceProtocol GInterface, a bit like TpBaseContactList? That might be overkill if we only ever need this one Presence method, though.

I think the Gabble implementation should include any plugin-defined statuses, because the Protocol statuses are defined to be a superset of all Connections' statuses. This might require a hook by which plugins can indicate which statuses they support.

> +  return g_strsplit (TP_IFACE_PROTOCOL_INTERFACE_PRESENCE, ";", -1);

This is a neat hack, but I think explicit is better than implicit: I'd prefer

    const gchar * const interfaces[] = {
        TP_IFACE_PROTOCOL_INTERFACE_PRESENCE,
        NULL };

    return g_strdupv ((GStrv) interfaces);
Comment 4 Vivek Dasmohapatra 2010-10-05 08:14:40 UTC
Ok, on most of thse - wrt the extra class - don't really know - will mull it over a bit more.
Comment 5 Vivek Dasmohapatra 2010-10-05 10:24:39 UTC
message flag now set only if status arg is both a string and called "message"

g_strdupv used instead of g_strsplit

statuses now exposed by a G_GNUC_INTERNAL helper that supplies the 
compound list from base + plugins
Comment 6 Simon McVittie 2010-10-14 09:35:04 UTC
Gabble doesn't pretend to be a library (unlike Mcd), so we don't need your last commit to that branch.

If you drop that one and wait for undrafted spec, the rest looks fine.

This is clearly implementable, so it no longer blocks the spec. Thanks!
Comment 7 Simon McVittie 2010-10-14 09:49:20 UTC
One thing this branch doesn't have is a client-side binding in TpProtocol, which would have to parse the statuses from .manager files (secretly part of TpConnectionManager if I remember correctly) and/or from D-Bus properties.

Straw man:

If supported, Presence is part of TP_PROTOCOL_FEATURE_CORE.

I don't know whether these method names should contain presence_ or not: it makes their meaning more explicit, but makes them more verbose.

/* requires CORE, returns NULL if Protocol.I.Presence is unsupported
 * or CORE isn't ready */
GStrv tp_protocol_dup_[presence_]status_names (TpProtocol *self);

/* returns FALSE without error for unrecognised names */
gboolean tp_protocol_can_set_[presence_]status (TpProtocol *self,
    const gchar *status_name);

/* returns FALSE without error for unrecognised names */
gboolean tp_protocol_[presence_]status_has_message (TpProtocol *self,
    const gchar *status_name);

/* perhaps this is useful to have? */
GStrv tp_protocol_dup_settable_[presence_]status_names (TpProtocol *self);
Comment 8 Simon McVittie 2010-10-25 12:25:23 UTC
The telepathy-glib branch needs rebasing onto / merging with / whatever the stuff from Bug #31102. Repurposing this bug for that; I'll clone it for Gabble and a client-side API.
Comment 9 Simon McVittie 2010-10-26 08:34:33 UTC
Branch stolen, updated to master and documented. (Please make sure check passes with gtk-doc enabled in future.)

Things I'm suspicious about here:

This makes TpBaseProtocol's API depend on TpPresenceStatusSpec, which is a bit crufty - it has an array of optional arguments from which we use only "message".

I wonder whether to turn one of the spare gpointer fields into a boolean indicating whether there's a message, which would make it less "this is a relic of a previous age, via TpPresenceMixin" and more SimplePresence'y.

Alternatively, we could introduce

TpNamedStatusSpec = struct {
    const gchar *name;
    TpConnectionPresenceType;
    gboolean settable;
    gboolean message;
    gpointer _padding[n];
}

and use that in both TpBaseProtocol and TpPresenceMixin?

Not a blocker for telepathy-glib 0.13.3, I don't think.
Comment 10 Simon McVittie 2010-11-04 11:20:45 UTC
(In reply to comment #9)
> I wonder whether to turn one of the spare gpointer fields into a boolean
> indicating whether there's a message, which would make it less "this is a relic
> of a previous age, via TpPresenceMixin" and more SimplePresence'y.

We can do that for 1.0; I think this API is fine for now.
Comment 11 Simon McVittie 2010-11-05 06:47:56 UTC
Fixed in git for 0.13.5


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.