> On Dec. 2, 2015, 3:15 p.m., Boudhayan Gupta wrote: > > src/platformtheme/kdemactheme.mm, line 39 > > <https://git.reviewboard.kde.org/r/126198/diff/3/?file=420466#file420466line39> > > > > This makes me very nervous. > > > > Using private APIs is almost always a guarantee the application won't > > preserve binary compatibility even across point releases (eg Qt 5.5.0 - > > 5.5.1), let alone across major releases. > > > > What makes it even more dangerous is that this file won't be built only > > on MacPorts/Fink but also by people making normal AppBundle releases - > > where you have no control over what versions of dependencies are being used. > > > > Please try to find a public API alternative, even if it ends up being a > > giant hack. I once found a very elegant private API solution to making a > > QQuickWidget emit a release event on the mouse cursor, but just because of > > the whole binary compat problem (some Linux distros don't even ship apps > > depending on private headers) I had to create a dummy item inside the > > QtQuick code and interact with it to get the cursor released. Giant hack, > > but at least no private API usage. > > > > Private API is **private** for a reason. > > Luigi Toscano wrote: > Afaik frameworkintegration is supposed to be an extension of the internal > Qt world and it has already been the case (I asked the same question few > Akademys ago). If you want to use QPA, you need to go down in the stack. > > Martin Gräßlin wrote: > yeah frameworkintegration is bound to use private API. So not really a > problem in this special case. > > AFAIK some distros have adjusted the packaging to force a recompile for > new Qt releases. > > Boudhayan Gupta wrote: > Ah, that makes sense. Okay, I'm dropping this issue. > > René J.V. Bertin wrote: > Whew - Hacking is so much more fun if you can do it there where you're > not supposed to go, rather than to avoid exactly that ;) > However, it might be possible to replace the direct call of the private > function by a call through a function pointer to `createPlatformTheme` > obtained through a dynamic resolver. Any idea if that is bound to fail > through a Qt function because it'd detect a request for a private API? > > In any case, you (Boudhayan) are right that this modified framework might > end up being used also in *standalone* AppBundle builds rather than only in > builds using shared resources (in which applications are also built as app > bundles!). At least I hope it will, for the very reason I've been pushing the > patch. But I think you're wrong to assume that using private APIs is a > problem there. If anything it should be (much) less the case, because > standalone app bundles form a much more protected environment. They're not > meant to be upgraded internally, except maybe by some special mechanism > controlled by the developer(s) in charge. Updates to Qt without an > accompanying forced rebuild ("revbump" in MacPorts speak) to > frameworkintegration are much more likely to occur in a shared environment. > > Boudhayan Gupta wrote: > There's no reason to go through a dynamic function resolver if the rest > of FWI uses private APIs happily. Luigi says, "frameworkintegration is > supposed to be an extension of the internal Qt world" - in that case it makes > perfect sense to make use of private APIs to get your job done. > > As for the AppBundles point - I'm thinking if it's possible Qt is > installed from e.g. the SDKs provided by qt.io and standalone AppBundles end > up depending on that. If not, using private APIs in standalone AppBundles > makes no difference - you won't upgrade Qt without upgrading the entire > AppBundle. > > René J.V. Bertin wrote: > Dynamic resolution appears out of the question anyway, because the > createPlatformTheme function is not a static member function. There's a > `static QCocoaIntegration *instance()` (`_ZN17QCocoaIntegration8instanceEv`), > though, which should return `QGuiApplicationPrivate::platform_integration`. > Regardless of whether it's acceptable to use a private API here, there is > still the question of handling a conflict/mismatch situation in a sensible > way, one in which it's apparent immediately what went wrong. > But maybe there's an easier way to do that, simply by comparing a > hard-coded Qt version with the runtime version. > > Standalone app bundles are by definition not dependent on anything but > system libraries, that's the whole idea behind them. When signed (e.g. > because shipped through the App Store) you can also not modify them because > if you do they won't launch. Of course you can build partially standalone app > bundles, but I think that most devs will either aim for truly standalone > products, or else product that share as much as possible (in which case Qt + > KF5 frameworks could be built as a composite framework bundle).
Don't complicate code too much unless you really, really need it :-) Simpler code has less bugs and is easier to maintain. In this case, it'll just be easier to make FWI depend on an exact version of Qt than to do a dynamic resolver. - Boudhayan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126198/#review89026 ----------------------------------------------------------- On Dec. 2, 2015, 1:59 a.m., René J.V. Bertin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126198/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2015, 1:59 a.m.) > > > Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie > Zimmerman. > > > Repository: frameworkintegration > > > Description > ------- > > The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to > Qt5; all that is required is to include the `qgenericunixservices` and > `qgenericunixthemes` components in the build, and to append `"kde"` to the > list returned by `QCocoaIntegration::themeNames()` for instance under the > condition that `KDE_SESSION_VERSION` is set to a suitable value in the > environment. > > This will allow KF5 and Qt5 applications to use the theme selected through > KDE if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, > which seems like a sufficiently specific set of conditions that is easy to > avoid by users who prefer to use the Mac native theme. > > While requestion the KDE theme is also possible through `-style kde` or `env > QT_STYLE_OVERRIDE=kde` the use of the KdePlatformTheme plugin appears to be > the only way to get the full theme, including the font and colour selection. > In my opinion it is above all the font customisation which is a very welcome > feature for Qt/Mac; by default Qt applications use the default system font > (Lucida Grande 13pt or even 14pt) throughout. This is a good UI font, but not > at that size (and most "native" OS X applications indeed use a range of > smaller sizes, depending on role. > > It does have introduce a number of regressions, which the current patch aims > to address. The most visible and problematic of these regressions is the loss > of the Mac-style menu bar and thus of all menu items (actions). > > The fix is straightforward : on OS X (and similarly affected platforms?), an > instance of the native Cocoa platform theme is created through the private > API, and used as a fallback rather than immediately falling back to the > default implementations from `QPlatformTheme`. In addition, methods missing > from (not overridden by) `KdePlatformTheme` are provided on OS X and call the > corresponding methods from the native theme. It is this change which restores > the menubar and even the Dock menu functionality. > One minor regression remains but should be easy to fix (elsewhere?): the > Preferences menu loses its keyboard shortcut (Command-,). > > Given the fallback nature of the native platform instance I have preferred to > print a warning rather than using something like `qFatal`, above all because > the message printed by qFatal tends to get lost on OS X. I can replace my use > of `qWarning` with a dialog giving the choice between continuing or exiting > the application - code that would be called in the menu methods because only > there is it certain that the application actually needs a menubar. > > In line with experience and feedback from the KDE(4)-Mac community I have > decided to force the use of native dialogs rather than the ones from the > KdePlatformPlugin. > > In addition I set the fallback value for `ShowIconsOnPushButtons` to false in > line with platform guidelines, and ensure that the autotests are not built as > app bundles. > > > Diffs > ----- > > autotests/CMakeLists.txt 7c2129c > src/kstyle/CMakeLists.txt bc26667 > src/kstyle/kstyle.mm PRE-CREATION > src/platformtheme/CMakeLists.txt 23f590e > src/platformtheme/kdemactheme.h PRE-CREATION > src/platformtheme/kdemactheme.mm PRE-CREATION > src/platformtheme/khintssettingsmac.h PRE-CREATION > src/platformtheme/khintssettingsmac.mm PRE-CREATION > src/platformtheme/main_mac.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126198/diff/ > > > Testing > ------- > > On Mac OS X with Qt 5.5.1, KF5 frameworks 5.16.0 and QtCurve git/head. > > I have not verified to what extent my use of a private `QGuiApplication` API > links builds to a specific Qt version (I consider that nothing shocking and a > minor price to pay). > >>> Do I need to add some glue to the CMake file so that it will warn if the > >>> private headerfiles are not available? Apparently no changes were > >>> required to find them. > > > File Attachments > ---------------- > > purely native OS X theme > > https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/650d0da7-52b3-40d1-a1f9-cb610494cf77__Screen_Shot_2015-11-30_at_15.42.31.png > native theme but with `-style kde` > > https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/72e2a6aa-2a7c-465b-b404-fc1e52b6fc69__Screen_Shot_2015-11-30_at_15.43.02.png > using the KDEPlatformTheme > > https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/309e5995-74fa-42fb-a6f3-936cedbf5246__Screen_Shot_2015-11-30_at_15.43.31.png > on Linux, using a purely "native" theme > > https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/eaa1d907-bf05-4ca2-821b-83dc062aea04__QtCreatorNativeLNX.png > KDEPlatformTheme with the "macintosh" native theme selected > > https://git.reviewboard.kde.org/media/uploaded/files/2015/12/01/de55a91f-3500-4db8-8a3b-d252fd7ea169__Screen_Shot_2015-12-01_at_13.52.35.png > > > Thanks, > > René J.V. Bertin > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
