Bug 50341 - Special trick to add a contact in WLM
Special trick to add a contact in WLM
Status: RESOLVED FIXED
Product: Telepathy
Classification: Unclassified
Component: gabble
unspecified
Other All
: medium normal
Assigned To: Telepathy bugs list
Telepathy bugs list
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 01:43 UTC by Xavier Claessens
Modified: 2012-06-28 06:44 UTC (History)
1 user (show)

See Also:


Attachments
WIP: TpBaseConnection: add a contact id converter function (10.77 KB, patch)
2012-05-28 01:29 UTC, Xavier Claessens
Details | Splinter Review
WIP: Implement WLM jidlookup (4.62 KB, patch)
2012-05-28 01:32 UTC, Xavier Claessens
Details | Splinter Review
WIP: Implement WLM jidlookup (5.68 KB, patch)
2012-05-28 10:54 UTC, Xavier Claessens
Details | Splinter Review
WIP: add tp_handle_ensure_async() (15.74 KB, patch)
2012-05-28 10:55 UTC, Xavier Claessens
Details | Splinter Review
TpHandleRepoIface: Use G_DEFINE_INTERFACE (2.96 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
TpHandleRepoIface: add tp_handle_ensure_async() (7.04 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
TpDynamicHandleRepo: factor out ensure_handle_take_normalized_id() (2.59 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
TpHandleRepoIface: expose internally _tp_handle_repo_default_ensure_handle_async() (1.52 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
TpDynamicHandleRepo: Support async normalization function (7.07 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
TpContactsMixin: use tp_handle_ensure_async() in GetContactByID (3.70 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
WIP: TpBaseConnection: use tp_handle_ensure_async() in RequestHandles (1.91 KB, patch)
2012-05-29 04:35 UTC, Xavier Claessens
Details | Splinter Review
Implement WLM jidlookup (5.82 KB, patch)
2012-05-29 04:42 UTC, Xavier Claessens
Details | Splinter Review
TpBaseConnection: use tp_handle_ensure_async() in RequestHandles (4.60 KB, patch)
2012-05-31 02:58 UTC, Xavier Claessens
Details | Splinter Review
TpBaseConnection: use tp_handle_ensure_async() in RequestHandles (4.75 KB, patch)
2012-06-01 02:52 UTC, Xavier Claessens
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Xavier Claessens 2012-05-25 01:43:13 UTC
We can't add user@live.com in MSN roster, we have to first "convert" that live ID to an XMPP jid in the form 1234@messenger.live.com. That conversion is done by non-standard stanza:

< <iq from='john@contoso.com' to='mary@contoso.com' type='get' id='id1'>
    <getjid xmlns='jidlookup'></getjid>
</iq>

> <iq from='john@contoso.com' to='mary@contoso.com' type='result' id='id1'>
    <getjid xmlns='jidlookup'>
        <jid>12345@messenger.live.com</jid>
    </getjid>
</iq>

As described in http://msdn.microsoft.com/en-us/library/live/hh550849.aspx

This could require spec changes as well?
Comment 1 Jonny Lamb 2012-05-25 02:04:13 UTC
Or perhaps we'll need to start allowing asynchronous handle normalization functions?
Comment 2 Xavier Claessens 2012-05-25 02:06:24 UTC
(In reply to comment #1)
> Or perhaps we'll need to start allowing asynchronous handle normalization
> functions?

+1
Comment 3 Xavier Claessens 2012-05-25 02:23:33 UTC
Thinking a bit about this, maybe the trick should be added only in GetContactByID, since that's the only place where we get an id from the user. There is also RequestHandles, but I already opened a bug to drop that in 'next'.
Comment 4 Xavier Claessens 2012-05-25 02:31:02 UTC
So what I can suggest is adding tp_handle_ensure_async() in TpHandleRepoIface. TpBaseConnection will use that in RequestHandles, and contacts-mixin will use that in GetContactByID. Basically it will be documented to be used by any user-provided ID, not needed for ids coming from the server.

TpDynamic/StaticHandleRepo will have a default impl of ensure_handle_async() that just use tp_handle_ensure() and return in idle.

Then gabble will override that implementation, if the connection has the jidlookup caps (yes MS server announce a special caps for that) then it will do the <getjid> dance, otherwise it just use default implementation.

Does that plan sounds sane?
Comment 5 Xavier Claessens 2012-05-25 07:48:30 UTC
Hm, there is also TARGET_ID in channel requests could need that trick as well.
Comment 6 Xavier Claessens 2012-05-28 01:29:51 UTC
Created attachment 62154 [details] [review]
WIP: TpBaseConnection: add a contact id converter function

It is used in RequestHandles and GetContactByID to let CM do
server roundtrip normalization/conversion of the ID.

This is used by WLM server to convert email addresses to XMPP
jid.
Comment 7 Xavier Claessens 2012-05-28 01:32:50 UTC
Created attachment 62155 [details] [review]
WIP: Implement WLM jidlookup

See description: http://msdn.microsoft.com/en-us/library/live/hh550849.aspx
Comment 8 Xavier Claessens 2012-05-28 01:33:53 UTC
With those 2 dirty patches, I can add contacts in my WLM account using empathy.

I would appreciate advices how to do this properly.
Comment 9 Jonny Lamb 2012-05-28 06:32:41 UTC
I preferred the idea of adding the following two new vfuncs to TpHandleRepoIfaceClass:

 1. lookup_handle_async
 2. ensure_handle_async

If adding this to TpDynamicHandleRepo is a bit annoying we could subclass it in Gabble to add the feature there perhaps? Possibly just adding it to tp-glib itself will be less work. What do you think?
Comment 10 Xavier Claessens 2012-05-28 10:54:57 UTC
Created attachment 62173 [details] [review]
WIP: Implement WLM jidlookup

See description: http://msdn.microsoft.com/en-us/library/live/hh550849.aspx
Comment 11 Xavier Claessens 2012-05-28 10:55:22 UTC
Created attachment 62174 [details] [review]
WIP: add tp_handle_ensure_async()

It is used in RequestHandles and GetContactByID to let CM do
server roundtrip normalization/conversion of the ID.

This is used by WLM server to convert email addresses to XMPP
jid.
Comment 12 Jonny Lamb 2012-05-28 11:36:59 UTC
Comment on attachment 62174 [details] [review]
WIP: add tp_handle_ensure_async()

Review of attachment 62174 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/base-connection.c
@@ +2587,4 @@
>        goto out;
>      }
>  
> +  /* FIXME: Quick And Dirty hack */

I assume this is just because you can't be bothered doing a queueing system right now, right?

::: telepathy-glib/handle-repo.c
@@ +41,2 @@
>  
> +G_DEFINE_INTERFACE (TpHandleRepoIface, tp_handle_repo_iface, G_TYPE_OBJECT);

You should put this in another patch; it just clutters up this one.

@@ +312,5 @@
> + * @callback: a callback to call when the operation finishes
> + * @user_data: data to pass to @callback
> + *
> + * Asyncronously normalize an identifier and create an handle for it. This could
> + * involve server round-trip. This is to be prefered over tp_handle_ensure() for

*a* server round-trip

"This is preferred". Actually shouldn't this be something like "This should be used instead of tp_handle_ensure() for user provided contact identifiers, but it is not necessary for identifiers from the server" ?

@@ +314,5 @@
> + *
> + * Asyncronously normalize an identifier and create an handle for it. This could
> + * involve server round-trip. This is to be prefered over tp_handle_ensure() for
> + * user provided contact identifiers, but not necessary for identifiers coming
> + * from server.

from *the* server

@@ +326,5 @@
> +    gpointer context,
> +    GAsyncReadyCallback callback,
> +    gpointer user_data)
> +{
> +  return TP_HANDLE_REPO_IFACE_GET_CLASS (self)->ensure_handle_async (self,

We should probably check this function is implemented and fallback somehow if not? Oh, or does your default implementation below guarantee this is always non-NULL?

@@ +347,5 @@
> +tp_handle_ensure_finish (TpHandleRepoIface *self,
> +    GAsyncResult *result,
> +    GError **error)
> +{
> +  return TP_HANDLE_REPO_IFACE_GET_CLASS (self)->ensure_handle_finish (self,

Same thing about checking here.

@@ +392,5 @@
> + *
> + * Since: 0.UNRELEASED
> + */
> +void
> +tp_handle_repo_iface_implement_ensure_handle_async (TpHandleRepoIface *self,

I don't understand this? Why don't we just make handle repos override these in their class inits?

@@ +519,5 @@
> +{
> +  GSimpleAsyncResult *simple = (GSimpleAsyncResult *) result;
> +
> +  g_return_val_if_fail (g_simple_async_result_is_valid (result,
> +      G_OBJECT (self), NULL), 0);

I was going to say "we can check the source tag here?" but this is so implementations don't need to implement _finish, right? If so, yes, good.
Comment 13 Jonny Lamb 2012-05-28 11:40:22 UTC
Comment on attachment 62173 [details] [review]
WIP: Implement WLM jidlookup

Review of attachment 62173 [details] [review]:
-----------------------------------------------------------------

::: src/connection.c
@@ +3025,5 @@
> +    {
> +      TpHandleRepoIface *contact_repo = tp_base_connection_get_handles (base,
> +          TP_HANDLE_TYPE_CONTACT);
> +
> +      tp_handle_repo_iface_implement_ensure_handle_async (contact_repo,

Hm, I guess this is why you want the extra function instead of specifying the async normalize function immediately.
Comment 14 Jonny Lamb 2012-05-28 11:43:15 UTC
This looks good. One pretty important thing this is missing is caching. We must only do the getjid dance once for every contact as it's quite expensive.

Let's assume a foo@live.com gets normalized to bar@messenger.live.com. If you try to normalize bar@messenger.live.com then, yeah, just return it as is and cache that. If you try to normalize foo@live.com, do the getjid dance and save both foo@live.com and bar@messenger.live.com as the same handle.

I guess with caching it means this stuff will have to move into TpDynamicHandleRepo itself though, no?

Does any of this make sense?
Comment 15 Xavier Claessens 2012-05-29 01:30:50 UTC
(In reply to comment #12)
> Comment on attachment 62174 [details] [review] [review]
> WIP: add tp_handle_ensure_async()
> 
> Review of attachment 62174 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-glib/base-connection.c
> @@ +2587,4 @@
> >        goto out;
> >      }
> >  
> > +  /* FIXME: Quick And Dirty hack */
> 
> I assume this is just because you can't be bothered doing a queueing system
> right now, right?

Exactly, it's a bit tricky to do with multiple ids since ordering is important. I'll fix that when we agree on the idea :)

> ::: telepathy-glib/handle-repo.c
> @@ +41,2 @@
> >  
> > +G_DEFINE_INTERFACE (TpHandleRepoIface, tp_handle_repo_iface, G_TYPE_OBJECT);
> 
> You should put this in another patch; it just clutters up this one.

Right, splitted.

> @@ +312,5 @@
> > + * @callback: a callback to call when the operation finishes
> > + * @user_data: data to pass to @callback
> > + *
> > + * Asyncronously normalize an identifier and create an handle for it. This could
> > + * involve server round-trip. This is to be prefered over tp_handle_ensure() for
> 
> *a* server round-trip
> 
> "This is preferred". Actually shouldn't this be something like "This should be
> used instead of tp_handle_ensure() for user provided contact identifiers, but
> it is not necessary for identifiers from the server" ?

fixed

> @@ +326,5 @@
> > +    gpointer context,
> > +    GAsyncReadyCallback callback,
> > +    gpointer user_data)
> > +{
> > +  return TP_HANDLE_REPO_IFACE_GET_CLASS (self)->ensure_handle_async (self,
> 
> We should probably check this function is implemented and fallback somehow if
> not? Oh, or does your default implementation below guarantee this is always
> non-NULL?

It can't be NULL, the iface defines a default implementation, and tp_handle_repo_iface_implement_ensure_handle_async() verifies that it's not set to NULL.

> @@ +392,5 @@
> > + *
> > + * Since: 0.UNRELEASED
> > + */
> > +void
> > +tp_handle_repo_iface_implement_ensure_handle_async (TpHandleRepoIface *self,
> 
> I don't understand this? Why don't we just make handle repos override these in
> their class inits?

The problem is it needs override only in gabble, and only after we discovered jidlookup capability from server.

But now that I think about it, that modifies the virtual method for all instances, since it's like a class struct.

Ideally I should subclass TpDynamicHandleRepo in gabble and override its default implementation of ensure_handle_async. Problem is TpDynamicHandleRepo is not subclassable since TpHandleRepoIfaceClass and TpDynamicHandleRepo structs are internal. I'll have to make them public...

> @@ +519,5 @@
> > +{
> > +  GSimpleAsyncResult *simple = (GSimpleAsyncResult *) result;
> > +
> > +  g_return_val_if_fail (g_simple_async_result_is_valid (result,
> > +      G_OBJECT (self), NULL), 0);
> 
> I was going to say "we can check the source tag here?" but this is so
> implementations don't need to implement _finish, right? If so, yes, good.

Exactly, it's like in TpBaseContactList, it works if _finish() is not overridden.
Comment 16 Jonny Lamb 2012-05-29 02:50:18 UTC
(In reply to comment #15)
> Exactly, it's a bit tricky to do with multiple ids since ordering is important.
> I'll fix that when we agree on the idea :)

Cool.

> The problem is it needs override only in gabble, and only after we discovered
> jidlookup capability from server.
> 
> But now that I think about it, that modifies the virtual method for all
> instances, since it's like a class struct.
> 
> Ideally I should subclass TpDynamicHandleRepo in gabble and override its
> default implementation of ensure_handle_async. Problem is TpDynamicHandleRepo
> is not subclassable since TpHandleRepoIfaceClass and TpDynamicHandleRepo
> structs are internal. I'll have to make them public...

Yeah I reckon that's the best route. Subclass TpDynamicHandleRepo, and when ensure_handle_async is called, see whether we're on live.com and if so, look it up. It's a shame about having to make more of the handle repo structs public but I think that's fine; you might have to add more public convenience functions to TpDynamicHandleRepo to reuse as much as code as possible in your gabble subclass.

I reckon in next we could just remove ensure_handle (the sync version) completely, no?
Comment 17 Xavier Claessens 2012-05-29 04:35:28 UTC
Created attachment 62205 [details] [review]
TpHandleRepoIface: Use G_DEFINE_INTERFACE
Comment 18 Xavier Claessens 2012-05-29 04:35:31 UTC
Created attachment 62206 [details] [review]
TpHandleRepoIface: add tp_handle_ensure_async()

A default implementation is provided that just use tp_handle_ensure()
and return the handle in an idle callback.
Comment 19 Xavier Claessens 2012-05-29 04:35:35 UTC
Created attachment 62207 [details] [review]
TpDynamicHandleRepo: factor out ensure_handle_take_normalized_id()
Comment 20 Xavier Claessens 2012-05-29 04:35:39 UTC
Created attachment 62208 [details] [review]
TpHandleRepoIface: expose internally _tp_handle_repo_default_ensure_handle_async()
Comment 21 Xavier Claessens 2012-05-29 04:35:42 UTC
Created attachment 62209 [details] [review]
TpDynamicHandleRepo: Support async normalization function

Override TpHandleRepoIface::ensure_handle_async() and use an
user provided async normalization function.
Comment 22 Xavier Claessens 2012-05-29 04:35:46 UTC
Created attachment 62210 [details] [review]
TpContactsMixin: use tp_handle_ensure_async() in GetContactByID

The identifier is most probably provided by the user, it could need
server-side normalization.
Comment 23 Xavier Claessens 2012-05-29 04:35:50 UTC
Created attachment 62211 [details] [review]
WIP: TpBaseConnection: use tp_handle_ensure_async() in RequestHandles
Comment 24 Xavier Claessens 2012-05-29 04:42:21 UTC
Created attachment 62213 [details] [review]
Implement WLM jidlookup

See description: http://msdn.microsoft.com/en-us/library/live/hh550849.aspx
Comment 25 Xavier Claessens 2012-05-29 04:50:59 UTC
Voilà my latest idea: I set an async normalization function on TpDynamicHandleRepo, which is already what it does for the sync normalization, so it makes sense to do like this.

I don't think tp_handle_ensure() should be removed in next, since it's much more convenient than tp_handle_ensure_async() for ids coming from server, which is where ids comes from most of the times.

I'm not sure it's worth having a cache tbh. The server round-trip happens only for ids typed by the user, so in practice it is only the "add contact" dialog, and the user is not going to re-type the same id that much.
Comment 26 Jonny Lamb 2012-05-30 03:41:32 UTC
Comment on attachment 62209 [details] [review]
TpDynamicHandleRepo: Support async normalization function

Review of attachment 62209 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/handle-repo-dynamic.c
@@ +594,5 @@
> +
> +  if (self->normalize_async == NULL)
> +    {
> +      /* Fallback to default implementation */
> +      _tp_handle_repo_default_ensure_handle_async (repo, connection, id,

Do you really have to make _tp_handle_repo_default_ensure_handle_async() public (within the library)? Can't you chain up to something like:

   TP_HANDLE_REPO_IFACE_CLASS (tp_dynamic_handle_repo_parent_class)->ensure_handle_async()

?
Comment 27 Jonny Lamb 2012-05-30 03:45:48 UTC
Comment on attachment 62209 [details] [review]
TpDynamicHandleRepo: Support async normalization function

Review of attachment 62209 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/handle-repo-dynamic.c
@@ +571,5 @@
> +  else
> +    {
> +      TpHandle handle;
> +
> +      handle = ensure_handle_take_normalized_id (self, normal_id);

If we normalize 'foo' to 'bar' and get a handle of 42 for 'bar', then we want to save 42 as the handle for both 'foo' and 'bar', don't we?
Comment 28 Jonny Lamb 2012-05-30 03:50:32 UTC
(In reply to comment #25)
> Voilà my latest idea: I set an async normalization function on
> TpDynamicHandleRepo, which is already what it does for the sync normalization,
> so it makes sense to do like this.

Seems sensible to me.

> I don't think tp_handle_ensure() should be removed in next, since it's much
> more convenient than tp_handle_ensure_async() for ids coming from server, which
> is where ids comes from most of the times.

Okay, fair enough.

> I'm not sure it's worth having a cache tbh. The server round-trip happens only
> for ids typed by the user, so in practice it is only the "add contact" dialog,
> and the user is not going to re-type the same id that much.

I still think it's worthwhile, and pretty easy to achieve with your current patches now, no? In the dyamic handle repo, in ensure_async, surely you can just look up the string in the string_to_handle hash table; If it exists then don't bother calling the normalize function?
Comment 29 Xavier Claessens 2012-05-30 06:54:15 UTC
(In reply to comment #26)
> Comment on attachment 62209 [details] [review] [review]
> TpDynamicHandleRepo: Support async normalization function
> 
> Review of attachment 62209 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-glib/handle-repo-dynamic.c
> @@ +594,5 @@
> > +
> > +  if (self->normalize_async == NULL)
> > +    {
> > +      /* Fallback to default implementation */
> > +      _tp_handle_repo_default_ensure_handle_async (repo, connection, id,
> 
> Do you really have to make _tp_handle_repo_default_ensure_handle_async() public
> (within the library)? Can't you chain up to something like:
> 
>    TP_HANDLE_REPO_IFACE_CLASS
> (tp_dynamic_handle_repo_parent_class)->ensure_handle_async()
> 
> ?

I though it wasn't possible, but now I've discovered g_type_default_interface_peek() to get the default vtable of an interface. I've modified the code to use that and dropped that commit.
Comment 30 Xavier Claessens 2012-05-31 02:58:35 UTC
Created attachment 62323 [details] [review]
TpBaseConnection: use tp_handle_ensure_async() in RequestHandles
Comment 31 Jonny Lamb 2012-05-31 10:15:45 UTC
Comment on attachment 62323 [details] [review]
TpBaseConnection: use tp_handle_ensure_async() in RequestHandles

Review of attachment 62323 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/base-connection.c
@@ +2551,5 @@
> +      *handle_p = handle;
> +    }
> +
> +out:
> +  if ((--request->n_pending) == 0)

http://i.imgur.com/8he1T.jpg good work.

@@ +2616,5 @@
>      }
>  
> +  for (cur_name = names; *cur_name != NULL; cur_name++)
> +    {
> +      count++;

g_strv_length()?
Comment 32 Xavier Claessens 2012-06-01 02:52:34 UTC
Created attachment 62367 [details] [review]
TpBaseConnection: use tp_handle_ensure_async() in RequestHandles
Comment 33 Jonny Lamb 2012-06-28 03:46:21 UTC
Comment on attachment 62367 [details] [review]
TpBaseConnection: use tp_handle_ensure_async() in RequestHandles

Review of attachment 62367 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/base-connection.c
@@ +2621,5 @@
> +  request->handles = g_array_sized_new (FALSE, TRUE, sizeof (guint), count);
> +  request->n_pending = count;
> +  request->context = context;
> +
> +  /* _sized_new() just pre-allocate memory, but handles->len is still 0 */

pre-allocates
Comment 34 Xavier Claessens 2012-06-28 06:44:03 UTC
Thanks for the review, branches merged (telepathy-glib 0.19.2 and gabble master).