> 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) > > Yichao Yu wrote: > 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. > > Marko Käning wrote: > Just for the record, QtCurve currently fails to build on OSX/CI: > ```` > > /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/qt5/config/qtcurveconfig.cpp:1085: > Warning: Macro argument mismatch. > In file included from > /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/lib/utils/dirs.cpp:22: > /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/lib/utils/dirs.h:68:1: > error: implicit instantiation of undefined template > 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1:: > allocator<char> >' > getConfFile(const std::string &file) > ^ > ```` > Shall I send the full build log of this failure to you via PM? > > Marko Käning wrote: > For completeness: I haven't THIS RR applied to my OSX/CI system as of > now. SHOULD I, perhaps???
Just realized that I didn't include `<string>` in that file although aparently it is pulled in by sth else on Linux. Can you check if it's working now? If not, a full log would be helpful. P.S. (OT), what's up with `thread_local` support on OSX? Have you got it working or do I have to use pthread's tls instead? It's not that hard but I'll be a little supprised if Clang doesn't support it on OSX.... P.P.S. Is it possible for me to access the OSX/CI results? - 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 > >
