[Xcb] EWMH API for XCB

[Jamey Sharp]

On Wed, Dec 16, 2009 at 01:14:47AM +0100, Arnaud Fontaine wrote:
> >>>>> Jamey Sharp <jamey at minilop.net> writes:
>     >> 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.

That makes sense.

> Therefore, I have just added an assert()[0]. I think it's ok this way,
> isn't it?

Josh's explanation of the difficulties with asserts aside, it's fine
with me. :-)

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

A nice way to test a multi-screen setup is with Xnest or Xephyr, which
let you configure several screens that all show up as windows on your
real screen.

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

Cool, I like this much better! I hope passing the screen number to all
those calls doesn't make using this code too painful?

Minor further suggestions, in xcb_ewmh_init_atoms:

You don't need to count the number of screens. You have a bunch of
options, but since you're getting an iterator over them, I'd just set
ewmh->nb_screens = screen_iter.rem before using the iterator in any
loops. Or if you want your code to be more self-explanatory, set
ewmh->nb_screens = xcb_setup_roots_length(setup).

Each entry in wm_cm_sn is used only once, so I'd declare it inside the
loop as a single buffer that you reuse each time, instead of using C99's
variable-length arrays (VLA). It's nice that C99 added VLA support, but
you don't need it here, and last I checked GDB did a bad job debugging
functions with VLAs in them.

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

Nicely done. ;-) Looks right to me now, and the history clearly shows
that it was right all along. ;-)

>     >> > Do you really want it in the xcb-util bundle though?
> 
> 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?

I'm not as concerned as you are about consistency across different
projects, but I think there's a lot of value in documenting "best
practices" for setting up these projects. I'd completely approve of
additions to the wiki about these topics, but of course it's a wiki, so
you don't need my approval anyway. :-)

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

Thanks for your hard work. :-) This is cool!

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/20091215/962c6529/attachment.pgp