[Bug 25236] TpHandler - a class that implements a Client.Handler

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-15 04:27:25 PDT ---
Design
======

If the intention is for this to be used in simple cases where subclassing-in-C
is too hard, it should perhaps be called TpSimpleHandler, and have a superclass
TpBaseClient (which would currently do hardly anything and just be a shim
around GObject, but could be extended over time).

Can language bindings arrange for arbitrary code to run in their class_init? If
so, the TpBaseClient base class could have a "hidden vtable" like the
TpSvcFooIface interfaces do, with class methods
tp_base_client_class_implement_handle_channels() etc., allowing us to add more
methods without having to reserve lots of ABI padding in the class struct.

To be honest I'd be more inclined to implement Observer first - it's the
easiest of the three client classes (Handler is next, and Approver is the
hardest, IMO).

I don't like the way the handler object does almost all the work, but then
requires you to request a bus name and register its object path? I'd be more in
favour of some sort of all-in-one "registration" function, like perhaps:

    client = tp_base_client_new (dbus_daemon, ...);

    tp_base_client_set_bypass_approval (client, TRUE);
    ... more setup functions that you can only call *before* register() ...

    /* registers you on @dbus_daemon */
    tp_base_client_register (client);

Would it be better for language bindings, etc., if the HandleChannels
pseudo-virtual-method was a signal, with it documented that you should have
exactly one handler for the signal?

It seems strange that TpHandlerHandleChannelsCb returns a boolean, but on
failure can only return one error (NotAvailable). Shouldn't it let you return
any GError?

I think the HandleChannels pseudo-virtual-method should have as its first
argument some sort of opaque context object. In telepathy-qt4, the equivalent
methods have a first parameter that abstracts returning from the D-Bus method
(it's a wrapper around the Qt equivalent of a DBusGMethodInvocation). The C
equivalent would be something like:

    void my_handle_channels_impl (TpHandler *self,
        TpHandlerContext *context,
        TpAccount *account,
        TpConnection *connection,
        TpChannel **channels,
        TpChannelRequest **requests,
        guint64 user_action_time);

    /* it is an error to not call one of these methods in HandleChannels */
    void tp_handler_context_delay (TpHandlerContext *);
    void tp_handler_context_accept (TpHandlerContext *);
    void tp_handler_context_fail (TpHandlerContext *, const GError *);

I think the Handler_Info dict should also be kept in the context object, so
that when we start defining keys for it, we can do things like this:

    /* generic extension API */
    const GValue *tp_handler_context_get_info (TpHandlerContext *, const gchar
*);
    /* imagine we'd defined keys "is-badgered" (b) and "mushroom-names" (as) */
    gboolean tp_handler_context_is_badgered (TpHandlerContext *);
    GStrv tp_handler_context_dup_mushroom_names (TpHandlerContext *);

We could even move (expected-to-be-)rarely-used arguments into the context
object: the channel requests, perhaps.

Details
=======

Why do the add-request, remove-request signals need vtable slots as well?

I'd be inclined to make add-request only contain the TpChannelRequest, and give
it an accessor for its own immutable properties.

A method called lookup_channel_request shouldn't return a ref; calling it
ensure_channel_request would be better, I think.

remove-request shouldn't be emitted at all for requests that we didn't already
know about, so we still need a lookup_channel_request, but one with lookup
(i.e. borrow-if-exists) semantics :-)

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