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

On Wed, 2012-04-04 at 00:41 +0000, Ryan Lortie wrote:
> - use a GQueue instead of prepend-and-reversing a list

Eh, okay. r427

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

The flag is only there so that the test suite can test both cases to
ensure there are no regressions. Since it is possible someone would
want to use it, I saw no reason to keep it private. There's really no
use-case to send singular events if the grouping is available.

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

Sure, but we need to maintain API compatibility no matter how someone
called the API it needs to work the same. It should be transparent to
upper level applications whether there is grouping or not. It's a
technical detail that can, and is, hidden in the library.

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

I'm unsure of what you're saying here. We do keep a ref to the client
so I think that case is taken care of... do you see a case where that's
not true?

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

Fixed. r428

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

Exactly. And I will :-) I like it when the compiler tells me when I've
done thing wrong not bug reports from apport :-)

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

The problem that we found is that in some cases we weren't giving enough
time for the mainloop to process and this would make application appear
"hung" to some users. Effectively appmenu-gtk is a leach on mainloop
time and we're trying to be as light a pull on that as possible. I
believe this will be especially the case with the number of events the
HUD is sending.

> Totally crazy outside-the-box type of idea (which I regret only having
> thought of just now): what if we just had a D-Bus API for "show all of
> the submenus that you know about" and the service side figured out
> which menus to deliver that to locally...

We kinda need to stay with as much as possible the events that already
exist. Adding a new program flow through all the various dbusmenu
implementations would be difficult and risky.

> The potential race caused by a "new submenu has appeared" signal
> crossing the bus at the same time as the "show all the submenus"
> method call presents an annoying problem. The API would rather have
> to be more like "enter into the always-showing mode" and "leave the
> always-showing mode" so that new items that got added on the client
> side would automatically have 'open' delivered to them without
> additional intervention from the HUD.

Most of the race issues in dbusmenu are resolved by version numbering
the layout tree. So that wouldn't really be an issue. Clients can even
recover from missing dbus messages in case of buffer overflows.

  --Ted