subject:"Re\: Review Request 126198\: \[OS X\] adaptations for the KdePlatformTheme"



> On May 17, 2016, 4:37 p.m., Martin Gräßlin wrote:
> > given that Plasma integration got removed I suggest to create a dedicated 
> > osx-integration repository for it as well.
> 
> René J.V. Bertin wrote:
> I quite like the way things are, but yes, I've played with that idea. 
> 
> I guess I can simply fork `frameworkintegration`, change the project name 
> in the toplevel CMake file and strip everything except for the platformtheme.
> 
> How does this work from that point onwards?

> I guess I can simply fork frameworkintegration, change the project name in 
> the toplevel CMake file and strip everything except for the platformtheme.

That's kind of how I did it for plasma-integration with git filter-tree.

> How does this work from that point onwards?

My suggestion would be to create something like an "OSX integration" project in 
the metadata and have then all your repositories grouped there including the 
new platform theme.


- Martin


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


On May 16, 2016, 9:49 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 May 16, 2016, 9:49 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, 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
> -
> 
>   CMakeLists.txt 6ea1cba 
>   src/CMakeLists.txt 1c54eb2 
>   src/platformtheme/CMakeLists.txt PRE-CREATION 
>   src/platformtheme/config-platformtheme.h.cmake PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.sh PRE-CREATION 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme



> On May 17, 2016, 4:36 p.m., Martin Gräßlin wrote:
> > > I saw that one of the justifications for the move was a dependency on 
> > > Plasma library/ies, but I don't see any such dependencies in the latest 
> > > version before the move (commit 07548ac1fe7a7fb31a941473911e982fb623c07d).
> > 
> > of course there were, they just were not properly specified. See for 
> > example 
> > https://quickgit.kde.org/?p=plasma-integration.git=commit=70049ac737aad8a8a6e5025fdd326981959c70fd
> >  which fixes the not specified dependency to breeze.
> 
> René J.V. Bertin wrote:
> Does the code really depend on Breeze in a different way than it depends 
> on Oxygen? And wouldn't you have achieved the same insurance of having Breeze 
> installed with only the change in the CMake file, or is there an actual 
> possibility that someone may install the theme under a different name?

> Does the code really depend on Breeze in a different way than it depends on 
> Oxygen?

Yes it's different in the way that Breeze is set as the default style. Because 
of that it's defined as a hard build dependency. But you are right for Oxygen 
we also need to implement the support to find it as an (optional) build 
dependency.

> And wouldn't you have achieved the same insurance of having Breeze installed 
> with only the change in the CMake file, or is there an actual possibility 
> that someone may install the theme under a different name?

Yes the important part is the CMake file, so that we can depend on it. The 
change to using the defined name is a lesson learned from the switch from 
Oxygen to Breeze which is in many places still hard coded in an incorrect way. 
By making it a proper dependency the next switch will be easier in future. One 
removes the Breeze dep and you get nice compile errors.


- Martin


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


On May 16, 2016, 9:49 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 May 16, 2016, 9:49 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2016-05-17 Thread René J . V . Bertin



> On May 17, 2016, 4:37 p.m., Martin Gräßlin wrote:
> > given that Plasma integration got removed I suggest to create a dedicated 
> > osx-integration repository for it as well.

I quite like the way things are, but yes, I've played with that idea. 

I guess I can simply fork `frameworkintegration`, change the project name in 
the toplevel CMake file and strip everything except for the platformtheme.

How does this work from that point onwards?


- René J.V.


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


On May 16, 2016, 9:49 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 May 16, 2016, 9:49 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, 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
> -
> 
>   CMakeLists.txt 6ea1cba 
>   src/CMakeLists.txt 1c54eb2 
>   src/platformtheme/CMakeLists.txt PRE-CREATION 
>   src/platformtheme/config-platformtheme.h.cmake PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.sh PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.upd PRE-CREATION 
>   src/platformtheme/kdemactheme.h PRE-CREATION 
>   src/platformtheme/kdemactheme.mm PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialogbase.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialogbase_p.h PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialoghelper.h PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialoghelper.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformsystemtrayicon.h PRE-CREATION 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2016-05-17 Thread René J . V . Bertin



> On May 17, 2016, 4:36 p.m., Martin Gräßlin wrote:
> > > I saw that one of the justifications for the move was a dependency on 
> > > Plasma library/ies, but I don't see any such dependencies in the latest 
> > > version before the move (commit 07548ac1fe7a7fb31a941473911e982fb623c07d).
> > 
> > of course there were, they just were not properly specified. See for 
> > example 
> > https://quickgit.kde.org/?p=plasma-integration.git=commit=70049ac737aad8a8a6e5025fdd326981959c70fd
> >  which fixes the not specified dependency to breeze.

Does the code really depend on Breeze in a different way than it depends on 
Oxygen? And wouldn't you have achieved the same insurance of having Breeze 
installed with only the change in the CMake file, or is there an actual 
possibility that someone may install the theme under a different name?


- René J.V.


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


On May 16, 2016, 9:49 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 May 16, 2016, 9:49 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, 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
> -
> 
>   CMakeLists.txt 6ea1cba 
>   src/CMakeLists.txt 1c54eb2 
>   src/platformtheme/CMakeLists.txt PRE-CREATION 
>   src/platformtheme/config-platformtheme.h.cmake PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.sh PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.upd PRE-CREATION 
>   src/platformtheme/kdemactheme.h PRE-CREATION 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme


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



given that Plasma integration got removed I suggest to create a dedicated 
osx-integration repository for it as well.

- Martin Gräßlin


On May 16, 2016, 9:49 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 May 16, 2016, 9:49 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, 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
> -
> 
>   CMakeLists.txt 6ea1cba 
>   src/CMakeLists.txt 1c54eb2 
>   src/platformtheme/CMakeLists.txt PRE-CREATION 
>   src/platformtheme/config-platformtheme.h.cmake PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.sh PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.upd PRE-CREATION 
>   src/platformtheme/kdemactheme.h PRE-CREATION 
>   src/platformtheme/kdemactheme.mm PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialogbase.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialogbase_p.h PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialoghelper.h PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialoghelper.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformsystemtrayicon.h PRE-CREATION 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformtheme.h PRE-CREATION 
>   src/platformtheme/kdeplatformtheme.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformtheme.json PRE-CREATION 
>   src/platformtheme/kdirselectdialog.cpp PRE-CREATION 
>   src/platformtheme/kdirselectdialog_p.h

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme


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



> I saw that one of the justifications for the move was a dependency on Plasma 
> library/ies, but I don't see any such dependencies in the latest version 
> before the move (commit 07548ac1fe7a7fb31a941473911e982fb623c07d).

of course there were, they just were not properly specified. See for example 
https://quickgit.kde.org/?p=plasma-integration.git=commit=70049ac737aad8a8a6e5025fdd326981959c70fd
 which fixes the not specified dependency to breeze.

- Martin Gräßlin


On May 16, 2016, 9:49 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 May 16, 2016, 9:49 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, 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
> -
> 
>   CMakeLists.txt 6ea1cba 
>   src/CMakeLists.txt 1c54eb2 
>   src/platformtheme/CMakeLists.txt PRE-CREATION 
>   src/platformtheme/config-platformtheme.h.cmake PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.sh PRE-CREATION 
>   src/platformtheme/frameworksintegration-5.16-font.upd PRE-CREATION 
>   src/platformtheme/kdemactheme.h PRE-CREATION 
>   src/platformtheme/kdemactheme.mm PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialogbase.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialogbase_p.h PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialoghelper.h PRE-CREATION 
>   src/platformtheme/kdeplatformfiledialoghelper.cpp PRE-CREATION 
>   src/platformtheme/kdeplatformsystemtrayicon.h PRE-CREATION 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2016-05-16 Thread René J . V . Bertin


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

(Updated May 16, 2016, 9:49 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, and 
Valorie Zimmerman.


Changes
---

Diff updated for git/master, and sync with what I hope are the relevant changes 
to the code in `plasma-integration`.


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 (updated)
-

  CMakeLists.txt 6ea1cba 
  src/CMakeLists.txt 1c54eb2 
  src/platformtheme/CMakeLists.txt PRE-CREATION 
  src/platformtheme/config-platformtheme.h.cmake PRE-CREATION 
  src/platformtheme/frameworksintegration-5.16-font.sh PRE-CREATION 
  src/platformtheme/frameworksintegration-5.16-font.upd PRE-CREATION 
  src/platformtheme/kdemactheme.h PRE-CREATION 
  src/platformtheme/kdemactheme.mm PRE-CREATION 
  src/platformtheme/kdeplatformfiledialogbase.cpp PRE-CREATION 
  src/platformtheme/kdeplatformfiledialogbase_p.h PRE-CREATION 
  src/platformtheme/kdeplatformfiledialoghelper.h PRE-CREATION 
  src/platformtheme/kdeplatformfiledialoghelper.cpp PRE-CREATION 
  src/platformtheme/kdeplatformsystemtrayicon.h PRE-CREATION 
  src/platformtheme/kdeplatformsystemtrayicon.cpp PRE-CREATION 
  src/platformtheme/kdeplatformtheme.h PRE-CREATION 
  src/platformtheme/kdeplatformtheme.cpp PRE-CREATION 
  src/platformtheme/kdeplatformtheme.json PRE-CREATION 
  src/platformtheme/kdirselectdialog.cpp PRE-CREATION 
  src/platformtheme/kdirselectdialog_p.h PRE-CREATION 
  src/platformtheme/kfiletreeview.cpp PRE-CREATION 
  src/platformtheme/kfiletreeview_p.h PRE-CREATION 
  src/platformtheme/kfontsettingsdata.h PRE-CREATION 
  src/platformtheme/kfontsettingsdata.cpp PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  src/platformtheme/khintssettings.h PRE-CREATION 
  src/platformtheme/khintssettings.cpp PRE-CREATION

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2016-05-16 Thread René J . V . Bertin


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

(Updated May 16, 2016, 3:21 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Plasma, and 
Valorie Zimmerman.


Changes
---

I'd been meaning for a while now to update this RR, since I wasn't getting any 
feedback on it.

Now that the platformtheme has been moved, what do you propose we do with the 
OS X platform theme?

I saw that one of the justifications for the move was a dependency on Plasma 
library/ies, but I don't see any such dependencies in the latest version before 
the move (commit 07548ac1fe7a7fb31a941473911e982fb623c07d).

It's a pity that this discussion fell into oblivion, I'd argue (or have argued) 
that
- there's sufficient reason to have a dedicated OS X platform theme because 
there are apparently no other feasible solutions to allow KF5 applications to 
benefit from KDE's font and colour palettes, regardless of whether that is with 
the native widget style (`"macintosh"`) or with another style. It should go 
without saying that applications designed with the automatic existence of 
specific fonts (and colour) palettes in mind will look better when they can use 
those fonts (and colours).
- applying this patch to `plasma-integration` to provide the Mac platform 
plugin via that package makes little sense because there's no Plasma (session) 
to integrate with. There *are* components in Plasma that make sense outside of 
Plasma sessions but they don't justify an interpretation where the 
`plasma-integration` package is for integrating Plasma with the host. Also, the 
package has dependencies on X11 and Wayland that would have to be removed on OS 
X.
- given the 2 above points, I'd suggest (have suggested) to keep the platform 
plugin where it is for OS X especially if the other components of the 
`framework-integration` framework are still relevant.

What bothers me a bit is that this move makes it much less trivial to maintain 
the Mac platform theme as a special version of the standard platform theme, 
subclassing its classes. Or maybe we can safely let the platform plugins 
diverge from this point? Would I have to revert all of my subclassing effort or 
would it be better to leave that aspect as is for the time being because it 
should make it easier to "sync" with the plasma platform theme?


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 -

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-18 Thread René J . V . Bertin



> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > I still want to see other OSX devs comment on that this is really the 
> > wanted approach. Also I need to point out that the split out of QPT to 
> > plasma-integration is prepared. I suggest to introduce a comparable 
> > osx-integration repository.

I have nothing against that, if done centrally. It could host/provide other 
things to, if set up appropriately. Maybe a better name would be inspired on 
Qt's MacExtras, X11Extras etc. (though arguably those are about improving 
integration).

KDE-Mac is CC'ed on each and every OS X related RR I file; from what I hear 
through the KDE-Mac ML their silence indicates general approval. You are of 
course not expected to take my word on this, but I do understand you have had 
some feedback from some of them concerning my contributions. And I think that 
includes the reality that I am currently basically the only active "OS X dev".

I'm aware that I'm in an uncomfortable position, working alone for and in name 
of a community the extent of which I don't even know. It's something that makes 
me wonder regularly why I'm doing it at all. I suppose that creating an 
osx-integration framework will make the lack of opinions from other "OSX devs" 
a bit more our internal affair rather than something affecting all of KF5?


> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR

I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code more 
compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?


- René J.V.


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


On Dec. 17, 2015, 7:40 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. 17, 2015, 7:40 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-18 Thread Aleix Pol Gonzalez



> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR
> 
> René J.V. Bertin wrote:
> I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
> advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code 
> more compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?

I don't see where you got that impression:
http://lists.qt-project.org/pipermail/development/2015-December/024040.html


- Aleix


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


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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-18 Thread René J . V . Bertin



> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR
> 
> René J.V. Bertin wrote:
> I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
> advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code 
> more compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?
> 
> Aleix Pol Gonzalez wrote:
> I don't see where you got that impression:
> 
> http://lists.qt-project.org/pipermail/development/2015-December/024040.html

Is that your idea of *relatively* recent? :) I was thinking of a discussion a 
bit longer ago (but frankly, don't ask me to dig it up, I doubt I'd be able to).

I haven't read the full thread above, in fact I stopped reading at 
```
Arguments against:
- it's uglier than "0", and more to type
```

because that sums up exactly how I think about the question. But in the end I 
only care that there are no chances of 32bit/64bit conflicts (I had some of 
those in code of my own, which led me to be almost religious about using NULL 
for pointers).

I do have a bit of an issue with using a *Q_* prefix, as it suggests a 
Qt-specific type that should only be used in calls to Qt functions...


- René J.V.


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


On Dec. 17, 2015, 7:40 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. 17, 2015, 7:40 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 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-18 Thread René J . V . Bertin


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

(Updated Dec. 18, 2015, 5:53 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

replaced 0 with Q_NULLPTR where appropriate, in the changed and new code I'm 
proposing.


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 (updated)
-

  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/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  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`

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-18 Thread Martin Gräßlin



> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR
> 
> René J.V. Bertin wrote:
> I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
> advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code 
> more compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?
> 
> Aleix Pol Gonzalez wrote:
> I don't see where you got that impression:
> 
> http://lists.qt-project.org/pipermail/development/2015-December/024040.html
> 
> René J.V. Bertin wrote:
> Is that your idea of *relatively* recent? :) I was thinking of a 
> discussion a bit longer ago (but frankly, don't ask me to dig it up, I doubt 
> I'd be able to).
> 
> I haven't read the full thread above, in fact I stopped reading at 
> ```
> Arguments against:
> - it's uglier than "0", and more to type
> ```
> 
> because that sums up exactly how I think about the question. But in the 
> end I only care that there are no chances of 32bit/64bit conflicts (I had 
> some of those in code of my own, which led me to be almost religious about 
> using NULL for pointers).
> 
> I do have a bit of an issue with using a *Q_* prefix, as it suggests a 
> Qt-specific type that should only be used in calls to Qt functions...

Rene, could you please not turn every review suggestion into a long discussion? 
And could you please remove your personal preferences. I also don't like to use 
Q_NULLPTR and would prefer nullptr, but that's not allowed in frameworks. Given 
that Q_NULLPTR is the best we have. It's better than 0 or NULL.


- Martin


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


On Dec. 17, 2015, 7:40 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. 17, 2015, 7:40 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-17 Thread René J . V . Bertin


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

(Updated Dec. 17, 2015, 7:40 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

this removes the qDebug() statements, and a few cleanups.

Can I presume that the `QDBusConnection::sessionBus.connect()` call 
`re`connects if a previous connection was already made with identical arguments 
except for sender and/or receiver?


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 (updated)
-

  src/platformtheme/kdemactheme.h PRE-CREATION 
  src/platformtheme/kdemactheme.mm PRE-CREATION 
  src/platformtheme/CMakeLists.txt 23f590e 
  src/platformtheme/kdeplatformtheme.h 97d09df 
  src/platformtheme/kdeplatformtheme.cpp 80dbcb7 
  src/platformtheme/kfontsettingsdata.h 4b92c7d 
  src/platformtheme/kfontsettingsdata.cpp b0a4bbf 
  src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-17 Thread Martin Gräßlin


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


I still want to see other OSX devs comment on that this is really the wanted 
approach. Also I need to point out that the split out of QPT to 
plasma-integration is prepared. I suggest to introduce a comparable 
osx-integration repository.


src/platformtheme/kdeplatformtheme.cpp (lines 45 - 46)


Q_NULLPTR


- Martin Gräßlin


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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-14 Thread René J . V . Bertin



> On Dec. 14, 2015, 7:56 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, line 45
> > 
> >
> > this looks like a not needed debug statement, same in dtor and 
> > loadSettings and KFontSettings

Evidently, sorry for that; they'll be gone in the next revision.

You're right: I added those statements chasing a bug that turned up to be 
upstream.


- René J.V.


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


On Dec. 12, 2015, 7:08 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. 12, 2015, 7:08 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:

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-14 Thread Martin Klapetek



> On Dec. 14, 2015, 7:56 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, line 45
> > 
> >
> > this looks like a not needed debug statement, same in dtor and 
> > loadSettings and KFontSettings
> 
> René J.V. Bertin wrote:
> Evidently, sorry for that; they'll be gone in the next revision.
> 
> You're right: I added those statements chasing a bug that turned up to be 
> upstream.

FYI you can have function name printed by qDebug by default by setting your 
QT_MESSAGE_PATTERN. See http://woboq.com/blog/nice-debug-output-with-qt.html


- Martin


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


On Dec. 12, 2015, 7:08 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. 12, 2015, 7:08 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-13 Thread Martin Gräßlin


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



src/platformtheme/kdeplatformtheme.cpp (line 45)


this looks like a not needed debug statement, same in dtor and loadSettings 
and KFontSettings


- Martin Gräßlin


On Dec. 12, 2015, 7:08 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. 12, 2015, 7:08 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-12 Thread René J . V . Bertin



> 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   0x00010355f9b6 QHashNode > QDBusConnectionPrivate::SignalHook>::~QHashNode() + 166 
> > (qgenericatomic.h:90)
> > 1   org.qt-project.QtCore   0x0001045b89b9 
> > QHashData::free_helper(void (*)(QHashData::Node*)) + 73 (qhash.cpp:405)
> > 2   org.qt-project.QtDBus   0x0001035531a8 
> > QDBusConnectionPrivate::~QDBusConnectionPrivate() + 712 (qhash.h:342)
> > 3   org.qt-project.QtDBus   0x0001035535ce 
> > QDBusConnectionPrivate::~QDBusConnectionPrivate() + 14 
> > (qdbusintegrator.cpp:1035)
> > 4   org.qt-project.QtDBus   0x00010354baf8 (anonymous 
> > namespace)::Q_QGS__q_sessionBus::innerFunction()::Cleanup::~Cleanup() + 152 
> > (qdbusconnection.cpp:1079)
> > 5   libsystem_c.dylib   0x7fff89e5d7a1 __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 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-12 Thread René J . V . Bertin


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

(Updated Dec. 12, 2015, 7:08 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

I've finally gotten around to building Kate, and wanted to show some 
comparisons, alas not completely working to my advantage.


For some reason, Christoph Cullman's own standalone app bundle build puts up an 
interface that has less glitches than the purely native interface I get when I 
deactivate (move aside) the frameworkintegration port with its KdeMacTheme 
plugin. The glitches I see with my own build are very much like what we used to 
see in KDE4/MacPorts. MacPorts never patched Qt's Mac platform theme, and Qt4 
had no QStandardPaths that we could (had to) patch. So what I'm seeing in my 
kate5 build is unlikely to be a direct result of using XDG compliant paths on 
OS X. Maybe the fact that those XDG compliant paths do contain all kinds of 
stuff that's not present in the standalone app bundle has something to do with 
it.
BTW, note that my build is an app bundle too, except that it contains only the 
strictly necessary files (executable, info dict and app icon).
Sadly for me, using the macintosh style with the KdeMacTheme plugin does not 
fix all the glitches though it does make the interface look less 
overdimensioned.

I hesitated to add a snap of kate running under X11 (with the macintosh style) 
but didn't to avoid confusion. However, it could have served to underline one 
tidbit that I didn't know myself until very recently:

> QWidgets (and QML) don't use native UI views. They draw everything themself. 
> The drawing is done in the style.

In other words, there is no native NSButton "behind" a QPushButton, no NSTabBar 
behind a QTabBar, etc. The mac style undoubtedly uses APIs that exist only on 
OS X to draw "everything", but it'll happily draw onto an X11 window (and 
create an almost identical result). This probably explains in part why we're 
seeing the glitches we're seeing, but as far as I'm concerned it also gives the 
native style less of a "it's native and thus better" status.


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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-07 Thread René J . V . Bertin


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


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   0x00010355f9b6 QHashNode::~QHashNode() + 166 (qgenericatomic.h:90)
1   org.qt-project.QtCore   0x0001045b89b9 
QHashData::free_helper(void (*)(QHashData::Node*)) + 73 (qhash.cpp:405)
2   org.qt-project.QtDBus   0x0001035531a8 
QDBusConnectionPrivate::~QDBusConnectionPrivate() + 712 (qhash.h:342)
3   org.qt-project.QtDBus   0x0001035535ce 
QDBusConnectionPrivate::~QDBusConnectionPrivate() + 14 
(qdbusintegrator.cpp:1035)
4   org.qt-project.QtDBus   0x00010354baf8 (anonymous 
namespace)::Q_QGS__q_sessionBus::innerFunction()::Cleanup::~Cleanup() + 152 
(qdbusconnection.cpp:1079)
5   libsystem_c.dylib   0x7fff89e5d7a1 __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`)...

- René J.V. Bertin


On Dec. 4, 2015, 7:01 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. 4, 2015, 7:01 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-07 Thread René J . V . Bertin



> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.h, lines 41-54
> > 
> >
> > why did you add the virtual? The methods are marked as Q_DECL_OVERRIDE 
> > which implies they are virtual
> 
> René J.V. Bertin wrote:
> Q_DECL_OVERRIDE indeed appears to resolve to `override` even with clang; 
> what about when it doesn't? Not supposed to happen and thus not to worry 
> about any issues that might cause?
> 
> For now I've left the virtual keywords on definitions that lack 
> Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?
> 
> Martin Gräßlin wrote:
> > what about when it doesn't?
> 
> It doesn't matter. override triggers a compile error if the base method 
> is not defined as virtual. By reimplementing in a subclass without virtual 
> you're not going to turn it non-virtual. We only used to add virtual in 
> derived classed to increase readability, to indicate that it overrides a base 
> method. Now we have language support for that.
> 
> > For now I've left the virtual keywords on definitions that lack 
> Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?
> 
> Don't change if it's not needed. If the method is virtual in the base 
> class then don't add virtual.
> 
> René J.V. Bertin wrote:
> Ok. Even the dtors? I thought it was good practice to make those virtual 
> almost always?
> 
> Martin Gräßlin wrote:
> with dtors it makes in-deed sense to mark them as virtual. At least I 
> follow that practice to have them always virtual even if I use override for 
> everything else. But of course such a change should be a dedicated 
> coding-style commit not touching anything else.

As it stands, only KFontSettings has a non-virtual dtor.


- René J.V.


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


On Dec. 4, 2015, 7:01 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. 4, 2015, 7:01 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-07 Thread René J . V . Bertin


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


Changes
---

revision with some (temporary!) debug trace outputs to go with 
https://git.reviewboard.kde.org/r/126198/#review89105


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 (updated)
-

  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`

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-07 Thread Martin Gräßlin



> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.h, lines 41-54
> > 
> >
> > why did you add the virtual? The methods are marked as Q_DECL_OVERRIDE 
> > which implies they are virtual
> 
> René J.V. Bertin wrote:
> Q_DECL_OVERRIDE indeed appears to resolve to `override` even with clang; 
> what about when it doesn't? Not supposed to happen and thus not to worry 
> about any issues that might cause?
> 
> For now I've left the virtual keywords on definitions that lack 
> Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?
> 
> Martin Gräßlin wrote:
> > what about when it doesn't?
> 
> It doesn't matter. override triggers a compile error if the base method 
> is not defined as virtual. By reimplementing in a subclass without virtual 
> you're not going to turn it non-virtual. We only used to add virtual in 
> derived classed to increase readability, to indicate that it overrides a base 
> method. Now we have language support for that.
> 
> > For now I've left the virtual keywords on definitions that lack 
> Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?
> 
> Don't change if it's not needed. If the method is virtual in the base 
> class then don't add virtual.
> 
> René J.V. Bertin wrote:
> Ok. Even the dtors? I thought it was good practice to make those virtual 
> almost always?

with dtors it makes in-deed sense to mark them as virtual. At least I follow 
that practice to have them always virtual even if I use override for everything 
else. But of course such a change should be a dedicated coding-style commit not 
touching anything else.


- Martin


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


On Dec. 4, 2015, 7:01 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. 4, 2015, 7:01 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-04 Thread Martin Gräßlin



> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/main_mac.cpp, line 45
> > 
> >
> > why does the mac platform want to setupXcbFlush?
> 
> René J.V. Bertin wrote:
> If it won't ever be signalled "from outside" then no, there's no need to 
> provide the slot at all. (There was a question about this on line 44 ;))
> 
> I take it the moc include is still required because the class derives 
> QObject?

> I take it the moc include is still required because the class derives QObject?
sure


- Martin


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


On Dec. 4, 2015, 2:05 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. 4, 2015, 2:05 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/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/kfontsettingsdatamac.h PRE-CREATION 
>   src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
>   src/platformtheme/khintssettings.h ec064d3 
>   src/platformtheme/khintssettingsmac.h PRE-CREATION 
>   src/platformtheme/khintssettingsmac.mm

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme


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

(Updated Dec. 4, 2015, 2:05 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

This takes Martin's feedback into account, and drops the autotest and KStyle 
parts.

I will only have the chance to test-build this (long story...) later this 
afternoon, apologies for any syntax errors that got through.


Summary (updated)
-

[OS X] adaptations for the KdePlatformTheme


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 (updated)
-

  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/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  src/platformtheme/khintssettings.h ec064d3 
  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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

2015-12-04 Thread Martin Gräßlin


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



src/platformtheme/kdemactheme.h (line 50)


same here



src/platformtheme/kdemactheme.mm (line 188)


do we still need that? I thought 5.3 is the minimum version anyway


- Martin Gräßlin


On Dec. 4, 2015, 2:05 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. 4, 2015, 2:05 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/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/kfontsettingsdatamac.h PRE-CREATION 
>   src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
>   src/platformtheme/khintssettings.h ec064d3 
>   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 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > Please include Hugo for a review on the KStyle changes.
> > 
> > I'd suggest to split the review into three parts: one about the adjusted 
> > test (ecm_foo) - that's a no brainer and doesn't need further discussion. 
> > One about the KStyle change and one about the platform theme change. These 
> > are independent libraries so a split review makes it easier IMHO.

split-offs created; I found only 1 Hugo in the list of known people, whom I 
included on the new KStyle RR.


> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/CMakeLists.txt, lines 67-69
> > 
> >
> > don't make thie an elsif, just an if. It's not an elseif condition

In practice it is, I think it has more or less been decided to assume `APPLE` 
means "no X11". Qt's XCB QPA can currently still be built on OS X (and is 
almost completely functional) but there's no guarantee how long that will still 
be possible.

Which should suit some of you, as full-blown X11 support would definitely give 
sense to supporting full-blown Plasma sessions (in so-called rooted mode of the 
XQuartz server). :P


> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.h, lines 41-54
> > 
> >
> > why did you add the virtual? The methods are marked as Q_DECL_OVERRIDE 
> > which implies they are virtual

Q_DECL_OVERRIDE indeed appears to resolve to `override` even with clang; what 
about when it doesn't? Not supposed to happen and thus not to worry about any 
issues that might cause?

For now I've left the virtual keywords on definitions that lack 
Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?


> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.h, lines 56-61
> > 
> >
> > there is no such thing as a protected variable (see e.g. 
> > https://programmers.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables
> >  ). If you need to access the variables, please add protected accessor and 
> > setter methods

That article suggests protected member variables do exist but should be 
avoided. Is that what you meant?

The only one I actually used was mKdeGlobals from the fontsettings class


> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/main_mac.cpp, line 45
> > 
> >
> > why does the mac platform want to setupXcbFlush?

If it won't ever be signalled "from outside" then no, there's no need to 
provide the slot at all. (There was a question about this on line 44 ;))

I take it the moc include is still required because the class derives QObject?


- René J.V.


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


On Dec. 3, 2015, 10:51 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. 3, 2015, 10:51 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme



> On Dec. 4, 2015, 8:26 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.h, lines 41-54
> > 
> >
> > why did you add the virtual? The methods are marked as Q_DECL_OVERRIDE 
> > which implies they are virtual
> 
> René J.V. Bertin wrote:
> Q_DECL_OVERRIDE indeed appears to resolve to `override` even with clang; 
> what about when it doesn't? Not supposed to happen and thus not to worry 
> about any issues that might cause?
> 
> For now I've left the virtual keywords on definitions that lack 
> Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?
> 
> Martin Gräßlin wrote:
> > what about when it doesn't?
> 
> It doesn't matter. override triggers a compile error if the base method 
> is not defined as virtual. By reimplementing in a subclass without virtual 
> you're not going to turn it non-virtual. We only used to add virtual in 
> derived classed to increase readability, to indicate that it overrides a base 
> method. Now we have language support for that.
> 
> > For now I've left the virtual keywords on definitions that lack 
> Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?
> 
> Don't change if it's not needed. If the method is virtual in the base 
> class then don't add virtual.

Ok. Even the dtors? I thought it was good practice to make those virtual almost 
always?


- René J.V.


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


On Dec. 4, 2015, 2:05 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. 4, 2015, 2:05 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 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme



> On Dec. 4, 2015, 2:40 p.m., Martin Gräßlin wrote:
> >

No, those we shouldn't need anymore as the toplevel CMake file will reject Qt 
versions earlier than 5.3.0 .

Should I clean up all occurrences from platformtheme, even where they have 
nothing to do with the Mac version?


- René J.V.


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


On Dec. 4, 2015, 7:01 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. 4, 2015, 7:01 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/kfontsettingsdata.cpp b0a4bbf 
>   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/CMakeLists.txt 23f590e 
>   src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
>   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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme


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

(Updated Dec. 4, 2015, 7:01 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

This takes the open issues into account, and also restores settings in the 
khintssettingsmac ctor that got lost when I added the `m_hints.clear()` command 
a rev. or 2 back. I also made the KHintsSettings member variables private 
again; I missed those earlier today.


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 (updated)
-

  src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdata.cpp b0a4bbf 
  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/CMakeLists.txt 23f590e 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-03 Thread Martin Gräßlin


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


Please include Hugo for a review on the KStyle changes.

I'd suggest to split the review into three parts: one about the adjusted test 
(ecm_foo) - that's a no brainer and doesn't need further discussion. One about 
the KStyle change and one about the platform theme change. These are 
independent libraries so a split review makes it easier IMHO.


src/platformtheme/CMakeLists.txt (lines 67 - 69)


don't make thie an elsif, just an if. It's not an elseif condition



src/platformtheme/kdeplatformtheme.h (lines 40 - 53)


why did you add the virtual? The methods are marked as Q_DECL_OVERRIDE 
which implies they are virtual



src/platformtheme/kdeplatformtheme.h (lines 55 - 60)


there is no such thing as a protected variable (see e.g. 
https://programmers.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables
 ). If you need to access the variables, please add protected accessor and 
setter methods



src/platformtheme/main_mac.cpp (line 45)


why does the mac platform want to setupXcbFlush?


- Martin Gräßlin


On Dec. 3, 2015, 10:51 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. 3, 2015, 10:51 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-03 Thread René J . V . Bertin


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

(Updated Dec. 3, 2015, 10:51 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

A slightly polished version. New here is support for the fact that OS X will 
use a bold `MessageBoxFont` by default (bold version of the system 
font/`GeneralFont`). This is now supported through a new cache entry 
corresponding to the bold `GeneralFont`. If however the user configured a 
different `GeneralFont`, this choice will be respected, but this font will 
still be made bold.

Interestingly, the `MessageBoxFont` is one of the fonts that OS X allows/ed to 
be configured (through a utility like `TinkerTool` that exposed preferences not 
otherwise exposed).


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 (updated)
-

  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/kdeplatformtheme.h 97d09df 
  src/platformtheme/kdeplatformtheme.cpp 80dbcb7 
  src/platformtheme/kfontsettingsdata.h 4b92c7d 
  src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  src/platformtheme/khintssettings.h ec064d3 
  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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 2, 2015, 10:45 a.m., Boudhayan Gupta wrote:
> > src/platformtheme/kdemactheme.mm, line 39
> > 
> >
> > 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.

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


- René J.V.

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 2, 2015, 10:45 a.m., Boudhayan Gupta wrote:
> > src/platformtheme/kdemactheme.mm, line 39
> > 
> >
> > 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.

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.


- René J.V.


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-02 Thread Boudhayan Gupta



> On Dec. 2, 2015, 3:15 p.m., Boudhayan Gupta wrote:
> > src/platformtheme/kdemactheme.mm, line 39
> > 
> >
> > 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.

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.


- 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 1, 2015, 9:34 p.m., René J.V. Bertin wrote:
> > src/platformtheme/kdemactheme.mm, lines 53-87
> > 
> >
> > I thought it would be best to use a native dialog here to show a 
> > warning dialog, but it turns out there is already a native event filter in 
> > place that causes a conflict and a crash.
> > I'll have to replace this with a QMessageBox.

QMessageBox isn't possible either because we don't have a QApplication instance 
at this point.
It *should* be possible to use a native dialog as shown above, but then I'd 
need to find a way to deactivate the nativeEventFilter, and reactivate it after 
the dialog has been closed.


- René J.V.


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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
> -
> 
>   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/
> 
> 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 2, 2015, 8:46 a.m., Martin Gräßlin wrote:
> > Overall I think this is now too much code duplication. With this appraoch 
> > you don't get bug fixes from the base code. I recommed to rather go for 
> > inheritance to have the actual code which can be shared still together.

Yes, I was aware of that; it was part of what I had in mind when I mentioned 
the maintenance burden being on KDE-Mac. 
I'll see what kind of difference inheritance can make here. I've been going 
over khintsettingsmac.mm because of a crash in there when testing without a 
kdeglobals file present. I'd left some look-and-feel remnants, and I think 
`KHintsSettings::readConfigValue()` will probably want to check if 
`mDefaultLnfConfig` isn't NULL before using it. 
Anyway, my impression is that even inheritance this class will be overriding 
(and thus duplicating) the brunt of the code; should I try it anyway?


> On Dec. 2, 2015, 8:46 a.m., Martin Gräßlin wrote:
> > src/platformtheme/CMakeLists.txt, lines 22-46
> > 
> >
> > please change this part to only contain differences. Otherwise it 
> > becomes difficult to maintain.
> > 
> > Like
> > 
> > set(platformtheme_SRCS ...)
> > if (APPLE)
> > set(platformtheme_SRCS ${platformtheme_SRCS} foo.mm)
> > endif()
> > 
> > 
> > Ideally I also don't want the generic part in the else branch. This 
> > should be more a feature based approach. What I don't want is that we 
> > generate a setup where it goes if(APPLE) else if (WINDOWS) else if 
> > (ANDROID)... You get what I mean ;-)

Erm, yes. Not sure why I didn't do this in the first place, maybe as a 
subconscious preparation for a possible need to fork and roll a related 
framework not intended to be Plasma-specific ;)


- René J.V.


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-02 Thread Martin Gräßlin



> On Dec. 2, 2015, 8:38 a.m., Martin Gräßlin wrote:
> > Just wondering: if your main aim is to change default settings, why don't 
> > follow what linux distros do? That is ship some default configuration 
> > interface. For inspiration check e.g. kubuntu-default-settings.
> 
> René J.V. Bertin wrote:
> If I understand you correctly, you're suggesting I write a set of default 
> configuration values that could go into a look-and-feel package (?) I 
> stripped support for those in an effort to be PC because they're expected in 
> a folder called `plasma`.
> 
> It does look like this idea could remove the need for a Mac-specific 
> KHintsSettings class.
> 
> One thing I don't understand here: precedence between the package 
> configured in kdeglobals (`LookAndFeelPackage`) which is stored in 
> `mDefaultLnfConfig` and the hardcoded `defaultLookAndFeelPackage` package. 
> The latter is stored in `mLnfConfig` if different from the user-selected 
> package, but then `readConfigValue()` gives precedence to `mLnfConfig` if 
> it's set. That looks as if the effective roles are reversed.
> 
> René J.V. Bertin wrote:
> Too fast again :-/
> 
> I see that my suggestion below to check `mDefaultLnfConfig` before 
> accessing it is moot, but shouldn't `mLnfConfig` be initialised to 0 if 
> `looknfeel == defaultLookAndFeelPackage`?

> If I understand you correctly, you're suggesting I write a set of default 
> configuration values that could go into a look-and-feel package 

kconfig files. You just install a more global config file with your default 
values. Best check with some distros on how to do that. (I'm from the opposite 
department, telling distros when they broke it :-P )


- Martin


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 2, 2015, 8:38 a.m., Martin Gräßlin wrote:
> > Just wondering: if your main aim is to change default settings, why don't 
> > follow what linux distros do? That is ship some default configuration 
> > interface. For inspiration check e.g. kubuntu-default-settings.
> 
> René J.V. Bertin wrote:
> If I understand you correctly, you're suggesting I write a set of default 
> configuration values that could go into a look-and-feel package (?) I 
> stripped support for those in an effort to be PC because they're expected in 
> a folder called `plasma`.
> 
> It does look like this idea could remove the need for a Mac-specific 
> KHintsSettings class.
> 
> One thing I don't understand here: precedence between the package 
> configured in kdeglobals (`LookAndFeelPackage`) which is stored in 
> `mDefaultLnfConfig` and the hardcoded `defaultLookAndFeelPackage` package. 
> The latter is stored in `mLnfConfig` if different from the user-selected 
> package, but then `readConfigValue()` gives precedence to `mLnfConfig` if 
> it's set. That looks as if the effective roles are reversed.

Too fast again :-/

I see that my suggestion below to check `mDefaultLnfConfig` before accessing it 
is moot, but shouldn't `mLnfConfig` be initialised to 0 if `looknfeel == 
defaultLookAndFeelPackage`?


- René J.V.


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 2, 2015, 8:38 a.m., Martin Gräßlin wrote:
> > Just wondering: if your main aim is to change default settings, why don't 
> > follow what linux distros do? That is ship some default configuration 
> > interface. For inspiration check e.g. kubuntu-default-settings.

If I understand you correctly, you're suggesting I write a set of default 
configuration values that could go into a look-and-feel package (?) I stripped 
support for those in an effort to be PC because they're expected in a folder 
called `plasma`.

It does look like this idea could remove the need for a Mac-specific 
KHintsSettings class.

One thing I don't understand here: precedence between the package configured in 
kdeglobals (`LookAndFeelPackage`) which is stored in `mDefaultLnfConfig` and 
the hardcoded `defaultLookAndFeelPackage` package. The latter is stored in 
`mLnfConfig` if different from the user-selected package, but then 
`readConfigValue()` gives precedence to `mLnfConfig` if it's set. That looks as 
if the effective roles are reversed.


- René J.V.


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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
> -
> 
>   autotests/CMakeLists.txt 7c2129c 
>   src/kstyle/CMakeLists.txt bc26667 
>   src/kstyle/kstyle.mm PRE-CREATION 
>   src/platformtheme/CMakeLists.txt 23f590e 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)


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

(Updated Dec. 2, 2015, 10:38 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

This revision is a probably heavy-handed attempt at implementing Martin's 
suggestion to re-use as much code as possible using inheritance. There's less 
code that can be reused than I'd have liked, and I have a strong impression 
that my implementation actually does quite a few things twice. Meaning once in 
the parent class and once in my own class, despite probably overly liberal use 
of `virtual` keywords. Then again it seems hard to avoid this given that the 
parent KdePlatformTheme class has KHintsSettings and KFontsSettings instances, 
and my KdeMacTheme has KHintsSettingsMac and KFontsSettingsMac instances.
Clear this isn't yet my strongest subject yet, so I hope I haven't made a 
complete mess of this :-/

I've made the private members protected, in order to be able to inherit them, 
and I've introduce a `KdePlatformTheme::fontType()` method so that it becomes 
possible to have a central `QPlatformTheme::Font -> 
KFontSettingsData::FontType` translation, and then do a platform-specific 
lookup in the dedicated `m_fontsData` array. Maybe that `fontType()` method 
should be in `KFontDataSettings`?

New in this revision is also `KFontDataSettingsMac`, which exists in order to 
obtain the default fonts from the system (though the sizes are hardcoded, 
currently). The current algorithm isn't very satisfactory, as it turns out to 
be a more or less known Qt issue that it doesn't provide this information 
reliably on OS X. Surprise, surprise :) I'll have to look if this information 
can be obtained through Cocoa.

The reason to do this dynamically (and leave the default colour palette to 
whatever Qt sets it to, for now) instead of using a predefined config file is 
that this information can change between OS versions. OS X 10.11 introduced a 
new system font (or 2), and the default colours aren't set in stone either (and 
users can chose the `Graphite` or default "Aqua" theme, as well as the more 
recent "Dark mode").


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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)


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

(Updated Dec. 2, 2015, 10:42 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

apologies, files were missing from the previous patch.


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 (updated)
-

  src/platformtheme/kdeplatformtheme.h 97d09df 
  src/platformtheme/kdemactheme.mm PRE-CREATION 
  src/platformtheme/kdemactheme.h PRE-CREATION 
  src/platformtheme/CMakeLists.txt 23f590e 
  src/kstyle/kstyle.mm PRE-CREATION 
  src/kstyle/CMakeLists.txt bc26667 
  autotests/CMakeLists.txt 7c2129c 
  src/platformtheme/kdeplatformtheme.cpp 80dbcb7 
  src/platformtheme/kfontsettingsdata.h 4b92c7d 
  src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  src/platformtheme/khintssettings.h ec064d3 
  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`

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-02 Thread Boudhayan Gupta



> On Dec. 2, 2015, 3:15 p.m., Boudhayan Gupta wrote:
> > src/platformtheme/kdemactheme.mm, line 39
> > 
> >
> > 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.

Ah, that makes sense. Okay, I'm dropping this issue.


- 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-02 Thread Boudhayan Gupta


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



src/platformtheme/kdemactheme.mm (line 39)


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.


- Boudhayan Gupta


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 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-02 Thread Luigi Toscano



> On Dic. 2, 2015, 10:45 a.m., Boudhayan Gupta wrote:
> > src/platformtheme/kdemactheme.mm, line 39
> > 
> >
> > 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.

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.


- Luigi


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


On Dic. 1, 2015, 9:29 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 Dic. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-02 Thread Martin Gräßlin



> On Dec. 2, 2015, 10:45 a.m., Boudhayan Gupta wrote:
> > src/platformtheme/kdemactheme.mm, line 39
> > 
> >
> > 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.

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.


- Martin


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


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)



> On Dec. 2, 2015, 8:38 a.m., Martin Gräßlin wrote:
> > Just wondering: if your main aim is to change default settings, why don't 
> > follow what linux distros do? That is ship some default configuration 
> > interface. For inspiration check e.g. kubuntu-default-settings.
> 
> René J.V. Bertin wrote:
> If I understand you correctly, you're suggesting I write a set of default 
> configuration values that could go into a look-and-feel package (?) I 
> stripped support for those in an effort to be PC because they're expected in 
> a folder called `plasma`.
> 
> It does look like this idea could remove the need for a Mac-specific 
> KHintsSettings class.
> 
> One thing I don't understand here: precedence between the package 
> configured in kdeglobals (`LookAndFeelPackage`) which is stored in 
> `mDefaultLnfConfig` and the hardcoded `defaultLookAndFeelPackage` package. 
> The latter is stored in `mLnfConfig` if different from the user-selected 
> package, but then `readConfigValue()` gives precedence to `mLnfConfig` if 
> it's set. That looks as if the effective roles are reversed.
> 
> René J.V. Bertin wrote:
> Too fast again :-/
> 
> I see that my suggestion below to check `mDefaultLnfConfig` before 
> accessing it is moot, but shouldn't `mLnfConfig` be initialised to 0 if 
> `looknfeel == defaultLookAndFeelPackage`?
> 
> Martin Gräßlin wrote:
> > If I understand you correctly, you're suggesting I write a set of 
> default configuration values that could go into a look-and-feel package 
> 
> kconfig files. You just install a more global config file with your 
> default values. Best check with some distros on how to do that. (I'm from the 
> opposite department, telling distros when they broke it :-P )

Ah, ok. For now I've been focussing on your other suggestion, using inheritance.
But in a sense I'd prefer using default values determined at runtime for the 
"perfect" native look; either using Qt calls as I've done in my latest attempt, 
or using calls into whatever native API is available for that purpose. That's 
why I've been using ObjC++ after all :)


- René J.V.


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


On Dec. 2, 2015, 10:38 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. 2, 2015, 10:38 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 
>

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

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

A couple of things I jotted down today, probably open doors but nonetheless 
maybe something to consider and take into account for those who have little or 
no first-hand experience with Qt on OS X or even MS Windows.

On Linux and other Unices that aren't OS X/iOS, there is no native platform SDK 
that provides predefined generic (and less generic) buttons, menus and whatever 
widgets. If we ignore Wayland, there was only X11, which simply provides 
windows, undecorated and empty bits of screen space that aren't even 
necessarily rectangular. What you do with those windows is completely up to 
you. And that's exactly what Qt did when they designed the Fusion theme that 
was introduced in Qt5.
Qt applications that are started on a Linux system and that don't have access 
to anything else, will by default use the QGenericUnixTheme, combined with 
Fusion. Given that this was designed from scratch, it is not surprising that 
its use gives interfaces designed with the generic Qt toolbox a pretty damn 
good look, leading to interfaces that are perfectly workable and maybe even 
enjoyable to use. They even don't look too much out of place on other 
platforms, because that was another design criterium from what I understand.

OS X and MS Windows on the other hand do provide their own SDKs with predefined 
generic (and less generic) buttons, menus and whatever widgets. Cross-platform 
middlewares like Qt can indeed be expected to use those SDKs as the basis for 
their platform implementation (which doesn't mean they should be forbidden from 
also providing a true cross-platform-coherent look). But such a middleware can 
only guarantee to provide generic controls that map to the generic native 
widgets, or else at most a smallest common denominator collection of more 
specialised widgets that exist in the SDKs of all supported platforms.
As a result, interfaces built with only that cross-platform toolbox will look 
more or less good depending on how well generic design principles that come 
with using Qt map to the underlying toolkits --- and influenced by Tufte as I 
am, I firmly believe that this can and will have implications for interface 
usability.

To borrow a metaphor and proverb: a generic, "bare-bones" Qt interface using 
the OS X native theme will look like it's wearing the emperor's clothes, but 
clearly wasn't designed for wearing those clothes. The extent to which this is 
the case varies from application to application, but you can (and likely will) 
end up with cases where the native theme makes an application look more out of 
place than a theme like Fusion (if it could be used in its full form).

This was particularly visible in KDE4 applications using the native "macintosh" 
theme/style (enough examples of kate and kcalc, among others, have been 
posted). It looks like the underlying code hasn't that much evolved since Qt 
4.8.7, so I'm expecting certain issues we identified in KDE4 apps to exist 
under KF5 too. (Indeed, the font issue with the tab selector widget looks very 
familiar.)

- René J.V. Bertin

On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

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

(Updated Dec. 1, 2015, 9:29 p.m.)

Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie
Zimmerman.

Changes
---

This is the promised new revision, which introduces Mac-specific files, with
Mac-specific KdeMacTheme and KHintSettingsMac classes. I stripped them of
everything I saw that was either irrelevant or unsupported.

I'm not so sure what to make of the other components; I don't see anything
Plasma-specific in InfoPage, the KMessageBox seems just as useful on OS X as
everywhere else; the same could probably be said about KF5Style if I understand
its mission correctly.

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 (updated)
-

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`

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)


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



src/platformtheme/kdemactheme.mm (lines 53 - 87)


I thought it would be best to use a native dialog here to show a warning 
dialog, but it turns out there is already a native event filter in place that 
causes a conflict and a crash.
I'll have to replace this with a QMessageBox.


- René J.V. Bertin


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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
> -
> 
>   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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)

2015-12-01 Thread Martin Gräßlin


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


Overall I think this is now too much code duplication. With this appraoch you 
don't get bug fixes from the base code. I recommed to rather go for inheritance 
to have the actual code which can be shared still together.


src/platformtheme/CMakeLists.txt (lines 22 - 46)


please change this part to only contain differences. Otherwise it becomes 
difficult to maintain.

Like

set(platformtheme_SRCS ...)
if (APPLE)
set(platformtheme_SRCS ${platformtheme_SRCS} foo.mm)
endif()


Ideally I also don't want the generic part in the else branch. This should 
be more a feature based approach. What I don't want is that we generate a setup 
where it goes if(APPLE) else if (WINDOWS) else if (ANDROID)... You get what I 
mean ;-)



src/platformtheme/CMakeLists.txt (line 46)


please use the newer and easier endif() variant.


- Martin Gräßlin


On Dec. 1, 2015, 9:29 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. 1, 2015, 9:29 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
> -
> 
>   autotests/CMakeLists.txt 7c2129c 
>   src/kstyle/CMakeLists.txt bc26667 
>   src/kstyle/kstyle.mm PRE-CREATION 
>   src/platformtheme/CMakeLists.txt 23f590e 
>   src/platformtheme/kdemactheme.h

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme (and autotests)