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

[David Faure]

> On Sept. 16, 2015, 4:23 p.m., Thomas Lübking wrote:
> > would one want to use the occasion to get rid of the bazillion variables mapped to a struct array and have atom(Atom a) and atomString(Atom a) for an array of atoms a matching string - maybe including some namespaces so that instead of p->atom->net_wm_to_many_underscores one would have atom(NET::WM::NoUnderscores) and atom(WM::NoUnderscores) etc.?
> > 
> > Ideally with a little preprocessor and include magic?
> > http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c
> 
> Martin Gräßlin wrote:
>     rather orthogonal to the change. I'm not sure whether a huge switch statemement will make it better. What might be more interesting is to get something like KWin's Xcb::Atom in to also only fetch the atoms which are actually used. Though the Atom class is of course considerable larger than just an xcb_atom_t.
> 
> Thomas Lübking wrote:
>     The switch in the demo code is just a usage demo - I'd promise red overweight ;-)
>     Constantly using atom(Atom a) would of course allow a sparse array that mostly contains of "0" and is only filled on demand.
>     
>     The reason why I thought it might be a good idea *now* is because this already requires a HUUUUUGE change that will block git blame.
> 
> Martin Gräßlin wrote:
>     > Constantly using atom(Atom a) would of course allow a sparse array that mostly contains of "0" and is only filled on demand.
>     
>     That sounds of having other disadvantages: round trips as we would need to resolve the atom when needed. Also the SharedDataPointer in the patch would not work as it would detach them on fetch and cause multiple NETWinInfo to catch the atoms again.
>     
>     So overall from the minimize roundtrip perspective fetching all atoms on first run still sounds like the best idea...
> 
> Thomas Lübking wrote:
>     "to get something like KWin's Xcb::Atom in to also only fetch the atoms which are actually used." ... :-P
>     
>     > SharedDataPointer in the patch would not work as it would detach them on fetch
>     
>     For "Atom" i actually meant an enum, not KWin's XCB::Atom - I don't see where this would detach anything, but the roundtrips would oc. occur.
>     One could decide to load a "standard" set on init and the rest only on demand, but you don't win memory by this (just "maybe" a little time on first NETWM invocation)
> 
> Martin Gräßlin wrote:
>     > "to get something like KWin's Xcb::Atom in to also only fetch the atoms which are actually used." ... :-P
>     
>     KWin's Xcb::Atom doesn't cause roundtrips.
>     
>     > I don't see where this would detach anything
>     
>     From QSharedDataPointer documentation "For write access operator ->() will automatically call detach()". My understanding is that if we update the value of an xcb_atom_t then it would detach. Granted one could change to QExplicitlySharedDataPointer.

QSharedDataPointer is used to write value classes, where const methods do not detach, and non-const methods do detach.

It sounds like you wanted to use QSharedPointer instead (refcounting).


- David


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


On Sept. 16, 2015, 1:42 p.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125259/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2015, 1:42 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> 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.
> 
> [autotests] NETWM tests get a new X server per test
> 
> Making use of new feature of allowing multiple X connections:
> start X server in init() instead of initTestCase().
> 
> 
> Diffs
> -----
> 
>   autotests/netrootinfotestwm.cpp e918873a5281f6ecbcc1769de3dadcf8f6222f6a 
>   autotests/netwininfotestclient.cpp a5b10376b943ea914a9521b5c07f9ef13a65d2f1 
>   autotests/netwininfotestwm.cpp a98e12fd690b0250337c7588e1525af1d0dda38c 
>   src/platforms/xcb/netwm.cpp d99a925ad2b99d5e60ab1dafcb01400b4a6a4c93 
>   src/platforms/xcb/netwm_p.h 917a86ed5b6c83f36e73bbc346360b943d457243 
> 
> Diff: https://git.reviewboard.kde.org/r/125259/diff/
> 
> 
> Testing
> -------
> 
> see adjusted unit tests. Also tried it in kwin_wayland
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150917/3ca99f6c/attachment.html>