<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/125259/">https://git.reviewboard.kde.org/r/125259/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 16th, 2015, 4:23 nachm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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</p></pre>
</blockquote>
<p>On September 17th, 2015, 6:11 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On September 17th, 2015, 7 vorm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The reason why I thought it might be a good idea <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">now</em> is because this already requires a HUUUUUGE change that will block git blame.</p></pre>
</blockquote>
<p>On September 17th, 2015, 9:01 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Constantly using atom(Atom a) would of course allow a sparse array that mostly contains of "0" and is only filled on demand.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So overall from the minimize roundtrip perspective fetching all atoms on first run still sounds like the best idea...</p></pre>
</blockquote>
<p>On September 17th, 2015, 9:31 vorm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"to get something like KWin's Xcb::Atom in to also only fetch the atoms which are actually used." ... :-P</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">SharedDataPointer in the patch would not work as it would detach them on fetch</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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)</p></pre>
</blockquote>
<p>On September 17th, 2015, 10:14 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"to get something like KWin's Xcb::Atom in to also only fetch the atoms which are actually used." ... :-P</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KWin's Xcb::Atom doesn't cause roundtrips.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't see where this would detach anything</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On September 17th, 2015, 10:22 vorm. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QSharedDataPointer is used to write value classes, where const methods do not detach, and non-const methods do detach.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It sounds like you wanted to use QSharedPointer instead (refcounting).</p></pre>
</blockquote>
<p>On September 17th, 2015, 12:01 nachm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It sounds like you wanted to use QSharedPointer instead</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No, the class I added only has const methods.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, but whenever or however you want to delay atom creation, that's done.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It would work for XCB::Atom only because "xcb_atom_t() const" performs a const cast on ::getReply(), so one would cheat "atom(AtomEnum a) const" in exactly the same way - and likely require a mutex in either case.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Afaiu, what you'd prefer in the end was an XCB::Atom-a-like approach, but w/ external m_connection pointer and a</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">union {
xcb_intern_atom_cookie_t cookie;
xcb_atom_t atom;
} u;
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Apple has that [1] (but) it (only) works as the cookie struct consist of only one unsigned integer.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[1] http://www.opensource.apple.com/source/X11libs/X11libs-40/xcb-util/xcb-util-0.3.3/atom/xcb_atom.h</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On September 16th, 2015, 1:42 nachm. UTC, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and kwin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Sept. 16, 2015, 1:42 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwindowsystem
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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().</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">see adjusted unit tests. Also tried it in kwin_wayland</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/netrootinfotestwm.cpp <span style="color: grey">(e918873a5281f6ecbcc1769de3dadcf8f6222f6a)</span></li>
<li>autotests/netwininfotestclient.cpp <span style="color: grey">(a5b10376b943ea914a9521b5c07f9ef13a65d2f1)</span></li>
<li>autotests/netwininfotestwm.cpp <span style="color: grey">(a98e12fd690b0250337c7588e1525af1d0dda38c)</span></li>
<li>src/platforms/xcb/netwm.cpp <span style="color: grey">(d99a925ad2b99d5e60ab1dafcb01400b4a6a4c93)</span></li>
<li>src/platforms/xcb/netwm_p.h <span style="color: grey">(917a86ed5b6c83f36e73bbc346360b943d457243)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125259/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>