[Bug 29457] TpAccountChannelRequest: request-and-observe helper

[bugzilla-daemon at freedesktop.org]

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

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|patch                       |review-
           Keywords|                            |patch

--- Comment #6 from Will Thompson <will.thompson at collabora.co.uk> 2010-12-16 11:18:14 PST ---
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?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.