[Xcb] EWMH API for XCB

[Arnaud Fontaine]

>>>>> "Jamey" == Jamey Sharp <jamey at minilop.net> writes:

Hi,

Thanks for reviewing it Jamey.

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

Well, I think  it's better to keep it within  xcb-util. For instance, if
all the libraries provided in xcb-util  would have not been shipped as a
bundle, I think they would be  in a much worst shape, mainly because the
original author of  the code is rarely the maintainer  on a long stretch
of time. Keeping it as a bundle ensures that even if the author does not
have time anymore to maintain his  code, someone else will take over the
maintenance and  update it  when necessary (this  is what  happened with
xcb-util/icccm at  least).  Moreover, xcb-util/icccm  and xcb-util/image
are  already   used  in   other  projects,  but   are  still   parts  of
xcb-util.

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

When adding the following macro:
#define BUILD_BUG_OR_ZERO(e) ((void) (sizeof(struct { int:-!!(e); })))

And the following line before the memcpy:
BUILD_BUG_OR_ZERO((data_len >> 2) > 5);

I get the following error I can't get rid of:
ewmh.c:563: error: bit-field ‘<anonymous>’ width not an integer constant

I'm  a bit  confused  by this  macro  and does  not understand  anything
besides of  the basic principle. I  was thinking about  simply adding an
assertion otherwise.

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

Thanks, I have just committed it[0].

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

Actually, it's  the caller who takes care  of xcb_ewmh_connection_t. For
instance, a window manager managing two displays would simply initialize
xcb_ewmh_connection_t by calling xcb_ewmh_init_atoms() twice. I think it
can't be done otherwise, because the Atoms are specific to a display and
so  do  xcb_connection_t.  That's  why  xcb_ewmh_connection_t  has  been
defined,  only to avoid  extra parameters  such as  xcb_connection_t and
EWMH Atoms in each function of the API.

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

Hope  that's not  too  ugly[1]  ;). Concerning  discarding  the rest  of
InternAtom replies  if one  fails, I have  just requested  the remaining
replies anyway.

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

Why doesn't it look right to you? 

    >  At  least,  the  comment  header  is  missing  a  "T".   :-)  And
    >  _NET_WM_SYNC_REQUEST_COUNTER  is mentioned  only  in the  comment
    > header.