[Xcb] EWMH API for XCB

[Jamey Sharp]

On Sat, Dec 12, 2009 at 08:48:16PM +0100, Arnaud Fontaine wrote:
> It provides an  API to easily deal with EWMH in  an asynchronous way and
> is  just a  straight implementation  of EWMH  specification  version 1.4
> draft-2[1].  I  will keep  maintaining this library  as it's  needed for
> Awesome and the compositing manager I'm currently working on.

That's great!

Do you really want it in the xcb-util bundle though? Why not manage it
as its own project? I originally intended xcb-util as a collection of
sample, proof-of-concept code, more than anything else. I'd really like
to see somebody split the useful bits out into their own repositories.
"Useful" is probably best defined as "somebody outside of xcb-util is
using this," which would mean your ewmh bits already don't belong. :-)

The XCB mailing list and wiki are great places for discussion of utility
libraries like these, of course, regardless of where they live.

This seems like a perfectly sensible implementation of EWMH to me,
macros and m4 and all. (By all means, do whatever it takes to avoid
copying and pasting code.) I do have a few minor comments:

In xcb_ewmh_send_client_message, you probably should check data_len
before copying data into a fixed-size buffer--unless you want to declare
that function static and ensure that all the callers are safe. If you
want to go the latter route you might look at the BUILD_BUG_ON macro
from include/linux/kernel.h, to get a compile-time check.

Also in xcb_ewmh_send_client_message, I'd encourage using memcpy instead
of a hand-coded loop: It's easier to understand at a glance, and GCC
will generate better code from it.

Huh. So an xcb_ewmh_connection_t is only usable on a single screen, and
you have to pick which one to use when you initialize it? I know X's
concept of screens is becoming pretty obsolete in the RandR era, but if
anything still has to deal with multi-screen setups it'd be window
managers.

xcb_ewmh_init_atoms_replies probably should explicitly discard the rest
of the InternAtom replies if one fails.

I'm a little dismayed that xcb_ewmh_init_atoms_replies gets 82 copies of
four nearly-identical lines of code, even if they are all auto-generated
from a bit of m4. I would be inclined to turn it into a simple loop,
either by casting the address of the first atom to (uint32_t *) and
treating it as an array, or by including
	offsetof(xcb_ewmh_connection_t, $1)
in the internal ewmh_atom_t struct.

The _NET_WM_SYNC_REQUEST section doesn't look right to me, but I don't
know the EWMH spec well enough to figure out why. At least, the comment
header is missing a "T". :-) And _NET_WM_SYNC_REQUEST_COUNTER is
mentioned only in the comment header.

Anyway, nice work!

Jamey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.freedesktop.org/archives/xcb/attachments/20091213/452da106/attachment-0001.pgp