DBus Menu

Code review comment for lp:~ted/libdbusmenu/event-grouping

event-grouping
Merge into trunk.0.6

Revision history for this message

Allison Karlitskaya (desrt) wrote on 2012-04-04:

In broad strokes, this is pretty much what I had in mind, but the devil is in the details.

A few suggestions:

- use a GQueue instead of prepend-and-reversing a list

- the flag to enable/disable event-grouping is awkward. By my read, it gets set according to the remote version but can be manually changed by the user but may change again depending on when we get the version number from the remote? Also: I may want to group open/close but not activations. It might be better (not to mention vastly reducing the lines of code in this patch) to have an explicit API to send a message to a group as specified by the user? I guess it could take a GList of DbusmenuMenuItem instances? In the case that it's not supported, the flattening would be done on the client side. It's not a totally awesome API but it would be a lot less complicated and I'd be willing to do the collecting of like-queries in the HUD.

- the above API would also allow simplifying the dbus method signature since there would only be the possibility of a single event-type and a single timestamp (and only multiple IDs)

- when doing mass-dispatching of events I'm unlikely to care about errors. I wouldn't bother reporting them. In fact, the only user of this API (the HUD) will have set the "don't send reply" flag.

- it's very possible that I might send some events and then unref the client before returning to the mainloop (or by returning to the mainloop to find an event source with a higher priority than your idle that causes the unref).

- the /* Get the icon theme directories if available */ comment is intensely confusing (copy/paste error?)

- some of your use of GVariant is a bit odd (although you could argue a style of prefering to avoid varargs use):

- typically you use g_variant_new("()") for GDBus argument lists -- it helps to document the signature of the method call at the site of use

- g_variant_builder_add_value(x, g_variant_new_*()) is odd -- this is what g_variant_builder_add() is for

- creating a tuple builder, loading it up with 4 values, ending it and then packing it inside of another builder can be replaced with a single g_variant_builder_add (x, "(....)", ...) line

- glad to see you took care to preserve calling gdbus with NULL async callbacks in the don't-care case :)

- the receiving side looks good except for the questionable use of an idle/timeout(0). Why did you do that? You're already in a rather direct dispatch from the mainloop -- punting to another idle here gains you nothing except for additional complexity and may also introduce ordering issues (ie: the next dbus message to arrive may end up being processed before the idle for your group-of-events that arrived first).

In broad strokes, this is pretty much what I had in mind, but the devil is in the details.

A few suggestions:

- use a GQueue instead of prepend-and-reversing a list

- the flag to enable/disable event-grouping is awkward.  By my read, it gets set according to the remote version but can be manually changed by the user but may change again depending on when we get the version number from the remote?  Also: I may want to group open/close but not activations.  It might be better (not to mention vastly reducing the lines of code in this patch) to have an explicit API to send a message to a group as specified by the user?  I guess it could take a GList of DbusmenuMenuItem instances?  In the case that it's not supported, the flattening would be done on the client side.  It's not a totally awesome API but it would be a lot less complicated and I'd be willing to do the collecting of like-queries in the HUD.

- the above API would also allow simplifying the dbus method signature since there would only be the possibility of a single event-type and a single timestamp (and only multiple IDs)

- when doing mass-dispatching of events I'm unlikely to care about errors.  I wouldn't bother reporting them.  In fact, the only user of this API (the HUD) will have set the "don't send reply" flag.

- the /* Get the icon theme directories if available */ comment is intensely confusing (copy/paste error?)

- some of your use of GVariant is a bit odd (although you could argue a style of prefering to avoid varargs use):

- typically you use g_variant_new("()") for GDBus argument lists -- it helps to document the signature of the method call at the site of use

- g_variant_builder_add_value(x, g_variant_new_*()) is odd -- this is what g_variant_builder_add() is for

- creating a tuple builder, loading it up with 4 values, ending it and then packing it inside of another builder can be replaced with a single g_variant_builder_add (x, "(....)", ...) line

- glad to see you took care to preserve calling gdbus with NULL async callbacks in the don't-care case :)

- the receiving side looks good except for the questionable use of an idle/timeout(0).  Why did you do that?  You're already in a rather direct dispatch from the mainloop -- punting to another idle here gains you nothing except for additional complexity and may also introduce ordering issues (ie: the next dbus message to arrive may end up being processed before the idle for your group-of-events that arrived first).

« Back to merge proposal