<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/116787/">https://git.reviewboard.kde.org/r/116787/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 13th, 2014, 10:31 p.m. 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;">did you try to make it not use QWidget anymore? There shouldn't be a reason why it is QWidget.</pre>
</blockquote>
<p>On March 14th, 2014, 9 a.m. UTC, <b>Aaron J. Seigo</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;">No; I can try it as a non QWidget ... let's see how that goes.</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;">So, challenges a plenty :)
First, this line in NETEventFilter's ctor:
(void) qApp->desktop();
Can result in the creation of a window, which triggers an assert. Removing that line (besides probably breaking root window event handling in some cases) simply pushes the problem to kwindowinfo_x11.cpp:54:
m_info.reset(new NETWinInfo(QX11Info::connection(), _win, QX11Info::appRootWindow(), props, 2));
appRootWindow() does the same if the root window is not yet instantiated. When it is, then there's no problem and KWindowInfo works fine from non-main-app threads. So qApp->desktop() *must* be called from the main app thread somewhere.
Second: I'm assuming that XFixesSelectSelectionInput requires an actual window and not just an xcb_window_t that doesn't have a window associated with it. If that assumption is correct (and I don't know for certain, but some simple testing I did seems to support that) then we still have the same issue: Qt asserts when xcb_create_window is called outside the main application thread. So the use of XFixesSelectSelectionInput becomes a sticking point.
Thirdly, there is the complication that KWindowSystemPrivateX11 is created in KWindowSystemStaticContainer which is, as the name suggests, a global static object. Care needs to be paid that no event handling is assumed to happen in the thread of its creation. Reading the code, it appears that all event handling (and therefore requirement for an event loop) happens in the thread that KWindowSystem lives in .. which is fine except for KWindowSystem::self() which should be as easy to fix as adding this to KWindowSystemStaticContainer():
kwm.moveToThread(QCoreApplication::instance()->thread());
ensuring that the static KWindowSystem always lives in the long-lived main application thread. This is exactly the sort of case I wrote about in my blog recently: creating internal (to the library) static objects with application lifespan which require an event loop in whatever thread happens to call them is broken. So that at least needs doing. I have patched this here and it works well.
So there is code in NETEventFilter that *must* be run in the man app thread even without the use of QWidget. .. updated patch coming. </pre>
<br />
<p>- Aaron J.</p>
<br />
<p>On March 13th, 2014, 6:44 p.m. UTC, Aaron J. Seigo wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks, kwin and Martin Gräßlin.</div>
<div>By Aaron J. Seigo.</div>
<p style="color: grey;"><i>Updated March 13, 2014, 6:44 p.m.</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;">When using KWindowInfo from a thread other than the main application thread, the application fails on an assert triggered by creating a QWidget in another thread. This is due to NETEventFilter being a QWidget. This patch addresses the issue by using a QObject to create the NETEventFilter, which is itself first moved to the main application thread. </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;">Used in the Sprinter Windows plugin and works lovely to find desktops, windows, etc.</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>src/kwindowsystem_p_x11.h <span style="color: grey">(75f3028)</span></li>
<li>src/kwindowsystem_x11.cpp <span style="color: grey">(95c396b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116787/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>