Re: Review Request 126241: [OS X] adapting KStyle (WIP)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126241/#review89220 --- I honestly have some doubt on the approach (but no counter proposal either) Basically the file kstyle.mm is a *copy* of the .cpp file, with a couple of lines changed. (which I had to track down by - downloading the patch - making an explicit diff of the two files (which makes reviewing quite difficult/useless) The mm file has the name, but different extension, and is compiled only for OSX That means that any future fix (by me, or others) applied to one on a different platform will either - not be ported to the other platform - be ported blindly without testing (I don't even know how to compile mm on my linux box) Is this really what we want to do ? Of course one could also add a common parent class to both the linux and osx implementation, that makes 99% of the job (some KStyleBase class), but then, wont that break binary compatibility badly for all widget styles that inherits these ? (oxygen, breeze) Finally, the changes applied here only work for kstyle derived styles (oxygen, breeze), and not the QStyle based one. (QtCurve ?) ... not convinced. (but not strong objection either) - Hugo Pereira Da Costa On Dec. 4, 2015, 12:27 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126241/ > --- > > (Updated Dec. 4, 2015, 12:27 p.m.) > > > Review request for KDE Software on Mac OS X, KDE Frameworks and Hugo Pereira > Da Costa. > > > Repository: frameworkintegration > > > Description > --- > > This is a split-off from RR https://git.reviewboard.kde.org/r/126198/, as > suggested there. > > The proposed change (which is a work in progress) contains a few > modifications mirroring those proposed for the KdePlatformTheme plugin, > aiming to adapt the library to Mac OS X. > > These modifications should probably be implemented by subclassing KStyle > rather than duplicating all code. > > I have been focussing on the platform theme modifications, without really > looking into the extent to which KStyle is used or potentially useful on OS > X. A separate RR should support discussion about that more easily. > > Would it for instance be possible to use KStyle to create a Qt *style* plugin > that does nothing more than extending the native theme/style with support for > KDE's font roles/types, colour palettes and icon themes? This could be > preferable for users or developers who are not interested in providing a > consistent cross-platform look (which presumable requires a platform *theme*) > and/or who do not want to depend on a theme that makes explicit use of a > private Qt API (cf. `KdeMacTheme` in the RR above). > > > Diffs > - > > src/kstyle/CMakeLists.txt bc26667 > src/kstyle/kstyle.mm PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126241/diff/ > > > Testing > --- > > Builds on OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 ; "source" > modifications are tested in the platform theme. > > > Thanks, > > René J.V. Bertin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126241: [OS X] adapting KStyle (WIP)
> On Dec. 7, 2015, 4:29 p.m., Hugo Pereira Da Costa wrote: > > I honestly have some doubt on the approach (but no counter proposal either) > > Basically the file kstyle.mm is a *copy* of the .cpp file, with a couple of > > lines changed. (which I had to track down by > > - downloading the patch > > - making an explicit diff of the two files > > (which makes reviewing quite difficult/useless) > > > > The mm file has the name, but different extension, and is compiled only for > > OSX > > That means that any future fix (by me, or others) applied to one on a > > different platform will either > > - not be ported to the other platform > > - be ported blindly without testing (I don't even know how to compile mm on > > my linux box) > > Is this really what we want to do ? > > > > Of course one could also add a common parent class to both the linux and > > osx implementation, that makes 99% of the job (some KStyleBase class), > > but then, wont that break binary compatibility badly for all widget styles > > that inherits these ? (oxygen, breeze) > > > > Finally, the changes applied here only work for kstyle derived styles > > (oxygen, breeze), and not the QStyle based one. (QtCurve ?) > > > > ... not convinced. (but not strong objection either) I agree with most points, and am myself not sure where I (we) could go with a Mac-specific `KStyle`. Moving the OS X-specific code to a separate file was initially meant to be sure that the maintenance burden of code that was maybe not really intended to be used on that platform would be completely the responsibility of those who'd be using it. With `KStyle`, that's really a braindead approach, so I have indeed made a todo note to use inheritance here too. Using a common base class is a nice idea, probably more elegant than overriding (as I've been doing with the platform plugin, and which *may* be causing me some issues). I guess you're right it might break binary compatibility, but aren't the dependents you cite supposed to be tight to a given `frameworkintegration` version already? That final point converges with Christoph's conclusion. I *am* also working on a modified version of the KdePlatformPlugin (https://git.reviewboard.kde.org/r/126198). If it's indeed not possible (as someone else suggested!) to create a QStyle plugin that modifies fonts and/or colours, then a platform theme would be the way to go. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126241/#review89220 --- On Dec. 4, 2015, 1:27 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126241/ > --- > > (Updated Dec. 4, 2015, 1:27 p.m.) > > > Review request for KDE Software on Mac OS X, KDE Frameworks and Hugo Pereira > Da Costa. > > > Repository: frameworkintegration > > > Description > --- > > This is a split-off from RR https://git.reviewboard.kde.org/r/126198/, as > suggested there. > > The proposed change (which is a work in progress) contains a few > modifications mirroring those proposed for the KdePlatformTheme plugin, > aiming to adapt the library to Mac OS X. > > These modifications should probably be implemented by subclassing KStyle > rather than duplicating all code. > > I have been focussing on the platform theme modifications, without really > looking into the extent to which KStyle is used or potentially useful on OS > X. A separate RR should support discussion about that more easily. > > Would it for instance be possible to use KStyle to create a Qt *style* plugin > that does nothing more than extending the native theme/style with support for > KDE's font roles/types, colour palettes and icon themes? This could be > preferable for users or developers who are not interested in providing a > consistent cross-platform look (which presumable requires a platform *theme*) > and/or who do not want to depend on a theme that makes explicit use of a > private Qt API (cf. `KdeMacTheme` in the RR above). > > > Diffs > - > > src/kstyle/CMakeLists.txt bc26667 > src/kstyle/kstyle.mm PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126241/diff/ > > > Testing > --- > > Builds on OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 ; "source" > modifications are tested in the platform theme. > > > Thanks, > > René J.V. Bertin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 126241: [OS X] adapting KStyle (WIP)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126241/ --- Review request for KDE Software on Mac OS X, KDE Frameworks and Hugo Pereira Da Costa. Repository: frameworkintegration Description --- This is a split-off from RR https://git.reviewboard.kde.org/r/126198/, as suggested there. The proposed change (which is a work in progress) contains a few modifications mirroring those proposed for the KdePlatformTheme plugin, aiming to adapt the library to Mac OS X. These modifications should probably be implemented by subclassing KStyle rather than duplicating all code. I have been focussing on the platform theme modifications, without really looking into the extent to which KStyle is used or potentially useful on OS X. A separate RR should support discussion about that more easily. Would it for instance be possible to use KStyle to create a Qt *style* plugin that does nothing more than extending the native theme/style with support for KDE's font roles/types, colour palettes and icon themes? This could be preferable for users or developers who are not interested in providing a consistent cross-platform look (which presumable requires a platform *theme*) and/or who do not want to depend on a theme that makes explicit use of a private Qt API (cf. `KdeMacTheme` in the RR above). Diffs - src/kstyle/CMakeLists.txt bc26667 src/kstyle/kstyle.mm PRE-CREATION Diff: https://git.reviewboard.kde.org/r/126241/diff/ Testing --- Builds on OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 ; "source" modifications are tested in the platform theme. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126241: [OS X] adapting KStyle (WIP)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126241/#review89137 --- > create a Qt *style* plugin that does nothing more than extending the native > theme/style with support for KDE's font roles/types, colour palettes and icon > themes? QWidget::style has no influence on colors, fonts, or icons (see QWidget::palette and QWidget::font; icons are determined by QIcon::themeName). QWidget::style influences appearance of frames and positioning text inside those frames. I guess what you are after is a modified OS X plugin for QPlatformTheme. - Christoph Feck On Dec. 4, 2015, 12:27 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126241/ > --- > > (Updated Dec. 4, 2015, 12:27 p.m.) > > > Review request for KDE Software on Mac OS X, KDE Frameworks and Hugo Pereira > Da Costa. > > > Repository: frameworkintegration > > > Description > --- > > This is a split-off from RR https://git.reviewboard.kde.org/r/126198/, as > suggested there. > > The proposed change (which is a work in progress) contains a few > modifications mirroring those proposed for the KdePlatformTheme plugin, > aiming to adapt the library to Mac OS X. > > These modifications should probably be implemented by subclassing KStyle > rather than duplicating all code. > > I have been focussing on the platform theme modifications, without really > looking into the extent to which KStyle is used or potentially useful on OS > X. A separate RR should support discussion about that more easily. > > Would it for instance be possible to use KStyle to create a Qt *style* plugin > that does nothing more than extending the native theme/style with support for > KDE's font roles/types, colour palettes and icon themes? This could be > preferable for users or developers who are not interested in providing a > consistent cross-platform look (which presumable requires a platform *theme*) > and/or who do not want to depend on a theme that makes explicit use of a > private Qt API (cf. `KdeMacTheme` in the RR above). > > > Diffs > - > > src/kstyle/CMakeLists.txt bc26667 > src/kstyle/kstyle.mm PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126241/diff/ > > > Testing > --- > > Builds on OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 ; "source" > modifications are tested in the platform theme. > > > Thanks, > > René J.V. Bertin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
