> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
