Bug 71508

I started simplifying a bit on Bug #50093, which is unreviewed.

What's left to do here?

What I said still stands.

It might well be easier to do this as an API break on next than as a graceful compatible thing on master.

(In reply to comment #0)
> Now that we have SimplePresence:
> 
> TpPresenceStatusOptionalArgumentSpec should go away altogether.
> 
> TpPresenceStatusSpec should replace
> 
>     gboolean self;
>     const TpPresenceStatusOptionalArgumentSpec *optional_arguments;
> 
> with
> 
>     typedef enum {
>         /*< flags >*/
>         TP_PRESENCE_STATUS_SPEC_FLAGS_NONE = 0,
>         TP_PRESENCE_STATUS_SPEC_FLAGS_CAN_SET_ON_SELF = (1 << 1),
>         TP_PRESENCE_STATUS_SPEC_FLAGS_HAS_MESSAGE = (1 << 2)
>     } TpPresenceStatusSpecFlags;
> 
>     TpPresenceStatusSpecFlags flags;
> 
> (or possibly a pair of gbooleans but I think it's clearer as a flags word).

The pair-of-booleans is implemented now, but I still think a flags-word would be better.

> The vfuncs in TpPresenceMixinClass should move to a GInterface that the
> Connection implements (TpPresenceMixable?), to allow for introspection in
> future.

Xavier and I came up with a better idea on Bug #77189: TpPresenceMixin[Interface] is a GInterface, storing data on the connection with qdata, so we only need init() and fill_contact_attributes().

> or maybe even
> 
> TpPresenceStatus *tp_presence_status_new (TpPresenceMixable *self,
>     const TpPresenceStatusSpec *spec,
>     const gchar *message) G_GNUC_WARN_UNUSED_RESULT;
> 
> TpPresenceMixinStatusAvailableFunc should take a const TpPresenceStatusSpec
> * too.

I still like this idea, but making it introspectable leads to some annoying boilerplate in CMs, so we might need more thought.

I'm half tempted to turn TpPresenceStatusSpec into a GObject.

Work in progress (single commit): http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=wip-presence-mixin

Created attachment 97765 [details] [review]
[work in progress] TpPresenceMixin: redo the API to be introspectable

TpPresenceStatusSpec and TpPresenceStatus are now opaque, refcounted
structures, with accessors for everything. Functions that return them
return a GList with (transfer full); this does lead to some irritating
boilerplate in the CMs, unfortunately.

----

I have only attempted to port the examples, not any of the out-of-tree CMs.

I only read the proposed API so far, and it looks great. Tried to compile your patch on top of my gdbus-all branch to give it a try in my JS CM, but the gi scanner is not happy:

<unknown>:: Warning: TelepathyGLib: Deprecated reference to identifier prefix Tp in GIName Tp.PresenceStatusSpec

also missing a transfer annotation on tp_base_protocol_dup_statuses().

I think we can get one step further and remove tp_presence_status_spec_get_id(). That whole index stuff has always been weird for me. I don't think it is required any more now that TpPresenceStatus refs its TpPresenceStatusSpec.

I don't know how other CMs implement that, but looking at our fake contacts-conn.c, it has 2 hashtables:


presence_statuses: TpHandle -> index of a TpPresenceStatusSpec
presence_messages: TpHandle -> message string

Merging those is exactly what TpPresenceStatus is made for. It would only have one table TpHandle->TpPresenceStatus.

Similarly tp_tests_contacts_connection_change_presences() takes an index and a message which is exactly the same as taking a single TpPresenceStatus.

I think CMs can keep a index->spec mapping to make it easier, but that's an internal implementation detail that should not leak into telepathy-glib API, IMHO.

(In reply to comment #8)
> I think we can get one step further and remove
> tp_presence_status_spec_get_id().

It's basically there because the example C CMs, and the real C CMs, do everything in terms of an enum. It's easier to say

    contact->presence = EXAMPLE_PRESENCE_STATUS_AVAILABLE;

than

    contact->presence = example_protocol_get_presences ()[EXAMPLE_PRESENCE_STATUS_AVAILABLE];

and it has less boilerplate and less diffstat than:

    contact->presence = example_protocol_get_presence_available ();

I'm tempted to make TpPresenceStatusSpec be a GObject, so it can have qdata or a CM-specific subclass or something, and then CMs can do whatever they want to do. The index is basically acting as "poor man's qdata" here.

> I don't know how other CMs implement that, but looking at our fake
> contacts-conn.c, it has 2 hashtables:
> 
> presence_statuses: TpHandle -> index of a TpPresenceStatusSpec
> presence_messages: TpHandle -> message string
> 
> Merging those is exactly what TpPresenceStatus is made for.

I sort of agree, but please look at some real CMs before being too sure about that. For instance, Haze's internal data structure is a PurpleContact or something, and Gabble and Salut's internal data structures ought to be some sort of WockyContact really.

In recent mixins I have been leaning towards making the obvious path be "the mixin is a 'view' of the real data, which is in CM-specific structures" because the better your IM protocol library is, the closer to that you're going to be.

Included your patch in my gdbus-all branch, added a few fixes, and my JS ConnectionManager now support presence! So that API is really introspectable.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/118.