[Bug 27309] respect the Observer.Recover flag

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Senko Rasic <senko at senko.net> 2010-04-13 04:44:56 PDT ---
(In reply to comment #2)
> 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)

Refactored according to this design.

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

This sounds like the best idea to me, so I implemented that.

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

Refactored a few things; one that I didn't is the actual call to
observe_channels - while trying to make a common implementation that both DO
and dispatcher while respawning could use, I found that it took the same amount
of code to prepare the arguments (since it's done differently in the two cases)
to not warrant the single implementation.

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

In the newly updated branch, I've rebased the entire changeset and split it up
into logical changes.

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

Done so.

> > +_build_chan_details (TpChannel *chan)
> 
> This could benefit from tp_value_array_build(). You could also move it to some

Tried, and found it didn't look any more clearer or less code (in fact, looks a
bit more implicit IMHO). 

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

Done so.

> > +GList* _mcd_handler_map_get_handled_channels (McdHandlerMap *self);
> 
> Nitpicking: swap the first * and the first space

Good catch of a typo, thanks.

(In reply to comment #3)
> The only way to be activatable is to install a .service file where MC will look
> (I think that means test/twisted/tools/ or something?) - to do this without
> interfering with other tests, you should probably use a unique dummy channel
> type ("ObserverRestartTest" or something) for all channels in this test.

Right, added a dummy "Logger" client observing
"org.freedesktop.Telepathy.Channel.Type.RespawnObservers".

(In reply to comment #4)
> commit. Any progress on breaking this up, or do you want it reviewed as a
> single large commit? Please ping this bug when it's ready for review.

I've split it in multiple logical changes, and is ready for review (as of
commit 4f4b1f3):

http://git.collabora.co.uk/?p=user/ptlo/telepathy-mission-control/.git;a=shortlog;h=refs/heads/observer-recovery

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