[Bug 27309] respect the Observer.Recover flag

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=27309





--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-03-29 08:30:32 PST ---
> +    master = mcd_master_get_default ();
> +    dispatcher = mcd_master_get_dispatcher (master);

Sorry, but I think this reduces MC's maintainability. I'm trying to phase out
the layering violations in MC: please don't introduce more.

(It's possible that mcd_master_get_default() ought to be marked as deprecated
within MC itself, to discourage this sort of thing.)

The layering currently goes like this, if I remember correctly:

McdDispatcher > McdDispatchOperation > McdHandlerMap
McdDispatcher > McdDispatchOperation > McdClientRegistry > McdClientProxy

(where ">" means "is bigger than/may reference/may call methods on", and is
transitive)

Similarly, the ChannelDispatcher and the AccountManager should have a minimum
of shared state, to reduce crazy interactions between the two (eventual goal:
MC can be an AM and/or a CD, independently talking to the other half via D-Bus
if it's non-local).

A couple of possible designs:

* Clients gain a reference to the handler map. The handler map gains a map from
channel object path to account object path (whose keys you could use as the
list of channels to recover, probably).

* Clients emit a signal when they want recovery. The dispatcher catches that
signal, and calls some method on the clients for each matching channel (in
practice, this would probably require the same enhancements to the handler
map).

You should hopefully also find that at least some of _mcd_client_recover() (and
the various helpers it calls) is duplicated by some existing code that could be
factored out (one example below).

------------------------

Minor things:

I'd prefer it if you hadn't checked in a commit that assumes a TpChannel is a
McdChannel (i.e. clearly doesn't work) followed by one that fixes it - rebase
and squash the two?

> +        g_object_get (conn, "object-path", &connection_path, NULL);

Please put one key/value per line when calling g_object_get or g_object_new,
even if there are 0 or 1 pairs.

In this case you could have just used tp_proxy_get_object_path(), though -
using convenience "C bindings" like tp_proxy_get_object_path() and
tp_channel_borrow_connection() is encouraged.

> +_build_chan_details (TpChannel *chan)

This could benefit from tp_value_array_build(). You could also move it to some
sort of "channel utilities" module, and use it in the implementation of the
McdChannel pseudo-method _mcd_channel_details_build_from_list().

(I'd suggest channel-utils.[ch] and the namespace _mcd_tp_channel_foo() for
such a file; _mcd_tp_channel_should_close() could move there from
mcd-channel-priv.h.)

> +GList* _mcd_handler_map_get_handled_channels (McdHandlerMap *self);

Nitpicking: swap the first * and the first space


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