[Xcb] EWMH API for XCB

[Arnaud Fontaine]

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

Hi,

    > Yeah, the data_len parameter in xcb_ewmh_send_client_message isn't
    > a constant,  so it can't be checked at compile  time. I meant that
    > you'd add something like this in every caller:

    > BUILD_BUG_ON(sizeof data > sizeof ((xcb_client_message_event_t *)
    > 0)->data);

    > Unlike an  assertion, this doesn't generate any  code; instead the
    > compiler  can prove  that all  those calls are  safe, but  only if
    > you've  checked all the callers,  which is why  I suggested making
    >     this    function    static.     Also,    you     could    wrap
    > xcb_ewmh_send_client_message up in a  macro that does this, if you
    >  wanted.

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

    > Absolutely, that would be better than nothing.

Well, I think  it would be better to  not make it static as  it could be
useful to any program implementing extensions to EWMH. Therefore, I have
just added an assert()[0]. I think it's ok this way, isn't it?

    >  I'm not talking  about different  connections. I'm  talking about
    >  different screens  on  the  same connection.  All  the atoms  are
    > guaranteed to be the same in that case, except for _NET_WM_CM_Sn.

Oh  ok, I  didn't think  at all  about that  as I  have never  used such
configuration. Therefore, I removed root member of xcb_ewmh_connection_t
in favor  of screens  and nb_screens.  I  added a parameter,  namely the
screen  number, to all  the functions  using the  root window.   Also, I
modified _NET_WM_CM_Sn  which is now  per-screen (xcb_atom_t *).  I have
committed it there[1].

    > The  patch looks good to me,  except that I think  it's wrong. :-)
    > You're assigning an XID  to a dereferenced pointer of type "char",
    >  as far as  I can  tell. I  think you  need one  more cast  on the
    > left-hand side, something like this:

    > *(xcb_atom_t *) ((char *) ewmh + ewmh_atoms[i].m_offset) =
    > reply->atom;

Hrm ok, oops... I have  silently and discretly edited my previous commit
to hide that :).

    >> That's why I have only defined accessors for
    >> _NET_WM_SYNC_REQUEST_COUNTER.

    > Yes, all your functions are named *_request_counter, but they all
    > use
    ewmh-> _NET_WM_SYNC_REQUEST. Isn't that wrong?

    > And the comment header still has that typo of missing a "T". ;-)

I did not see it at all. Thanks much! I have just committed it[1].

    >> > Do you really want it in the xcb-util bundle though?
    >> 
    >> Well, I think it's better to keep it within xcb-util.

    >  I'd be  interested in  hearing  your thoughts  on the  discussion
    > Julien and I had today on this topic.

    >  I  think the  history  of  free  software contradicts  this.  For
    > example, bundling  lots of code into libX11  certainly didn't help
    >  it be  maintained any  better. :-)  Conversely, lots  of projects
    >  abandoned  by their  original  authors  have  been picked  up  by
    >  somebody else  who needed  them, despite  not being  bundled with
    > anything else.

    >> Moreover,  xcb-util/icccm and xcb-util/image are  already used in
    >> other projects, but are still parts of xcb-util.

    > Yes, so are aux, and render-util I think, and awesome is using the
    > property and event libraries  apparently. And yes, I'd like all of
    > those  split out,  but just because  that hasn't  happened doesn't
    > mean we should add anything more to the mess.

I think it  does make sense and  I will put it in  a separate repository
then. However, I think it would  be good to share at least configure.ac,
coding style (I  have not found anything like that  except by looking at
existing  code but  it's  pretty unconsistent  ATM  AFAIK), and  Doxygen
configuration in  order to  have something consistent  across libraries,
even though I'm  not sure how it could be achieve.   Maybe a document on
the  wiki giving  some templates  such as  Doxygen and  configure.ac and
another one for the coding style as it's more general?

Thank you  very much for all  your comments. The code  looks much better
now!

Cheers,
Arnaud

[0] http://cgit.freedesktop.org/~arnau/xcb-util/commit/?h=xcb-ewmh&id=d9fa9092f0b7dadef0797a01b301605c7d35984e
[1] http://cgit.freedesktop.org/~arnau/xcb-util/commit/?h=xcb-ewmh&id=d301f9210a3594829451e4559644747c1f88444a
[2] http://cgit.freedesktop.org/~arnau/xcb-util/commit/?h=xcb-ewmh&id=9e76acc5599026d22c329e10c4e9a14ada90900d