[Bug 24120] Refactor the McdDispatcher

[bugzilla-daemon at freedesktop.org]

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





--- Comment #31 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-10-20 11:31:49 PST ---
(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.


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