[KDE/Mac] Review Request 126198: [OS X] adaptations for the KdePlatformTheme

Sat Dec 12 17:45:28 UTC 2015

> On Dec. 7, 2015, 5:41 p.m., René J.V. Bertin wrote:
> > So with the current implementation that uses inheritance, there are 2 classes that have a `delayedDBusConnects` method which uses `QDBusConnection::sessionBus().connect()` to register things on the DBus. Both invoke that method explicitly from their ctor, and in general, with the inheritance/overriding going on I think I might be over-initialising certain things. I have modified the 2 `loadSettings` methods so that they only create new instances when `m_fontsData==0` (initialised in the respective ctors), so that should limit the redundancy.
> > 
> > Still I wonder if this isn't going to lead to DBus name conflicts (if the DBus connect method doesn't disconnect/reconnect)?
> > 
> > I also wonder if the crash-on-exit I just discovered in `kdebugdialog5` (but not in what few example KF5 applications I have at the moment) is related:
> > 
> > ```
> > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> > 0   org.qt-project.QtDBus         	0x000000010355f9b6 QHashNode<QString, QDBusConnectionPrivate::SignalHook>::~QHashNode() + 166 (qgenericatomic.h:90)
> > 1   org.qt-project.QtCore         	0x00000001045b89b9 QHashData::free_helper(void (*)(QHashData::Node*)) + 73 (qhash.cpp:405)
> > 2   org.qt-project.QtDBus         	0x00000001035531a8 QDBusConnectionPrivate::~QDBusConnectionPrivate() + 712 (qhash.h:342)
> > 3   org.qt-project.QtDBus         	0x00000001035535ce QDBusConnectionPrivate::~QDBusConnectionPrivate() + 14 (qdbusintegrator.cpp:1035)
> > 4   org.qt-project.QtDBus         	0x000000010354baf8 (anonymous namespace)::Q_QGS__q_sessionBus::innerFunction()::Cleanup::~Cleanup() + 152 (qdbusconnection.cpp:1079)
> > 5   libsystem_c.dylib             	0x00007fff89e5d7a1 __cxa_finalize + 177
> > ```
> > 
> > When I add some trace statements at relevant locations, I see this on the calling terminal:
> > ```
> > > kdebugdialog5
> > QCoreApplication::arguments: Please instantiate the QApplication object first
> > QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave.
> > KdePlatformTheme::KdePlatformTheme() 0x7fe420e0e2b0
> > KdePlatformTheme::loadSettings() 0x7fe420e0e2b0
> > KFontSettingsData::KFontSettingsData() KFontSettingsData(0x7fe420e15bc0)
> > KHintsSettings::KHintsSettings() KHintsSettings(0x7fe420e15690)
> > KdeMacTheme::KdeMacTheme() 0x7fe420e0e2b0
> > KdeMacTheme::loadSettings() 0x7fe420e0e2b0
> > KFontSettingsData::KFontSettingsData() KFontSettingsData(0x7fe420c94dc0)
> > KFontSettingsDataMac::KFontSettingsDataMac() KFontSettingsDataMac(0x7fe420c94dc0)
> > KHintsSettings::KHintsSettings() KHintsSettings(0x7fe420e2f110)
> > KHintsSettingsMac::KHintsSettingsMac() KHintsSettingsMac(0x7fe420e2f110)
> > KdeMacTheme::createPlatformSystemTrayIcon() 0x7fe420e0e2b0
> > KFontSettingsData::delayedDBusConnects() KFontSettingsData(0x7fe420e15bc0)
> > KFontSettingsData::delayedDBusConnects() KFontSettingsDataMac(0x7fe420c94dc0)
> > KFontSettingsDataMac::delayedDBusConnects() KFontSettingsDataMac(0x7fe420c94dc0)
> > KdeMacTheme::~KdeMacTheme() 0x7fe420e0e2b0
> > KdePlatformTheme::~KdePlatformTheme() 0x7fe420e0e2b0
> > KFontSettingsData::~KFontSettingsData() KFontSettingsData(0x7fe420e15bc0)
> > KHintsSettings::~KHintsSettings() KHintsSettings(0x7fe420e15690)
> > Segmentation fault
> > Exit 139
> > ```
> > 
> > the non-crashes don't call `KdeMacTheme::createPlatformSystemTrayIcon()`, so even if `kdebugdialog5` doesn't actually put up a systray icon/menu there must be some relation between that call and the crash.
> > I'd hope this is just a bug in `kdebugdialog5`, but it doesn't express itself when I don't use the KdePlatformTheme (`KdeMacTheme`)...

That crash turns out completely unrelated, but caused by the fact Qt still unloads plugins at exit. An upstream patch is being evaluated that stops that from happening, and after applying it I no longer see any crashing.

- René J.V.

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

On Dec. 7, 2015, 5:43 p.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. 7, 2015, 5:43 p.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
> -----
> 
>   src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
>   src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
>   src/platformtheme/CMakeLists.txt 23f590e 
>   src/platformtheme/kdemactheme.h PRE-CREATION 
>   src/platformtheme/kdemactheme.mm PRE-CREATION 
>   src/platformtheme/kdeplatformtheme.h 97d09df 
>   src/platformtheme/kdeplatformtheme.cpp 80dbcb7 
>   src/platformtheme/kfontsettingsdata.h 4b92c7d 
>   src/platformtheme/kfontsettingsdata.cpp b0a4bbf 
>   src/platformtheme/khintssettings.h ec064d3 
>   src/platformtheme/khintssettings.cpp 8adf6c5 
>   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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20151212/decff2ab/attachment.html>