Review Request 121390: make Qt5 theme build on Linux again

Sun Mar 1 20:38:17 GMT 2015

> On 十二月 8, 2014, 9:07 a.m., Martin Gräßlin wrote:
> > this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be:
> > #if HAVE_X11
> > // x11 specific stuff
> > #endif
> > 
> > obviously it also needs a runtime check:
> > if (QX11Info::isPlatformX11())
> > 
> > as we no longer should assume that X11 is the only platform on Unix(non OSX).
> 
> René J.V. Bertin wrote:
>     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.
>     
>     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 `Q11Info::isX11Active()` (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.)
> 
> Martin Gräßlin wrote:
>     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.
>     
>     Concerning the runtime check:
>     kwrite -platform wayland
>     
>     and your app is going to crash like hell if there is no runtime checks.
> 
> René J.V. Bertin wrote:
>     ```> 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".
>     
>     Available platform plugins are: linuxfb, minimal, offscreen, xcb.
>     
>     Reinstalling the application may fix this problem.
>     Abort (core dumped)
>     ```
>     
>     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.
>     
>     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.
> 
> Thomas Lübking wrote:
>     > Right, so with runtime checks it doesn't crash, it just self-destructs. 
>     
>     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 *can* 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.
>     
>     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.
> 
> Martin Gräßlin wrote:
>     for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/
>     
>     Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using.
> 
> René J.V. Bertin wrote:
>     @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. *)
>     
>     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.
>     
>     *) or else I'll start making a ruckus to have kwin and more Plasma goodies on OS X!! ;)
> 
> Martin Gräßlin wrote:
>     Yes, it's not about compile time checks, it's about introducing runtime checks as Thomas and I wrote ;-)
> 
> René J.V. Bertin wrote:
>     Actually, Thomas wrote "The compile time checks have no implication on the runtime". Surely a typo, but those can have devastating consequences around code ;)
> 
> René J.V. Bertin wrote:
>     (published too fast again)
>     
>     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 ...
>     
>     And I can't stop thinking something that has been stamped into me: "ifs are expensive". Guess that shows my age ...
> 
> Thomas Lübking wrote:
>     That's not a typo. Meaning distorting partial quote.
>     I wrote:
>     "The compile time checks have no implication on the runtime *environment*."
>     
>     "Ifs are expensive" might be stamped into your mind and/or true, but they're completely inavoidable in this context.
>     
>     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) ;-)
> 
> René J.V. Bertin wrote:
>     yes, I know I didn't copy the last word of your statement. That doesn't change the fact that your 2nd word was *compile* instead of *run*, 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.
>     
>     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.)
>     
>     > "Ifs are expensive" might be stamped into your mind and/or true, but they're completely inavoidable in this context.
>     
>     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)

Sorry somehow my filter missed this review request and I've just seen it today...........

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 `libqtcurve-utils` 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).

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 `Display` 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.

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.

- Yichao

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

On 十二月 8, 2014, 4:59 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121390/
> -----------------------------------------------------------
> 
> (Updated 十二月 8, 2014, 4:59 p.m.)
> 
> 
> Review request for KDE Frameworks, Qt KDE and Yichao Yu.
> 
> 
> Repository: qtcurve
> 
> 
> Description
> -------
> 
> 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.
> 
> This patch restores building on Unix/X11 by replacing
> 
> `#ifdef Q_WS_X11`
> 
> with
> 
> `#if defined(Q_OS_UNIX) && !defined(Q_OS_OSX)`
> 
> please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?)
> 
> 
> Diffs
> -----
> 
>   qt5/style/blurhelper.cpp 5dcc95c 
>   qt5/style/qtcurve.cpp 7b0d7e6 
>   qt5/style/qtcurve_plugin.cpp febc977 
>   qt5/style/qtcurve_utils.cpp 728c26a 
>   qt5/style/shadowhelper.cpp a239cf1 
>   qt5/style/utils.cpp 0680442 
>   qt5/style/windowmanager.cpp 3c2bc1c 
> 
> Diff: https://git.reviewboard.kde.org/r/121390/diff/
> 
> 
> Testing
> -------
> 
> On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150301/b578bf88/attachment.htm>