Review Request 116787: Always create NETEventFilter (a QWidget subclass) in the main application thread

[Aaron J. Seigo]

> On March 13, 2014, 10:31 p.m., Martin Gräßlin wrote:
> > did you try to make it not use QWidget anymore? There shouldn't be a reason why it is QWidget.
> 
> Aaron J. Seigo wrote:
>     No; I can try it as a non QWidget ... let's see how that goes.

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. 


- Aaron J.


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


On March 13, 2014, 6:44 p.m., Aaron J. Seigo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116787/
> -----------------------------------------------------------
> 
> (Updated March 13, 2014, 6:44 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> 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. 
> 
> 
> Diffs
> -----
> 
>   src/kwindowsystem_p_x11.h 75f3028 
>   src/kwindowsystem_x11.cpp 95c396b 
> 
> Diff: https://git.reviewboard.kde.org/r/116787/diff/
> 
> 
> Testing
> -------
> 
> Used in the Sprinter Windows plugin and works lovely to find desktops, windows, etc.
> 
> 
> Thanks,
> 
> Aaron J. Seigo
> 
>

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