<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126324/">https://git.reviewboard.kde.org/r/126324/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 14th, 2015, 12:23 p.m. IST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/126324/diff/3/?file=421934#file421934line2" style="color: black; font-weight: bold; text-decoration: underline;">src/gui/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">find_package</span><span class="p">(</span><span class="s">Qt5Widgets</span> <span class="o">${</span><span class="nv">REQUIRED_QT_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span> <span class="s">NO_MODULE</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">this introduces a QtWidgets dependency and thus changes the integration level of the framework. I highly recommend to not go this way.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looking at the code there is no reason for this. The usage of QDesktopWidget is not needed as QScreen provides the same. Similar one doesn't need the QWidget usage as QWindow is already there.</p></pre>
</blockquote>
<p>On December 14th, 2015, 3:07 p.m. IST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm all for reducing the number of dependencies, but haven't found another way to get at QWidget::saveGeometry and QWidget::restoreGeometry.
You're probably much more familiar with what those functions really save and restore, and thus to what extent they're essentially convenience functions here for something I could just as well access via QWindow::geometry or QWindow::frameGeometry. I'd have to figure out on my end which of the two I'd need to use because that'd be specific to OS X (knowing there is no QWindow::setFrameGeometry). I won't be able to test that on MS Windows though.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What integration level are you invoking? This dependency doesn't make kconfig a Tier 2 framework, does it? Is it so bad to add a dependency on Qt5Widgets to something that already depends on Qt5Gui?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A more fundamental question would be why this is in KConfig. One could argue that window size (and position) are not application configuration parameters when they're saved automatically; they're a reflection of application interface state (@). Maybe a subtle difference (and maybe a debate that was already held a long time ago), but doesn't this rather (or better) belong in KWindowSystem?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@) Off-topic, but like other state variables saved automatically it might even be wise to save them in a separate file so it's easier to reset state without doing a full "factory reset".</p></pre>
</blockquote>
<p>On December 14th, 2015, 3:10 p.m. IST, <b>Boudhayan Gupta</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In Qt5, a dependency on QtWidgets is the difference between having to use a QGuiApplication (without) and QApplication (with). QtWidgets is a 20MB or so dependency, so in terms of library load times at runtime the difference is somewhat significant.</p></pre>
</blockquote>
<p>On December 14th, 2015, 4:33 p.m. IST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm sensitive to that kind of argument. Interesting btw; my OS X QtWidgets library is 8.6Mb (which should include debug info), but on Linux using the same script for installing Qt it is 97Mb (!). So there is indeed some reason to introduce the extra dependency only on platforms where it's needed, if it's needed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But (something I cannot NOT ask in this context) : just how many applications are there that present a user interface without any need for QtWidgets in any of their dependencies?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The idea is that you can write OpenGL apps without introducing the QtWidgets dependency, by using Qt's event loop and drawing on a QWindow. The KDE Frameworks follow this philosophy - because they're supposed to be used independently of KDE, the frameworks strive to minimise their dependency footprint.</p></pre>
<br />
<p>- Boudhayan</p>
<br />
<p>On December 13th, 2015, 7:24 p.m. IST, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X and KDE Frameworks.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Dec. 13, 2015, 7:24 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In KDElibs4, the KMainWindow::saveWindowSize() and KMainWindow::restoreWindowSize() function saved and restored not only the size but also the position (i.e. the geometry) of windows, using QWidget::saveGeometry and QWidget::restoreGeometry.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2 main reasons for this (according to the comments):
- Under X11 restoring the position is tricky
- X11 has a window manager which might be considered responsible for that functionality (and I suppose most modern WMs have the feature enabled by default?)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Both arguments are moot on MS Windows and OS X, and on both platforms users expect to see window positions restored as well as window size. On OS X there is also little choice in the matter: most applications offer the geometry restore without asking (IIRC it is the same on MS Windows).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would thus like to propose to port the platform-specific code that existed for MS Windows (and for OS X as a MacPorts patch that apparently was never submitted upstreams). I realise that this violates the message conveyed by the function names but I would like to think that this is a case where function is more important.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You may also notice that the Mac version does not store resolution-specific settings. This happens to work best on OS X, where multi-screen support has been present since the early nineties, and where window geometry is restored regardless of the screen resolution (i.e. connect a different external screen with a different resolution, and windows will reopen as they were on that screen, not with some default geometry).
I required I can update the comments in the header to reflect this subtlety.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note that for optimal functionality a companion patch to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KMainWindow::event</code> is required:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #A00000">--- a/src/kmainwindow.cpp</span>
<span style="color: #00A000">+++ b/src/kmainwindow.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -772,7 +772,7 @@ bool KMainWindow::event(QEvent *ev)</span>
{
K_D(KMainWindow);
switch (ev->type()) {
<span style="color: #A00000">-#ifdef Q_OS_WIN</span>
<span style="color: #00A000">+#if defined(Q_OS_WIN) || defined(Q_OS_OSX)</span>
case QEvent::Move:
#endif
case QEvent::Resize:
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This ensures that the window geometry save is performed also after a move (to update the position) without requiring a dummy resizing operation.
Do I need to create a separate RR for this change or is it small enough that I can push it if and when this RR is accepted?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On OS X 10.6 through 10.9 with various KDElibs4 versions and now with Qt 5.5.1 and frameworks 5.16.0 (and Kate as a test application).
I presume that the MS Windows code has been tested sufficiently in KDELibs4; I have only adapted it to Qt5 and tested if it builds.</p></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/gui/CMakeLists.txt <span style="color: grey">(9663e09)</span></li>
<li>src/gui/kwindowconfig.h <span style="color: grey">(48a8f3c)</span></li>
<li>src/gui/kwindowconfig.cpp <span style="color: grey">(d2f355c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126324/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>