|
Description
Xavier Claessens
2012-05-25 01:43:13 UTC
Or perhaps we'll need to start allowing asynchronous handle normalization functions? (In reply to comment #1) > Or perhaps we'll need to start allowing asynchronous handle normalization > functions? +1 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'. 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? Hm, there is also TARGET_ID in channel requests could need that trick as well. 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. Created attachment 62155 [details] [review] WIP: Implement WLM jidlookup See description: http://msdn.microsoft.com/en-us/library/live/hh550849.aspx With those 2 dirty patches, I can add contacts in my WLM account using empathy. I would appreciate advices how to do this properly. 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? Created attachment 62173 [details] [review] WIP: Implement WLM jidlookup See description: http://msdn.microsoft.com/en-us/library/live/hh550849.aspx 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 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 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. 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? (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. (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? Created attachment 62205 [details] [review] TpHandleRepoIface: Use G_DEFINE_INTERFACE 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. Created attachment 62207 [details] [review] TpDynamicHandleRepo: factor out ensure_handle_take_normalized_id() Created attachment 62208 [details] [review] TpHandleRepoIface: expose internally _tp_handle_repo_default_ensure_handle_async() Created attachment 62209 [details] [review] TpDynamicHandleRepo: Support async normalization function Override TpHandleRepoIface::ensure_handle_async() and use an user provided async normalization function. 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. Created attachment 62211 [details] [review] WIP: TpBaseConnection: use tp_handle_ensure_async() in RequestHandles Created attachment 62213 [details] [review] Implement WLM jidlookup See description: http://msdn.microsoft.com/en-us/library/live/hh550849.aspx 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 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 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? (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? (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. Created attachment 62323 [details] [review] TpBaseConnection: use tp_handle_ensure_async() in RequestHandles 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()? Created attachment 62367 [details] [review] TpBaseConnection: use tp_handle_ensure_async() in RequestHandles 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 Thanks for the review, branches merged (telepathy-glib 0.19.2 and gabble master). |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
