[Bug 27687] new GIO-style async methods for requesting TpContacts

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27687

--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-10 09:53:00 PDT ---
(In reply to comment #7)
> IIRC the reason we had (guint n_handles, TpHandle *handles) and not a GArray,
> is to make it slightly easier for single-contact request.

It's also because it's very nearly as easy to pass (arr->len, arr) as it is to
pass arr, but if you have a non-GArray array of handles, it's a pain to copy it
into a GArray just to pass it as an argument.

> Now if we consider having dedicated API for the single-contact request, I don't
> think we have any reason anymore to not take a GArray of TpHandle or a
> NULL-terminated GStrv. Also it looks even better to take a TpIntSet since the
> multi-contact request almost certainly comes just after a
> tp_channel_group_get_members().

Bindings don't actually care what we do, as long as it's something that can be
annotated in g-i, because the binding user will give us a Python list of Python
strings or whatever, and the binding will copy it into whatever we told them to
use. GArrays and 0-terminated quark arrays doesn't actually work in gjs due to
feature omissions, but Danielle's working on that.

So, I think we only care about C/C++ users, and perhaps also binding users
who're passing a TpIntSet.

Comparing how it would look to pass various things in the current API:

* GArray *arr => (arr->len, arr)
* 0-terminated array of handles => count with a loop, then use (len, arr)
* C array and length => trivial 1:1
* TpIntSet => copy to an intermediate GArray

* GStrv strv => (g_strv_length (strv), strv) (even works for NULL!)
* GPtrArray *pa => (pa->len, (gchar **) pa->pdata) (a bit irritating)
* C array of gchar * and its length => trivial 1:1
* GList of gchar * => copy to an intermediate GPtrArray with a loop

and your proposed API with GArray<TpHandle> and GStrv:

* GArray *arr => trivial 1:1
* 0-terminated array of handles => count with a loop, copy to a GArray
* Array and length => copy to a GArray
* TpIntSet => copy to a GArray

* GStrv strv => trivial 1:1
* GPtrArray *pa => copy and append NULL (can skip the copy if we own it)
* C array of gchar * and its length => copy and append NULL
* GList of gchar * => copy to GPtrArray with a loop, append NULL

So if (length, array) can be made to bind nicely (= ordinary list) in g-i, I
still think it's the most versatile "C binding". We should probably have a shim
around the basic async API to use TpIntSet, though.

> Note about single-contact request API: One benefit is to have the same
> _finish() function for both by_handle and _by_id variants. Like that we can use
> the same callback in both cases. After all, it's just about giving a single
> GError and TpContact, in both cases.

That's not the GIO convention; I'm not necessarily saying that it's wrong, but
it's certainly unconventional.

(Presumably, to be nice to bindings, this would be implemented by having two
finish functions, one of which is a thin shim around the other, and documenting
the fact that they are interchangeable.)

It's also not necessarily appropriate, I don't think. A request starting from a
handle can be context-free (on success, the TpContact has that handle; on
failure, what can you do about it anyway?), but for a request starting from a
(possibly invalid) ID, you probably want to tell the caller the ID they started
from, so they won't have to push it through the user_data?

> So if TpContactFeature were falgs, I could just #define it somewhere and give
> it everywhere I have a tp_connection_get_contacts_*(). The (guint n_feature,
> TpContactFeature *features) way is a bit annoying IMO, we have to define a
> feature array each time we need to request a contact.

extern TpContactFeature *empathy_usual_features;
#define N_EMPATHY_USUAL_FEATURES 7

or even add:

#define EMPATHY_USUAL_FEATURES \
    7, empathy_usual_features

Empathy can do this now, because it's a monolithic process, and in practice it
wants every currently-defined feature, but as we add more features and break
Empathy up into smaller components, I think it'll become less appropriate to do
this. I don't think Empathy-the-chatroom-UI should be asking for Capabilities
or "user tune", and perhaps it shouldn't even ask for Avatars or Location,
whereas Empathy-the-contact-list wants all of those.

> I understand flags are
> dangerous in case we get more than 31 features (can it really happen?
> implementation actually already uses flags).

The point of this API is to *allow for* more than 32 features; the
implementation currently uses flags because that's easy, but avoids exposing
them into the API so that if we need to "pay the price" and expand to more than
32 later, we can do that without breaking ABI.

As for number of features: alias, avatar token, presence, geolocation,
capabilities, contact info, user tune, user activity (OLPC-style), mood, "am I
mobile or not?", avatar data. That's 11 already, even assuming that "alias" is
re-interpreted to incorporate the Names interface from Bug #14540.

> Note that for consistency, we could use 0-terminated GQuark array.

With hindsight, I agree, and we should indeed switch to quark arrays when we
break ABI, but until then we should keep the current setup. The API would be
very confusing if there were two incompatible sets of TpContact features, both
small integers (=> passing values from one namespace where the other is
expected can't be caught by the compiler).

We use GQuark arrays in the other classes because we have to use (something
isomorphic to) strings, so we have namespaces for subclassability - a
TpConnection subclass, say SugarTpConnection, could define
SUGAR_TP_CONNECTION_FEATURE_XO_COLOUR (which would expand to the quark for
"sugar-tp-connection-feature-xo-colour" or some such).

TpContact isn't subclassable, so we control its features and it doesn't have
this requirement; unfortunately, it was designed first, so it wasn't clear that
it needed to be consistent with the stricter requirements of TpProxy features.

In telepathy-qt4, features are a small class that encapsulates a pair (Qt type
ID, class-specific integer) - the GObject equivalent would be a struct
containing GType and class-specific integer. I think that's only correct
because C++ gives us good syntactic sugar for it, though - quarks are a better
compromise in C.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.