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