<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/121390/">https://git.reviewboard.kde.org/r/121390/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On 十二月 8th, 2014, 9:07 a.m. EST, <b>Martin Gräßlin</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;">this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be:</p>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">if HAVE_X11</h1>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// x11 specific stuff</p>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">endif</h1>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">obviously it also needs a runtime check:
if (QX11Info::isPlatformX11())</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">as we no longer should assume that X11 is the only platform on Unix(non OSX).</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 9:35 a.m. EST, <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 found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform "is" X11, though. It's known at build time if a platform "is" X11. Doing a runtime check before each theming action if <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Q11Info::isX11Active()</code> (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed...
(In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.)</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 9:57 a.m. EST, <b>Martin Gräßlin</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;">HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Concerning the runtime check:
kwrite -platform wayland</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and your app is going to crash like hell if there is no runtime checks.</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 10:39 a.m. EST, <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;">```> neon5-exec /opt/project-neon5/bin/kwrite -platform wayland
This application failed to start because it could not find or load the Qt platform plugin "wayland".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Available platform plugins are: linuxfb, minimal, offscreen, xcb.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Reinstalling the application may fix this problem.
Abort (core dumped)
```</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :)
Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support.
To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely.</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 10:47 a.m. EST, <b>Thomas Lübking</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Right, so with runtime checks it doesn't crash, it just self-destructs. </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You're missing the point entirely. The compile time checks have no implication on the runtime environment.
Of course you cannot use the wayland platform plugin if it's not available, but you <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">can</em> compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application -> thus you must check whether you're operating on X11/xdg at runtime.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also testing for "UNIX but not OSX" to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all.</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 10:58 a.m. EST, <b>Martin Gräßlin</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;">for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using.</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 11:11 a.m. EST, <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;">@Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)).
I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable display/rendering engine present doesn't mean that indidual KDE applications or plugins can no longer decide to support only a subset to be set at build time. *)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No issue either with "Unix but not OS X" - that's what I came up with for lack of something better. Turns out Yichao has his own alternative to HAVE_X11, I'll see if I can make do with that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">*) or else I'll start making a ruckus to have kwin and more Plasma goodies on OS X!! ;)</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 11:28 a.m. EST, <b>Martin Gräßlin</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;">Yes, it's not about compile time checks, it's about introducing runtime checks as Thomas and I wrote ;-)</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 12:51 p.m. EST, <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;">Actually, Thomas wrote "The compile time checks have no implication on the runtime". Surely a typo, but those can have devastating consequences around code ;)</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 12:54 p.m. EST, <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;">(published too fast again)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Actually, that blog post of yours also starts out by talking exclusively about compile-time checks for about 2/3 of its length. It's only after the screenshot that it becomes clear you actually use the compile-time check to include a runtime-check or not. A casual reader might be tempted to stop reading early, thinking he got the message ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And I can't stop thinking something that has been stamped into me: "ifs are expensive". Guess that shows my age ...</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 1:33 p.m. EST, <b>Thomas Lübking</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;">That's not a typo. Meaning distorting partial quote.
I wrote:
"The compile time checks have no implication on the runtime <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">environment</em>."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Ifs are expensive" might be stamped into your mind and/or true, but they're completely inavoidable in this context.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Just that X11 was available at runtime does NOT ("no more w/ Qt5") mean that it's available at runtime.
=> Keep the branching out of hot loops (as always) ;-)</p></pre>
</blockquote>
<p>On 十二月 8th, 2014, 4:33 p.m. EST, <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;">yes, I know I didn't copy the last word of your statement. That doesn't change the fact that your 2nd word was <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">compile</em> instead of <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">run</em>, in a context where you (at least) seemed to be saying that I apparently claimed that those (= compile time) checks had an impact on runtime performance.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, yes, I understood perfectly well that X11 might not be available at runtime while it was when compiling, and that an application trying to do X11 calls will exit with an error when trying to connect to an inexisting X11 server. (Or crash if X11 was actually uninstalled ... but it would take other runtime checks to protect against that, and frankly that'd just be crazy.)</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Ifs are expensive" might be stamped into your mind and/or true, but they're completely inavoidable in this context.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not true, see my remarks about using function pointers above. Not that that would be particularly clever and less expensive when X11 is the only platform that provides a certain functionality ... :)
(I do seem to recall that using function pointers instead of normal functions was hardly more expensive on x86)</p></pre>
</blockquote>
</blockquote>
<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;">Sorry somehow my filter missed this review request and I've just seen it today...........</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To answer Martin's concern, I totally agree and it's in my mind the first time I added X11 support back to the Qt5 version. The X11 related stuff in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">libqtcurve-utils</code> have also always had that check. All X11 related functions are guarded by both a compile time check (e.g. if libxcb/X11 is not found or somehow the user simply don't want to link to them...) and a runtime check (i.e. the X11 related functions are no-ops if X11 is not initialized first at runtime).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now (AFAIK) the compile failure on OSX seems to be related to some sturture name conflict (or whatever it is that causes a forward declaration of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Display</code> not working...). The real issue is already addressed in another review request and it is not necessary to disable calls to X11 related functions (which might be no-ops) on OSX anymore.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In any case, the issue related to this request should already be resolved now and the status is also monitored on build.kde.org (and AFAIK both Qt4 and Qt5 versions build successfully now). I think this review request can be discarded.</p></pre>
<br />
<p>- Yichao</p>
<br />
<p>On 十二月 8th, 2014, 4:59 p.m. EST, 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 Frameworks, Qt KDE and Yichao Yu.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated 十二月 8, 2014, 4:59 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
qtcurve
</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;">Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch restores building on Unix/X11 by replacing</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#ifdef Q_WS_X11</code></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">with</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#if defined(Q_OS_UNIX) && !defined(Q_OS_OSX)</code></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?)</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 KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment.</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>qt5/style/blurhelper.cpp <span style="color: grey">(5dcc95c)</span></li>
<li>qt5/style/qtcurve.cpp <span style="color: grey">(7b0d7e6)</span></li>
<li>qt5/style/qtcurve_plugin.cpp <span style="color: grey">(febc977)</span></li>
<li>qt5/style/qtcurve_utils.cpp <span style="color: grey">(728c26a)</span></li>
<li>qt5/style/shadowhelper.cpp <span style="color: grey">(a239cf1)</span></li>
<li>qt5/style/utils.cpp <span style="color: grey">(0680442)</span></li>
<li>qt5/style/windowmanager.cpp <span style="color: grey">(3c2bc1c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121390/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>