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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #7 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-12-21 05:09:54 PST ---
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.

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