Review Request 125309: Support multiple X servers in the NETWM classes

[Martin Gräßlin]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125309/#review86036
-----------------------------------------------------------


hard cost. I now understand why I didn't get what you meant in the other review request ;-) Yeah from code side it looks nicer, though the enum is really hard to get IMHO.


src/platforms/xcb/atoms_p.h (line 1)
<https://git.reviewboard.kde.org/r/125309/#comment59357>

    even if questionable whether there is any copyright on the atom listing: I suggest to add the copyright header.



src/platforms/xcb/atoms_p.h (lines 4 - 20)
<https://git.reviewboard.kde.org/r/125309/#comment59358>

    uff, I just needed a quarter of an hour to understand how that works. Could you please add a comment section, to explain what it does.



src/platforms/xcb/netwm.cpp (lines 299 - 300)
<https://git.reviewboard.kde.org/r/125309/#comment59359>

    shouldn't we undef the ENUM_CREATE_CHAR_ARRAY again?



src/platforms/xcb/netwm_p.h (line 39)
<https://git.reviewboard.kde.org/r/125309/#comment59360>

    I don't like the type name "Atom" as that's also an XLib type. As the actual name is hardly used we could get it to something more precise, maybe NetAtom?


- Martin Gräßlin


On Sept. 19, 2015, 3:22 p.m., Thomas Lübking wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125309/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2015, 3:22 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> alteration review #125259 minus the autotests
> 
> Significant difference is the creation and handling of the Atom enum.
> 
> 1. Net::Utf8 makes no sense
> 2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE (there's nothing such as XA_WM_STATE)
> 3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, KDE_NET_, or ... UTF8, I'm not sure why one would add more enums (since this is internal API and the resolved type will always be xcb_atom_t - in worst case we run into errors on comparing the enums
> 4. I wanted to get rid of the explicit counter variable as well as any loose assignment between atom variable/enum and string.
> 
> -----
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> 
> Diffs
> -----
> 
>   src/platforms/xcb/netwm_p.h 917a86e 
>   src/platforms/xcb/atoms_p.h PRE-CREATION 
>   src/platforms/xcb/netwm.cpp d99a925 
> 
> Diff: https://git.reviewboard.kde.org/r/125309/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150928/8f2e729d/attachment-0001.html>