Review Request 121390: make Qt5 theme build on Linux again

[René J.V. Bertin]

> On Dec. 8, 2014, 3:07 p.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.

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


- René J.V.


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


On Dec. 8, 2014, 2:38 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 Dec. 8, 2014, 2:38 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/20141208/1b87d7a2/attachment.htm>