Review Request 125915: Drop runtime dependency on QApplication

[Martin Gräßlin]

> On Nov. 2, 2015, 9:04 a.m., Martin Gräßlin wrote:
> > I consider this a very dangerous change. We don't have unit tests for this specific code, it's an unlikely code path to be taken and we won't notice regressions.
> > 
> > Personally I don't see a problem with having a dependency on QApplication. KWindowSystem links against Qt::Widgets that makes it obvious to any user that it requires a QApplication. If we want to make it work with QGuiApplication, then we need to make sure that we also get rid of Qt::Widgets dependency. Otherwise I think we are still in undefined behavior area if someone uses KWindowSystem without a QApplication.
> > 
> > So overall from my side rather a -1 to this change. Risk of breakage too high for little benefit.
> 
> David Edmundson wrote:
>     >Very dangerous
>     
>     It's not that bad, it's a copy and paste from QDesktopWidgetPrivate::_q_updateScreens 
>     
>     >Personally I don't see a problem with having a dependency on QApplication. KWindowSystem links against Qt::Widgets that makes it obvious to any user that it requires a QApplication. 
>     
>     Not that obvious, I made the "mistake". You get to save nearly ~4Mb from QApp -> QGuiApp
>     Also PlasmaCore import is full of KWindowSystem calls which means ksplashapp, sddm-greeter and probably few others are already in the same situation.
>     
>     
>     > then we need to make sure that we also get rid of Qt::Widgets dependency.
>     
>     ++. Espsecially if we think longer term. 
>     Unfortuantely, we can't without an API break. There's a few helper methods where we have a version with wID and a version with QWidget* just to get the wID.

> It's not that bad,

I stay with "very dangerous" without having a single test case for this code.

> ksplashapp, sddm-greeter and probably few others are already in the same situation.

yes, and they all should use QApplication to get QStyle. Otherwise there might be runtime breakage like wrong colours being picked up, etc.

> Unfortuantely, we can't without an API break

I know.

> There's a few helper methods where we have a version with wID and a version with QWidget* just to get the wID.

We can deprecate those and add QWindow variants. But that's about it. No chance to get this changed till KF 6.

But it's not the only usage. There is also usage of QWidget in the library. An example is kxmessages. As long as we internally use QWidget I think it's a sure thing that this is a QWidget library and requires QApplication.


- Martin


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


On Nov. 1, 2015, 11:58 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125915/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2015, 11:58 p.m.)
> 
> 
> Review request for KDE Frameworks and Albert Astals Cid.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> QDesktopWidget was used which is broken for applications using
> QGuiApplication.
> 
> QDesktopWidget is now just a thin wrapper over QScreen so we can use that directly.
> 
> QApplication::activeWindow is ported to QGuiApplication::topLevelWindows
> and then itterating to see if one is active.
> 
> 
> Diffs
> -----
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d287043c24894ca3c29c439c7939b139da055e8 
> 
> Diff: https://git.reviewboard.kde.org/r/125915/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151102/4c92e7cc/attachment-0001.html>