> 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
> 
>

_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Reply via email to