[Bug 24120] Refactor the McdDispatcher

[bugzilla-daemon at freedesktop.org]

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





--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-09-29 09:10:52 PST ---
(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.


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