Bug 29457

This API is needed to solve this Empathy bug: https://bugzilla.gnome.org/show_bug.cgi?id=623682

I started implementing this in http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/request-observe-29457

I haven't reviewed this in detail since merge is blocked, but it looks OK in principle, except for this bit:

(In reply to comment #0)
> provide a new requesting method that will fail if your MC is too old for it
> (probably by fixing Bug #28866 at the same time), and make this telepathy-glib
> API (which would maybe be called
> tp_account_channel_request_ensure_and_observe_async()?) use it.

I'd prefer the "and observe" case to call EnsureChannelWithHints, which has the guarantee that if it works, then SucceededWithChannel will be emitted.

That means that if MC can't provide the semantics demanded by the API user, we get an error "cleanly", without the side-effect of creating an unobserved channel.

Done.

I also rebased the branch and did some squashed changes:
- use new fixed channel factory API
- sync with latest FUTURE spec

Rebased version using the stable API. I added support for Hints as well.

http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/request-observe-29457

Note that I didn't test it in real life as MC hasn't be updated yet.

I think TpChannelRequest should guarantee that succeeded-with-channel will always be emitted, documenting that @channel may be NULL if your MC doesn't support the hints stuff (and including a version number in the documentation). This would simplify the implementation in account-channel-request.c.

+      GError err = { TP_ERRORS, TP_ERROR_NOT_IMPLEMENTED,
+          "Channel has been created but MC didn't give it back to us" };

Obviously you'll still have to produce an error in the case where CreateChannelWithHints succeeds but the underlying SucceededWithChannel signal isn't emitted; if you've got as far as the succeeded-with-channel callback, then you know that the former succeeded, so you can use TP_ERROR_CONFUSED to indicate that MC is confused.

+  return request_and_observe_channel_finish (self, result,
+      tp_account_channel_request_create_and_observe_channel_async, error);

I think this is secret code for _tp_implement_finish_copy_pointer() if you stashed the object on the GSimpleAsyncResult as its op_result. Is there a reason not to do that?

+ * which has been created. Note that your are NOT handling this channel and so

Typo: this should say “you”           ^^^^ 

and you should use real emphasis rather than uppercase for NOT, and you should actually link to something which explains what “interact[ing] with the channel as an Observer” means. (Do we even have such a document? Maybe the relevant chapter of the Book™.)

+ * If a suitable channel already existed, its handler will be notified that
+ * the channel was requested again (for instance with
+ * #TpAccountChannelRequest::re-handled, #TpBaseClientClassHandleChannelsImpl
+ * or #TpSimpleHandler:callback), and can move its window to the foreground,
+ * if applicable. Otherwise, a new channel will be created and dispatched to
+ * a handler.

I think the parenthetical clause should read “for instance, with #TpAccountChannelRequest::re-handled, #TpBaseClientClassHandleChannelsImpl or #TpSimpleHandler:callback, if it is implemented using Telepathy-GLib”. I think I'd rephrase “and can move its window to the foreground, if applicable” to “so that it can re-present the window to the user, for example”.

In the test, why is ensure_and_observe_cb defined where it is? It should either be defined immediately after create_and_observe_cb, or (probably better) just before the start of the ensure tests, rather than in the middle of the create tests.

If we have a tp_channel_request_set_channel_factory() method, why is the corresponding property CONSTRUCT_ONLY? I think you mean CONSTRUCT maybe?

+channel_prepare_cb (GObject *source,
+    GAsyncResult *result,
+    gpointer user_data)
+{
+  TpAccountChannelRequest *self = user_data;
+  GError *error = NULL;
+
+  if (!tp_proxy_prepare_finish (source, result, &error))
+    {
+      DEBUG ("Failed to prepare channel: %s", error->message);
+      g_error_free (error);
+    }
+
+  complete_result (self);
+}

Why is the channel preparation error not propagated to the result of create_and_handle... ?

+  param_spec = g_param_spec_boxed ("hints", "Hints",
+      "metadata provided when requesting the channel",
+      TP_HASH_TYPE_STRING_VARIANT_MAP,
+      G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS);
+  g_object_class_install_property (object_class, PROP_HINTS, param_spec);

Metadata provided by whom? If you say “Metadata provided by the channel's requester, if any”, it's clearer maybe?

+   *
+   * Read-only.

Doesn't gtk-doc infer this for us from the definition below? Or is it too shit?

You duplicate the code to turn a dict of requests and their properties into a list of TpChannelRequests. Consider having an internal function?

I rebased the branch, the last commit you reviewed was:
  add tp_observe_channels_context_get_requests()

(In reply to comment #6)
> I think TpChannelRequest should guarantee that succeeded-with-channel will
> always be emitted, documenting that @channel may be NULL if your MC doesn't
> support the hints stuff (and including a version number in the documentation).
> This would simplify the implementation in account-channel-request.c.

Done. I added a TODO about the exact MC version as, AFAIK, the final API is
not implemented in MC yet (I'll check).

> +      GError err = { TP_ERRORS, TP_ERROR_NOT_IMPLEMENTED,
> +          "Channel has been created but MC didn't give it back to us" };
> 
> Obviously you'll still have to produce an error in the case where
> CreateChannelWithHints succeeds but the underlying SucceededWithChannel signal
> isn't emitted; if you've got as far as the succeeded-with-channel callback,
> then you know that the former succeeded, so you can use TP_ERROR_CONFUSED to
> indicate that MC is confused.

changed.

> +  return request_and_observe_channel_finish (self, result,
> +      tp_account_channel_request_create_and_observe_channel_async, error);
> 
> I think this is secret code for _tp_implement_finish_copy_pointer() if you
> stashed the object on the GSimpleAsyncResult as its op_result. Is there a
> reason not to do that?

Good catch; changed.

> + * which has been created. Note that your are NOT handling this channel and so
> 
> Typo: this should say “you”           ^^^^ 

fixed.

> and you should use real emphasis rather than uppercase for NOT, and you should
> actually link to something which explains what “interact[ing] with the channel
> as an Observer” means. (Do we even have such a document? Maybe the relevant
> chapter of the Book™.)

fixed, except the link. I guess we could link
http://telepathy.freedesktop.org/doc/book/sect.channel-dispatcher.clients.html
Do you know how to do external links in gtk-doc?
http://library.gnome.org/devel/gtk-doc-manual/stable/documenting_docbook.html
is only about internal links.

> + * If a suitable channel already existed, its handler will be notified that
> + * the channel was requested again (for instance with
> + * #TpAccountChannelRequest::re-handled, #TpBaseClientClassHandleChannelsImpl
> + * or #TpSimpleHandler:callback), and can move its window to the foreground,
> + * if applicable. Otherwise, a new channel will be created and dispatched to
> + * a handler.
> 
> I think the parenthetical clause should read “for instance, with
> #TpAccountChannelRequest::re-handled, #TpBaseClientClassHandleChannelsImpl or
> #TpSimpleHandler:callback, if it is implemented using Telepathy-GLib”. I think
> I'd rephrase “and can move its window to the foreground, if applicable” to “so
> that it can re-present the window to the user, for example”.

changed.

> In the test, why is ensure_and_observe_cb defined where it is? It should either
> be defined immediately after create_and_observe_cb, or (probably better) just
> before the start of the ensure tests, rather than in the middle of the create
> tests.

Probably for historical reason as I secretly didn't write all my tests in this
order. :)
I moved it.

> If we have a tp_channel_request_set_channel_factory() method, why is the
> corresponding property CONSTRUCT_ONLY? I think you mean CONSTRUCT maybe?

right, fixed.

> +channel_prepare_cb (GObject *source,
> +    GAsyncResult *result,
> +    gpointer user_data)
> +{
> +  TpAccountChannelRequest *self = user_data;
> +  GError *error = NULL;
> +
> +  if (!tp_proxy_prepare_finish (source, result, &error))
> +    {
> +      DEBUG ("Failed to prepare channel: %s", error->message);
> +      g_error_free (error);
> +    }
> +
> +  complete_result (self);
> +}
> 
> Why is the channel preparation error not propagated to the result of
> create_and_handle... ?

Because the features are prepared with a "best effort" guarantee. If we failed
to prepare those the channel has still be created and you can still observe
it, so I don't think we should fail the whole operation.

> +  param_spec = g_param_spec_boxed ("hints", "Hints",
> +      "metadata provided when requesting the channel",
> +      TP_HASH_TYPE_STRING_VARIANT_MAP,
> +      G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS);
> +  g_object_class_install_property (object_class, PROP_HINTS, param_spec);
> 
> Metadata provided by whom? If you say “Metadata provided by the channel's
> requester, if any”, it's clearer maybe?

changed.

> +   *
> +   * Read-only.
> 
> Doesn't gtk-doc infer this for us from the definition below? Or is it too shit?

Don't think so, and we usually say it explicitely in tp-glib's doc.

> You duplicate the code to turn a dict of requests and their properties into a
> list of TpChannelRequests. Consider having an internal function?

Good idea; done.

drive-by:

(In reply to comment #7)
> fixed, except the link. I guess we could link
> http://telepathy.freedesktop.org/doc/book/sect.channel-dispatcher.clients.html
> Do you know how to do external links in gtk-doc?
> http://library.gnome.org/devel/gtk-doc-manual/stable/documenting_docbook.html
> is only about internal links.

http://www.docbook.org/tdg/en/html/ulink.html

(In reply to comment #5)
> Note that I didn't test it in real life as MC hasn't be updated yet.

I tested it my branch fixing bug #30000 and it works fine.

I added a link to the TP book and the MC version which will implement SucceedWithChannel.

in observe-channels-context.c and handle-channels-context.c:

+      tp_proxy_get_dbus_daemon (self->account),request_props);

Coding style: there should be a space here     ^^

    * @connection and @channel may be %NULL your telepathy-mission-control is
-   * too old.
-   * TODO: put the actual version of MC implementing this.
+   * too old (< 5.7.1).

Aside from the missing “if”, this sentence would be more clearly written as something like:

“With telepathy-mission-control version 5.7.1 and earlier, @connection and @channel will be %NULL. When using newer versions, they will be correctly set to the newly-created channel, and the connection which owns it.”


+   * Deprecated: since 0.13.UNRELEASED. Use
+   * #TpChannelRequest::succeeded-with-channel instead

“Use #..., which provides the resulting channel, instead.”

(In reply to comment #11)
> in observe-channels-context.c and handle-channels-context.c:
> 
> +      tp_proxy_get_dbus_daemon (self->account),request_props);
> 
> Coding style: there should be a space here     ^^

fixed.

>     * @connection and @channel may be %NULL your telepathy-mission-control is
> -   * too old.
> -   * TODO: put the actual version of MC implementing this.
> +   * too old (< 5.7.1).
> 
> Aside from the missing “if”, this sentence would be more clearly written as
> something like:
> 
> “With telepathy-mission-control version 5.7.1 and earlier, @connection and
> @channel will be %NULL. When using newer versions, they will be correctly set
> to the newly-created channel, and the connection which owns it.”

Changed.

> +   * Deprecated: since 0.13.UNRELEASED. Use
> +   * #TpChannelRequest::succeeded-with-channel instead
> 
> “Use #..., which provides the resulting channel, instead.”

fixed.

Thanks for the review!

Merged to master; will be in 0.13.14.