Bug 27687

Added both API calls.

Since the commit message explicitly says "WIP", I assume you just want API comments and don't actually consider this to be good to merge yet? As such, I mostly haven't reviewed the code yet, just the APIs.

The abuse of weak_object to pass through a strong reference is an interesting hack, but wouldn't it be better to just use the user_data argument? Then telepathy-glib wouldn't waste time setting up and tearing down a weak ref! (Also, you seem to leak the result at the end.)

> +void tp_connection_get_contacts_by_id_async (TpConnection *self,
> +    guint n_ids, const gchar * const *ids, guint n_features,
> +    const TpContactFeature *features, GAsyncReadyCallback callback,
> +    gpointer user_data);

If the idea of this method is to be more binding-friendly at the cost of being less C-friendly, perhaps the IDs should be NULL-terminated (so bindings can claim it's a "const GStrv"), or a GList?

I'm not sure how we'd represent lists of features or handles in a binding-happy way; I suppose the answer is that we wait for your g-i bug reports to be fixed, or fix them ourselves :-/

> + * @self: A connection, which must be ready (#TpConnection:connection-ready
> + *  must be %TRUE)

This should be documented in terms of the CORE and CONNECTED features in new code. Perhaps this method should even wait for CONNECTED automatically if necessary?

> + * @successful_requests: (out) (transfer none) (element-type utf8 TelepathyGLib.Contact): map of identifiers to successfully created #TpContact<!-- -->s

This line is huge, can you wrap it?

It would be good to say explicitly that the keys are what the caller asked for, not the normalized version (i.e. if you request "Fred.Bloggs" on a protocol where normalization includes lower-casing, you get back a map { "Fred.Bloggs": <a TpContact whose identifier is "fred.bloggs"> }.

> + * tp_connection_get_contacts_by_handle_async:

Following discussion with sjokkis on IRC: we should probably also have a get_contacts_by_handle_set_async() that takes a TpIntSet, for use with, e.g., members-changed.

(Implementation detail: it would temporarily convert it to a GArray using existing API, pass the GArray's contents to get_contacts_by_handle_async, then free the GArray.)

So I've wondered whether a GArray and GStrv would be more useful. Than static arrays, since members-changed gives you a GArray, and it's easy to get a GArray from TpIntSet (of course in C it's easy to pass a GArray as the array, as well).

I'm kind of wondering whether that's required though, or really we should just look into trying to fix gjs/convince someone to fix gjs.

(In reply to comment #4)
> So I've wondered whether a GArray and GStrv would be more useful. Than static
> arrays

GArray -> (array,len) is trivial in C - I expect about 50% of callers will actually be using a GArray, but using ((TpHandle *) arr->pdata, arr->len) is almost as easy, and also allows for non-GArrays.

GStrv -> (array,len) is also trivial in C, because g_strv_length() exists; I consider it to be a significant advantage that the same API is useful for NULL-terminated and non-NULL-terminated arrays, and in particular that non-NULL-terminated arrays don't have to be copied in order to call the function.

Adding a shim around the current methods to have better bindability might be worth it if it's a common problem throughout g-i; I don't think it's right if it's only a gjs limitation.

> I'm kind of wondering whether that's required though, or really we should just
> look into trying to fix gjs/convince someone to fix gjs.

We shouldn't add API just to work around gjs' limitations (particularly if it works fine in other g-i languages, e.g. pygi).

Xavier would like single-contact versions too; see Bug #28044.

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 a pain to create a GArray just for one handle, but it's easy to pass (1, &handle). Same for by_id variant, (1, &id) is easy, but if you need it to be NULL-terminated it's a pain.

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

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.

About TpContactFeature, one issue I see when thinking about droping the EmpathyTpContactFactory wrapper is the set of features we want on all TpContact created inside empathy. Atm we have a wrapper function around tp_connection_get_contacts_by_handle/id() that doesn't take a feature set in its args, but always gives the same feature set (actually all features) to tp_connection_get_contacts_*().

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. I understand flags are dangerous in case we get more than 31 features (can it really happen? implementation actually already uses flags).

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

*** Bug 28044 has been marked as a duplicate of this bug. ***

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

I opened bug #29527 which is about being able to ask for preparation of TpConnection features when requesting TpContact's. That's probably something we should think about if we redesign this API.

(Dropping this from the review queue since it's WIP.)

*** Bug 49370 has been marked as a duplicate of this bug. ***

Note that lots of this bug changed since then:
 - We have TpSimpleClientFactory to define the features we want
 - We have immortal handles and spec gives handle+id pairs ~everywhere
 - high-level objects already gives prepared TpContact objects instead of TpHandle.

Bug #49370 is a dup, but with a more modern description IMO:


Atm, to get a TpContact we have those functions:

tp_connection_get_contacts_by_handle():
 - I think CONTACT TpHandle must totally disappear from our API, and this
function should be killed. I think in general high-level APIs should expose
TpContact objects. At least spec should always give handle+id and so the
recommended way to get a TpContact is tp_client_factory_ensure_contact() +
eventually upgrade. Note that 'next' already make immortal handles mandatory.

tp_connection_get_contacts_by_id():
 - It should be proper _async().
 - Should use GetContactAttributesByID (bug #30874) for massive simplification
and single round-trip.

tp_connection_upgrade_contacts():
 - It should be proper _async().

tp_connection_dup_contact_if_possible():
 - Since we are making immortal handles mandatory, it is always possible to dup
a contact with handle+id. In that case just use
tp_client_factory_ensure_contact()
 - As said above, if you don't have id+handle, you should just never get a
TpContact.
 - IIRC there are legitimate uses internally, when the spec gives only an
handle but we are supposed to already have the TpContact (like removed members
in group). So I suggest keeping some form of this but internal only.

One thing I would like is to ensure that at any moment, even internally, a
TpContact has both id+handle. Making those 2 construct-only mandatory
properties.

(In reply to comment #13)
> tp_connection_get_contacts_by_handle():
>  - I think CONTACT TpHandle must totally disappear from our API, and this
> function should be killed. I think in general high-level APIs should expose
> TpContact objects. At least spec should always give handle+id and so the
> recommended way to get a TpContact is tp_client_factory_ensure_contact() +
> eventually upgrade. Note that 'next' already make immortal handles mandatory.

Agreed, let's drop it.

> tp_connection_get_contacts_by_id():
>  - It should be proper _async().
>  - Should use GetContactAttributesByID (bug #30874) for massive simplification
> and single round-trip.

Actually wouldn't it be easier to have it as a method on the *factory* instead?
The factory acts as the de-facto place where all the features we care about are stored so that could make things easier.

Something like:

void tp_simple_client_factory_ensure_contact_by_id_async (TpSimpleClientFactory *factory,
    TpConnection *connection,
    const gchar *id,
    GAsyncReadyCallback callback,
    gpointer user_data);

It also fix bug #30874 has a patch doing all that. More cleanup will be possible in 'next'.

Fixed with the branch for bug #30874