(In reply to comment #0)
> [D] (depends on [B]) McdDispatchOperation should use McdClientProxy to
> implement much of the logic which is now in McdDispatcher/McdDispatcherContext

I meant "... should use McdClientRegistry to ...".

Object hierarchy (where > means "is conceptually larger than, and may use the API of"):

McdDispatcher > many McdDispatchOperations > one McdClientRegistry > many McdClients

Currently up to 26cf02b0767fc85d16827482252b89978c53f7d3 with my review (most things are nitpicks, generally stuff looks good):

* operation_list_active seems a bit of a premature optimisation, existing code
  though

* if (approved_handler != NULL && approved_handler[0] != '\0')
    reminds me that we have EMP_STR_EMPTY in empathy, which is a macro that
    does exactly this, might be useful to put in tp-glib as well as it's a
    commen telepathy pattern.

* mcd_dispatcher_get_possible_handlers -> _get_ functions returning refs is
  unexpected (again existing code though)

* 5171899833d482548f3ff96143e35ee4d0619ead
  +     * dup'd bus name (string) => arbitrary non-NULL pointer */
    arbitrary? really ? from the looks it seems it means dummy pointer

  function names seems a bit unobvious:
    _mcd_dispatch_operation_set_handler_failed ->
      _mcd_dispatch_operation_mark_handler_failed or
      _mcd_dispatch_operation_add_failed_handler

   _mcd_dispatch_operation_get_handler_failed ->
     _mcd_dispatch_operation_handler_failed or
     _mcd_dispatch_operation_handler_has_failed

* activateable property on McdClientProxy should probably be readable as well

(In reply to comment #2)
> Currently up to 26cf02b0767fc85d16827482252b89978c53f7d3 with my review (most
> things are nitpicks, generally stuff looks good):
> 
> * operation_list_active seems a bit of a premature optimisation, existing code
>   though

Yeah, mardy insisted (it was a condition he placed on enabling the OperationList interface at all...)

> * if (approved_handler != NULL && approved_handler[0] != '\0')
>     reminds me that we have EMP_STR_EMPTY in empathy, which is a macro that
>     does exactly this, might be useful to put in tp-glib as well as it's a
>     commen telepathy pattern.

That might be reasonable to do, but I don't think it's a merge blocker.

> * mcd_dispatcher_get_possible_handlers -> _get_ functions returning refs is
>   unexpected (again existing code though)

Yes, that's strange. Fixed in dispatcher-refactor-part1 branch.

> * 5171899833d482548f3ff96143e35ee4d0619ead
>   +     * dup'd bus name (string) => arbitrary non-NULL pointer */
>     arbitrary? really ? from the looks it seems it means dummy pointer

I meant "arbitrary" in the sense of "the value of this pointer is not API - only the fact that it's not NULL is API". I've changed it to "dummy".

>   function names seems a bit unobvious:
>     _mcd_dispatch_operation_set_handler_failed ->
>       _mcd_dispatch_operation_mark_handler_failed or
>       _mcd_dispatch_operation_add_failed_handler
>
>    _mcd_dispatch_operation_get_handler_failed ->
>      _mcd_dispatch_operation_handler_failed or
>      _mcd_dispatch_operation_handler_has_failed

Agreed on IRC not to be a big deal, since this API will not remain API when I've finished refactoring anyway.

> * activateable property on McdClientProxy should probably be readable as well

Agreed on IRC not to be a big deal: MC is not (really) a library, and when in doubt I'm using C accessors rather than properties (to make it easier to grep for them!).

I might turn this into a more conventional GObject when I've finished refactoring, but for the moment, the C accessors are all that matter.

(++ from Sjoerd, up to dispatcher-refactor-part1)

(++ from Sjoerd, up to dispatcher-refactor-part2)

dispatcher-refactor-part3 should be reviewable if you're feeling keen.

dispatcher-refactor is not, yet (I'm still working on it).

dispatcher-refactor-part4 closes this bug (for now...) and is nearly ready for review.

Things to review for this:

* 52-test-plugin-delay (additional regression test coverage, for 5.2)
* test-plugin-delay (non-trivial merge of 52-test-plugin-delay into master; requires 52-t-p-d)
* dispatcher-refactor-part3 (migrate some of McdDispatcher into McdClientRegistry)
* dispatcher-refactor-part4 (migrate some of McdDispatcher into McdDispatchOperation; requires -part3 and t-p-d)

(In reply to comment #7)
> * 52-test-plugin-delay (additional regression test coverage, for 5.2)
> * test-plugin-delay (non-trivial merge of 52-test-plugin-delay into master;
> requires 52-t-p-d)

These were reviewed by Alberto and Sjoerd, and merged.

> * dispatcher-refactor-part3 (migrate some of McdDispatcher into
> McdClientRegistry)
> * dispatcher-refactor-part4 (migrate some of McdDispatcher into
> McdDispatchOperation; requires -part3 and t-p-d)

These are still pending review.

I've added a patch to -part3 to remove the call to ReloadConfig (which was removed in a necessarily-conflicting way in master), and merged today's master branch into it.

I've also merged -part3 into -part4.

It's getting quite tricky to keep the branches in sync, I'd appreciate review :-)

-part3 has 25 patches (mostly small), plus a no-changes merge to keep git happy. It implements [C] from the initial comment.

-part4 has about 90 patches (mostly small). When someone has reviewed -part3, -part4 could probably be rebased onto it without too much pain.

part3:

We should really add TpStringPool. It's silly to keep using the dynamic handle
repo and have to explain all over the place that they're not actually handles.

+  /* the ListNames call we'll make in _constructed is the initial lock */
+  self->priv->startup_lock = 1;

Wouldn't it be clearer to set that alongside the call?

Otherwise, looks good, in a kind of "well, it looks plausible, assuming the rest of MC behaves how I expect it does" way. :)

(In reply to comment #11)
> We should really add TpStringPool. It's silly to keep using the dynamic handle
> repo and have to explain all over the place that they're not actually handles.

Since tp-glib 0.9 is semi-frozen with a much shorter than normal development cycle, I'm inclined to defer this to 0.11, but yes this is a bug. I've filed Bug #24618.

> +  /* the ListNames call we'll make in _constructed is the initial lock */
> +  self->priv->startup_lock = 1;
> 
> Wouldn't it be clearer to set that alongside the call?

Perhaps, I'll have a look.

(In reply to comment #12)
> Perhaps, I'll have a look.

14:33 < smcv> wjt: I think it's better to have startup_lock already set in 
              _init, to make it obvious that it is locked initially
14:33 < smcv> even though the thing we're conceptually waiting for can't 
              actually happen til _constructed

This didn't seem to meet with any objection, so merging -part3. Thanks!

dr4.1-delete-stuff is a reviewable prefix of -part4. It deletes signals that nobody listens for, a deprecated public method that nobody calls, and a private method that no longer does anything.

(In reply to comment #14)
> dr4.1-delete-stuff is a reviewable prefix of -part4. It deletes signals that
> nobody listens for, a deprecated public method that nobody calls, and a private
> method that no longer does anything.

Ship it.

(In reply to comment #15)
> Ship it.

Thanks, I did. More incremental branches, rebased onto each other:

dr4.2-handler-map-tp-channel: simplify McdHandlerMap by it being in terms of TpChannel only

dr4.3-move-to-cdo: move various members of McdDispatcherContext to McdDispatchOperation, since every MDC now has an MDO

dr4.4-fix-reinvocation: fix the semantics of reinvoking handlers due to EnsureChannel being called again, which were never right (secretly not actually a refactoring, but I only spotted this bug during development of this branch); this branch introduces some slightly duplicate code to call HandleChannels, which is fixed in undup-handle-channels (Bug #24569)

... more "milestones" to follow when I've tagged them.

(In reply to comment #16)
> dr4.3-move-to-cdo: move various members of McdDispatcherContext to
> McdDispatchOperation, since every MDC now has an MDO

This introduces a FIXME which is addressed in the third patch after dr4.4. I'm having a hard time splitting up the tail of the branch; I might just add some arbitrary milestones.

I've added some more or less arbitrary milestones, dr4.5, dr4.6 and dr4.7. The part of dispatcher-refactor-part4 after dr4.7 is basically just cleanup now.

Up to 4.3:

I noticed that McdDispatchOperationPrivate has a gchar *claimer. I think that should be gchar *plaintiff!

+        if (G_LIKELY (dbus_connection))
+            dbus_g_connection_register_g_object (dbus_connection,
+                                                 priv->object_path, object);

Why wouldn't it have one?

I'm a little suspicious about inc_observers() reffing the McdDispatchOperation. Surely whatever owns the MDO has its own ref? I fear accidental ref leaks.

block_finished, may_finish, check_finished, actually_finished? Pretty confusing stuff. :)

(In reply to comment #19)
> I noticed that McdDispatchOperationPrivate has a gchar *claimer. I think that
> should be gchar *plaintiff!

Thanks but no :-P

This is temporary anyway, as fixing Bug #21003 will probably result in claimer/handler being replaced by a queue of structs representing Claim, HandleWith and EnsureChannel calls, and the handlers they'd like; when one such call succeeds, subsequent Claim and HandleWith calls will be told they've failed.

> +        if (G_LIKELY (dbus_connection))
> +            dbus_g_connection_register_g_object (dbus_connection,
> +                                                 priv->object_path, object);
> 
> Why wouldn't it have one?

Pre-existing, I think. I believe it always would, but you never know in MC... I'll double-check, and either remove the G_LIKELY or make it some flavour of assertion.

> I'm a little suspicious about inc_observers() reffing the McdDispatchOperation.
> Surely whatever owns the MDO has its own ref? I fear accidental ref leaks.

At the moment, whatever owns the MDO (the McdDispatcherContext) does have its own ref. However, it does this by reffing the McdDispatcherContext across various calls. One of the goals of this branch is to have the McdDispatcherContext manage its own life-cycle, so the McdDispatcherContext can be relegated to being a helper struct used to invoke C plugins, which can go away entirely as soon as the last plugin has finished; this is finally achieved somewhere around the 4.7 branch.

(The refcount of the McdDispatcherContext is much less amenable to debugging than that of the MDO, since it's not refdbg'able, leading to strange hacks.)

There is, unfortunately, no signal emitted by the MDO that is suitable to cause the McdDispatcher to unref it - Finished can perhaps be that signal once I fix Bug #21003.

> block_finished, may_finish, check_finished, actually_finished? Pretty confusing
> stuff. :)

At the moment Finished is emitted far too early: delaying its emission until we have *actually* finished is part of Bug #21003. Part of the reason for Finished being so early is that it tells the Dispatcher to start calling HandleChannels (until the 6th commit of branch 4.5, which splits it into two signals, one for D-Bus and one to indicate that dispatching can continue; the signal indicating that dispatching can continue is later removed, since every effect it caused was moved to the MDC).

block_finished/unblock_finished are pre-existing; they're deleted halfway through branch 4.5, in favour of having them be implicit in inc_observers/dec_observers/inc_ado/dec_ado. may_finish() encapsulates "we are allowed to emit Finished and ChannelLost if we want to", i.e. "no observers and no approvers are pending".

finished (renamed to wants_to_finish during this branch, to indicate what it really means) indicates whether we've reached a situation where Finished should (in the current implementation) be emitted as soon as possible: i.e. HandleWith was called, or Claim was called, or EnsureChannel caused approval, or all channels were lost.

check_finished() is called by anything that decrements the pending observer/approver count; if there are none pending, it emits any queued ChannelLost signals, then calls actually_finish() if wants_to_finish is true.

actually_finish() is called by check_finished(), and also whenever wants_to_finish becomes TRUE while may_finish() is TRUE. It emits any queued Finished signal. I see your point that actually_finish and check_finished should probably be one function.

Yes, confusing, but may I file a bug about it instead, and sort it out after (or more likely, as part of) Bug #21003?

I've filed Bug #24637 to represent Finished being emitted too soon. In practice, Bug #24637 and Bug #21003 will probably be fixed in the same branch.

(In reply to comment #20)
> (In reply to comment #19)
> > +        if (G_LIKELY (dbus_connection))
> > +            dbus_g_connection_register_g_object (dbus_connection,
> > +                                                 priv->object_path, object);
> > 
> > Why wouldn't it have one?
> 
> Pre-existing, I think. I believe it always would, but you never know in MC...
> I'll double-check, and either remove the G_LIKELY or make it some flavour of
> assertion.

It can actually be NULL in the debug build of MC, which doesn't exit instantly in libdbus when disconnected, but instead waits a few seconds to clean up stale objects. I'll add a comment.

I've merged dr4.2-handler-map-tp-channel.

(In reply to comment #22)
> I'll add a comment.

Added to dr4.3-move-to-cdo.

(In reply to comment #23)
> Added to dr4.3-move-to-cdo.

Ship it.

dr4.4-fix-reinvocation:

I'm suspicious of the new logic which invokes the first handler object on the unique name that originally handled the channel that matches the filter. Could the McdHandlerMap remember which well-known name handled the channel as well? Maybe MC could try to use that if it still exists, and fall back to the first-matching-one behaviour?

+        DEBUG ("Handler %s does not exist in client registry, not "
+               "reinvoking", possible_handlers[0]);
+        mcd_dispatcher_finish_reinvocation (request);
+        return;
+    }

should goto finally;

+mcd_dispatcher_finish_reinvocation (McdChannel *request)
 {
+    _mcd_channel_set_status (request, MCD_CHANNEL_STATUS_DISPATCHED);

Why is that necessary? We only enter this code path in the first place if the status is Dispatched.

(In reply to comment #25)
> dr4.4-fix-reinvocation:
> 
> I'm suspicious of the new logic which invokes the first handler object on the
> unique name that originally handled the channel that matches the filter. Could
> the McdHandlerMap remember which well-known name handled the channel as well?
> Maybe MC could try to use that if it still exists, and fall back to the
> first-matching-one behaviour?

In principle, yes it could. I'm assuming here that "multi-headed" clients will always have to have some centralized map from object path to GtkWindow (or whatever), in order to reply to Get("HandledChannels") correctly, and so it doesn't really matter which head we call - they'll all have an early-return guard, "if already handling this, bump the window into the foreground and return".

There are two ways that the new logic could pick the wrong head from such a hydra:

* The handler has put up an extra head that matches the channel better; that new head catches the channel, instead of it going to the less specific head (this would happen if the fake Empathy in dispatcher/capture-bundle.py had a channel redispatched to it)

* The handler previously had an extra head that matched the channel better, but it has cut off that head (I can't think of a real-world use case for this); due to the fallback behaviour you propose, your logic and mine are effectively equivalent here

To what extent do you consider the former case to be a problem? I could try to code up this functionality if you think it's a blocker, or just file a bug about it.

> +        DEBUG ("Handler %s does not exist in client registry, not "
> +               "reinvoking", possible_handlers[0]);
> +        mcd_dispatcher_finish_reinvocation (request);
> +        return;
> +    }
> 
> should goto finally;

Good catch, minor memory leak, fixed in the branch (I haven't rebased subsequent branches).

> +mcd_dispatcher_finish_reinvocation (McdChannel *request)
>  {
> +    _mcd_channel_set_status (request, MCD_CHANNEL_STATUS_DISPATCHED);
> 
> Why is that necessary? We only enter this code path in the first place if the
> status is Dispatched.

You'd think... but no. We have two McdChannel objects. After the renaming done at the beginning of this chapter of the branch, they are named consistently as 'channel' and 'request'. The situation is:

* 'channel' exists already
* 'request' (which is an McdChannel with its "really a ChannelRequest" hat on) is an EnsureChannel() call that returned the same object-path that 'channel' has
* 'channel' has been DISPATCHED
* _mcd_channel_copy_details copies the TpChannel from 'channel' to 'request', but nothing else
* reinvocation runs
* 'request' now needs to be set to DISPATCHED

So, I think I was right the first time.

dr4.5:

+    channels_array = _mcd_dispatch_operation_dup_channel_details
+        (context->operation);

Not putting the ( on the same line as the function name tricks cscope into not realising this is a function call.  Not that big a deal I guess.

     context->operation = _mcd_dispatch_operation_new (priv->clients,
-        !requested, channels, (const gchar * const *) possible_handlers);
-    /* ownership of @channels is stolen, but the GObject references are not */
+        !requested, g_list_copy (channels),
+        (const gchar * const *) possible_handlers);
+    /* the copy of @channels is stolen, but the GObject references are not */

Would it be clearer to make _mcd_dispatch_operation_new() deep-copy the channels?

+    /* if it was a channel request, and it was cancelled, then the whole
+     * context should be aborted */

This comment no longer makes sense inside MDO.

+        /* remove the context from the list of active contexts */
+        priv->operations = g_list_remove (priv->operations, context->operation);
Comment doesn't match the implementation. I think the comment is more correct.

> McdDispatchOperation: emit ready-to-dispatch just after finished

Why after and not before?

It's slightly confusing that mcd_dispatcher_run_handlers() is both a signal callback and a method called when running a handler fails, but I think I'll get over it.

(In reply to comment #26)
> There are two ways that the new logic could pick the wrong head from such a
> hydra:
> 
> * The handler has put up an extra head that matches the channel better; that

(or just as well, and happens to be sorted first)

> new head catches the channel, instead of it going to the less specific head
> (this would happen if the fake Empathy in dispatcher/capture-bundle.py had a
> channel redispatched to it)
> 
> * The handler previously had an extra head that matched the channel better, but
> it has cut off that head (I can't think of a real-world use case for this); due
> to the fallback behaviour you propose, your logic and mine are effectively
> equivalent here
> 
> To what extent do you consider the former case to be a problem? I could try to
> code up this functionality if you think it's a blocker, or just file a bug
> about it.

I guess it's not a big deal. I'd be okay with filing a bug about it.

> Good catch, minor memory leak, fixed in the branch (I haven't rebased
> subsequent branches).

Fix looks good.

> > +mcd_dispatcher_finish_reinvocation (McdChannel *request)
> >  {
> > +    _mcd_channel_set_status (request, MCD_CHANNEL_STATUS_DISPATCHED);
> > 
> > Why is that necessary? We only enter this code path in the first place if the
> > status is Dispatched.
> 
> You'd think... but no. We have two McdChannel objects. After the renaming done
> at the beginning of this chapter of the branch, they are named consistently as
> 'channel' and 'request'. The situation is:
> 
> * 'channel' exists already
> * 'request' (which is an McdChannel with its "really a ChannelRequest" hat on)
> is an EnsureChannel() call that returned the same object-path that 'channel'
> has
> * 'channel' has been DISPATCHED
> * _mcd_channel_copy_details copies the TpChannel from 'channel' to 'request',
> but nothing else
> * reinvocation runs
> * 'request' now needs to be set to DISPATCHED
> 
> So, I think I was right the first time.

Cool! I stand corrected. I guess there's a good reason why copy_details() doesn't copy the status?

On that assumption, 4.4 looks good to go.

(In reply to comment #27)
> dr4.5:

Oops, looks like I didn't quite review it all.

> McdDispatchOperation: inline _mcd_dispatch_operation_is_invoking_early_clients

You forgot to remove it from the .h.

> McdDispatcher: match_filters: operate entirely in terms of properties

the immutable_properties() paths assert that channel_properties is not NULL;
the requested_properties() path does not, and nor does the "both" path.

(In reply to comment #28)
> (In reply to comment #26)
> > To what extent do you consider the former case to be a problem? I could try to
> > code up this functionality if you think it's a blocker, or just file a bug
> > about it.
> 
> I guess it's not a big deal. I'd be okay with filing a bug about it.

Bug #24645.

> Cool! I stand corrected. I guess there's a good reason why copy_details()
> doesn't copy the status?

Not necessarily, but I refuse to expand the scope of this branch to "alter the behaviour of McdChannel, fixing any hidden assumptions" :-)

(In reply to comment #27)
> dr4.5:
> 
> +    channels_array = _mcd_dispatch_operation_dup_channel_details
> +        (context->operation);
> 
> Not putting the ( on the same line as the function name tricks cscope into not
> realising this is a function call.  Not that big a deal I guess.

I'll fix that, it's a more common style I think.

>      context->operation = _mcd_dispatch_operation_new (priv->clients,
> -        !requested, channels, (const gchar * const *) possible_handlers);
> -    /* ownership of @channels is stolen, but the GObject references are not */
> +        !requested, g_list_copy (channels),
> +        (const gchar * const *) possible_handlers);
> +    /* the copy of @channels is stolen, but the GObject references are not */
> 
> Would it be clearer to make _mcd_dispatch_operation_new() deep-copy the
> channels?

Possibly. The underlying problem is that the property is of type G_TYPE_POINTER, which doesn't do any reffing or copying or anything, rather than a boxed type that deep-copies the GList automatically.

I *think* we're now far enough through that making this conventional wouldn't cause conflicts later, but just in case, can I defer this to the end?

> +    /* if it was a channel request, and it was cancelled, then the whole
> +     * context should be aborted */
> 
> This comment no longer makes sense inside MDO.

True, I'll fix it.

> +        /* remove the context from the list of active contexts */
> +        priv->operations = g_list_remove (priv->operations,
> context->operation);
> Comment doesn't match the implementation. I think the comment is more correct.

We do both (the contexty version is half a dozen lines below); the comment is wrong here, and I'll fix it. As a transitional thing we have a list of contexts *and* a list of operations; one commit after dr4.7, I drop the list of contexts, keeping only the list of operations.

> > McdDispatchOperation: emit ready-to-dispatch just after finished
> 
> Why after and not before?

Because the things I want to happen on ready-to-dispatch are currently in the finished handler, and they happen after the things I want to stay in the finished handler. I'm trying to change behaviour as little as possible; making the finished signal sane is a job for Bug #24637.

> It's slightly confusing that mcd_dispatcher_run_handlers() is both a signal
> callback and a method called when running a handler fails, but I think I'll get
> over it.

Happily, dr4.7 (or thereabouts) fixes this, by moving the entire "running Handlers" code path into the DispatchOperation.

(In reply to comment #29)
> > McdDispatchOperation: inline _mcd_dispatch_operation_is_invoking_early_clients
> 
> You forgot to remove it from the .h.

Fixed in f6ae712c79, halfway through 4.6. Sorry, I should have rebased that more aggressively.

> > McdDispatcher: match_filters: operate entirely in terms of properties
> 
> the immutable_properties() paths assert that channel_properties is not NULL;
> the requested_properties() path does not, and nor does the "both" path.

I'll look into it.

(In reply to comment #31)
> (In reply to comment #27)
> > dr4.5:
> > 
> > +    channels_array = _mcd_dispatch_operation_dup_channel_details
> > +        (context->operation);
> > 
> > Not putting the ( on the same line as the function name tricks cscope into not
> > realising this is a function call.  Not that big a deal I guess.
> 
> I'll fix that, it's a more common style I think.

In fact, I re-wrap that line when I move mcd_dispatcher_handle_channels into the CDO in the second-from-last patch of chapter 4.7, so I won't bother.

> > +    /* if it was a channel request, and it was cancelled, then the whole
> > +     * context should be aborted */
> > 
> > This comment no longer makes sense inside MDO.
> 
> True, I'll fix it.

Fixed in the branch. I haven't rebased.

> > +        /* remove the context from the list of active contexts */
> > +        priv->operations = g_list_remove (priv->operations,
> > context->operation);
> > Comment doesn't match the implementation. I think the comment is more correct.
> 
> We do both (the contexty version is half a dozen lines below); the comment is
> wrong here, and I'll fix it.

... actually, I'd rather not, since I deleted that comment halfway through 4.7.

> > > McdDispatcher: match_filters: operate entirely in terms of properties
> > 
> > the immutable_properties() paths assert that channel_properties is not NULL;
> > the requested_properties() path does not, and nor does the "both" path.
> 
> I'll look into it.

In all three cases channel_properties shouldn't be NULL. I'd rather do this after 4.7, since the code making the assertion moves into the CDO during the 4.7 branch (and is then badgered by the branch I'm currently working on, but I'll rebase that).

(In reply to comment #32)
> In all three cases channel_properties shouldn't be NULL. I'd rather do this
> after 4.7, since the code making the assertion moves into the CDO during the
> 4.7 branch (and is then badgered by the branch I'm currently working on, but
> I'll rebase that).

Fixed at the end of dr4.7, now.

(In reply to comment #31)
> > Would it be clearer to make _mcd_dispatch_operation_new() deep-copy the
> > channels?
> 
> Possibly. The underlying problem is that the property is of type
> G_TYPE_POINTER, which doesn't do any reffing or copying or anything, rather
> than a boxed type that deep-copies the GList automatically.
> 
> I *think* we're now far enough through that making this conventional wouldn't
> cause conflicts later, but just in case, can I defer this to the end?

In a patch appended to dr4.7, I've made mcd_dispatch_operation_set_property() deep-copy the channels, so _mcd_dispatch_operation_new doesn't (effectively) steal them; made _mcd_dispatcher_enter_state_machine not steal them; and made _mcd_dispatcher_take_channels free them, for the moment, since its name is sufficient indication that it steals things, I think.

Renaming _mcd_dispatcher_take_channels and making it not free the GList can be a job for some future point.

I've rebased everything after dr4.7 onto the new dr4.7, since the merge was non-trivial.

I also made the undup-handle-channels, limit-approval-bypass, wip-prefer-preferred-handler stack independent of always-observe-after-refactor, to reduce the stacking a little. I still recommend reviewing them in the same order they previously had (a-o-a-r first), though.

(In reply to comment #34)
> I also made the undup-handle-channels, limit-approval-bypass,
> wip-prefer-preferred-handler stack independent of
> always-observe-after-refactor

I take it back, these do need to be ordered.

Up to 92d18cf0d4:

dr4.6 looks fine.

In get_connection:

+        _mcd_dispatch_operation_get_connection_path
+            (MCD_DISPATCH_OPERATION (self)));

Opening paren should be on the same line as the function name.

+    /* collect object paths into a hash table, to drop duplicates */
+    for (c = channels; c != NULL; c = c->next)
+    {
+        const GList *reqs = _mcd_channel_get_satisfied_requests (c->data);
+

Why would there be duplicate satisfied requests?

Out of interest, what was the rationale for the spec saying:

> If the same NewChannels signal announces some channels that match the filter, and some that do not, then only a subset of the channels (those that do match the filter) are passed to [ObserveChannels].

It makes the code in MC a bit less straightforward than it would be otherwise, and well-behaved observers still have to deal if MC gives them channels they don't understand.

(In reply to comment #36)
> Up to 92d18cf0d4:
> 
> dr4.6 looks fine.

May I merge dr4.5 and dr4.6, on the basis that your criticisms of dr4.5 were all fixed in dr4.7?

> In get_connection:
> 
> +        _mcd_dispatch_operation_get_connection_path
> +            (MCD_DISPATCH_OPERATION (self)));
> 
> Opening paren should be on the same line as the function name.

Appended to dr4.7

> +    /* collect object paths into a hash table, to drop duplicates */
> +    for (c = channels; c != NULL; c = c->next)
> +    {
> +        const GList *reqs = _mcd_channel_get_satisfied_requests (c->data);
> +
> 
> Why would there be duplicate satisfied requests?

Pre-existing problem, I just moved the code. I *believe* that there should never be duplicates, because each request is satisfied by at most one channel - but beware that McdChannel is sometimes a Channel and sometimes a ChannelRequest, so it's non-obvious.

When I refactor MC so McdChannel doesn't exist (long-term goal), this can probably start asserting. For now, may I "resolve" this by filing a bug, and adding a FIXME comment referencing it?

> Out of interest, what was the rationale for the spec saying:
> 
> > If the same NewChannels signal announces some channels that match the filter, and some that do not, then only a subset of the channels (those that do match the filter) are passed to [ObserveChannels].
> 
> It makes the code in MC a bit less straightforward than it would be otherwise,
> and well-behaved observers still have to deal if MC gives them channels they
> don't understand.

A good point. We should perhaps reverse that, either in 0.19.x or when we break D-Bus API (depending how serious a change the spec cabal think it is).

The rationale was that the observer only asked to see certain channels, and it's somewhat easy for MC to arrange for that to happen. You're right that a well-behaved observer needs to cope with badness anyway, although I'm not sure how many observers will in practice be well-behaved :-)

(In reply to comment #37)
> When I refactor MC so McdChannel doesn't exist (long-term goal), this can
> probably start asserting. For now, may I "resolve" this by filing a bug, and
> adding a FIXME comment referencing it?

This is Bug #24763, which is referenced in a patch appended to dr4.7.

(In reply to comment #37)
> (In reply to comment #36)
> > Up to 92d18cf0d4:
> > 
> > dr4.6 looks fine.
> 
> May I merge dr4.5 and dr4.6, on the basis that your criticisms of dr4.5 were
> all fixed in dr4.7?

Seems fine to me.

Didn't mean to reassign that. Reassigning to Sjoerd who's continuing the review.

ship it! :)

Fixed in git. Thanks, everyone!