[Bug 50341] Special trick to add a contact in WLM

[bugzilla-daemon at freedesktop.org]

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

--- Comment #15 from Xavier Claessens <xclaesse at gmail.com> 2012-05-29 01:30:50 PDT ---
(In reply to comment #12)
> 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?

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.

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