Discussion:
D5111: Provide demo/preview for checkable menu items and colour scheme comparison
René J.V. Bertin
2017-03-20 11:46:53 UTC
Permalink
rjvbb created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
This is a continuation/transfer of https://git.reviewboard.kde.org/r/128109/ (which still has a few screenshots)

There currently is no "official" cheap way to preview how checked menu items are rendered as a function of installed widget style, nor an easy/cheap way to compare the look of a given widget style under different colour schemes. I have been annoyed by the fact one has to (re)launch often heavy applications to assess the impact of changes to these look-and-feel aspects.

This patch explores a straightforward way of adding that missing functionality via the most exhaustive look-and-feel preview utility I am aware of that is cheap in resource requirements: Oxygen's demo app.

This demo already has menus in the MDI preview. The patch makes the layout items checkable and puts them in a QActionGroup so their mutually exclusive nature is taken into account. It also adds a "right to left" non-exclusive checkable item which is linked to the "Left to Right" checkbox in the demo frame.

For colour scheme exploration I adapted a thin wrapper around KColorSchemeManager written originally by Alexander Zhigalin for KDevelop; it is used to add a button with drop-down menu to the page widget's button box which makes it available in all views.

TEST PLAN
The menu additions have been checked on Linux and Mac OS X with Qt 5.6.0 - 5.8.0 and frameworks 5.22.0 - 5.32.0; the colour scheme addition with Qt 5.8.0 and frameworks 5.32.0 .

I looked for an appropriate way to uncheck the Tile/Cascade/Tabbed menu items (= when the user moves or resizes one of the MDI windows) but it seems those events are not available through signals and would thus require subclassing MdiSubWindow .

It would make sense IMHO to separate this demo from Oxygen. It has nothing Oxygen-specific and could be provided as a standalone theme explorer or bundled with another package that doesn't have too strong connotations of being "Plasma-exclusive" (Qt has widget style and palette support on most if not all platforms it runs on).

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

AFFECTED FILES
kstyle/demo/CMakeLists.txt
kstyle/demo/oxygendemodialog.cpp
kstyle/demo/oxygendemodialog.h
kstyle/demo/oxygenmdidemowidget.cpp
kstyle/demo/oxygenmdidemowidget.h
kstyle/demo/oxygenschemechooser.cpp
kstyle/demo/oxygenschemechooser.h

To: rjvbb, hpereiradacosta, jriddell, anthonyfieroni, zhigalin
Cc: kde-mac, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
René J.V. Bertin
2017-03-20 11:50:04 UTC
Permalink
rjvbb removed a project: Plasma.
rjvbb removed a subscriber: plasma-devel.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, hpereiradacosta, jriddell, anthonyfieroni, zhigalin
Cc: kde-mac, #frameworks
Hugo Pereira Da Costa
2017-03-20 12:00:19 UTC
Permalink
hpereiradacosta added a comment.


Hi,
Thanks for the set of patches.

in general, i am ok with the change but:

- please re-add the screenshot from Review Board. (sorry I was not aware of this review request cause I was not in the list of reviewers, even though official maintainer of oxygen ...)
- right to left layout action <- no. There is already one at the bottom of the window.
- this review should really be several, one per feature: one for the checkboxes/radiobuttons in the mdi window, one for the colorschemechooser. Can you split ?
- finally, there is need for more detail review (once above is done). For instance, in ColorSchemeChooser you use SUPPORT_THEME_SAVING, but this one is set/defined nowhere. So the whole corresponding code should go, right ? Or is it work in progress ? Personally I would disagree with having oxygen-demo being anything other than a demo, and for instance altering configuration. This is not the right place. The right place is the relevant KCM dialog.

Best,

Hugo

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-20 12:19:28 UTC
Permalink
rjvbb added a comment.
Post by Hugo Pereira Da Costa
- please re-add the screenshot from Review Board. (sorry I was not aware of this review request cause I was not in the list of reviewers, even though official maintainer of oxygen ...)
Sorry about that, I thought you'd be a member of the Plasma group. But I had a hunch so added a few reviewers manually this time.
Post by Hugo Pereira Da Costa
- right to left layout action <- no. There is already one at the bottom of the window.
The goal was not to duplicate functionality, but to add an additional item to the Layout menu that was not mutually exclusive with the others, as different styles can render such items differently. It also gives the possibility to add a separator.
IIRC the patch *would* become somewhat simpler if I just leave the menu item but make it a noop. Would you be OK with that?
Post by Hugo Pereira Da Costa
- this review should really be several, one per feature: one for the checkboxes/radiobuttons in the mdi window, one for the colorschemechooser. Can you split ?
Yay, extra work... Fortunately not too much, will do later today.
Post by Hugo Pereira Da Costa
- finally, there is need for more detail review (once above is done). For instance, in ColorSchemeChooser you use SUPPORT_THEME_SAVING, but this one is set/defined nowhere.
TBH, I was more or less hoping for feedback like this. I also don't see the need to save this setting but didn't want to strip it out immediately, also out of some form of respect for Alexander's original work.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-20 12:20:50 UTC
Permalink
rjvbb added a comment.


Screenshots:

F2988075: oxydemo-breeze.png <https://phabricator.kde.org/F2988075>

F2988076: oxydemo-qtcurve.png <https://phabricator.kde.org/F2988076>

F2988077: oxydemo-oxygen.png <https://phabricator.kde.org/F2988077>

F2988078: oxydemo-qtcurve-menu.png <https://phabricator.kde.org/F2988078>

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-20 12:26:21 UTC
Permalink
rjvbb added a comment.


Once more. Note how Oxygen renders an icon on the menu button despite me having disabled the icons-in-buttons feature in the settings. Only QtCurve seems to respect this setting for regular buttons (in dialog button boxes) nowadays.

F2988220: oxydemo-oxygen-menu.png <https://phabricator.kde.org/F2988220>

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-20 14:41:03 UTC
Permalink
rjvbb updated this revision to Diff 12644.
rjvbb edited the summary of this revision.
rjvbb added a comment.


Updated, not yet split.

I've kept the former left-to-right menu action as a "check here" noop action and made the distinction a bit more explicit by adding a menu section (which also didn't didn't benefit from a preview yet).

FWIW: notice how Qt flips "<- Check here" to "Check here ->" automatically when flipping the layout! Should a comment about this feature be added so that translators will preserve it?

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5111?vs=12634&id=12644

REVISION DETAIL
https://phabricator.kde.org/D5111

AFFECTED FILES
kstyle/demo/CMakeLists.txt
kstyle/demo/oxygendemodialog.cpp
kstyle/demo/oxygendemodialog.h
kstyle/demo/oxygenmdidemowidget.cpp
kstyle/demo/oxygenschemechooser.cpp
kstyle/demo/oxygenschemechooser.h

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-20 15:08:08 UTC
Permalink
rjvbb added a comment.


Colour scheme chooser proposition split off: https://phabricator.kde.org/D5113

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-20 15:15:29 UTC
Permalink
rjvbb updated this revision to Diff 12647.
rjvbb added a comment.


stripped diff.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5111?vs=12644&id=12647

REVISION DETAIL
https://phabricator.kde.org/D5111

AFFECTED FILES
kstyle/demo/oxygendemodialog.cpp
kstyle/demo/oxygendemodialog.h
kstyle/demo/oxygenmdidemowidget.cpp

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
René J.V. Bertin
2017-03-21 16:02:15 UTC
Permalink
rjvbb updated this revision to Diff 12666.
rjvbb added a comment.


Maintain Qt4 compatibility

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5111?vs=12647&id=12666

REVISION DETAIL
https://phabricator.kde.org/D5111

AFFECTED FILES
kstyle/demo/main.cpp
kstyle/demo/oxygendemodialog.cpp
kstyle/demo/oxygendemodialog.h
kstyle/demo/oxygenmdidemowidget.cpp

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks
Luigi Toscano
2017-03-21 16:05:00 UTC
Permalink
ltoscano added a comment.


Which repository is this? Oxygen? Please add it.

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: ltoscano, kde-mac, #frameworks
René J.V. Bertin
2017-03-21 16:55:19 UTC
Permalink
rjvbb set the repository for this revision to R113 Oxygen Theme.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: ltoscano, kde-mac, #frameworks
René J.V. Bertin
2017-03-21 16:55:39 UTC
Permalink
rjvbb added a comment.
Post by Luigi Toscano
Which repository is this? Oxygen? Please add it.
Apologies, something must have gone wrong adding it the 1st time.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: ltoscano, kde-mac, #frameworks
René J.V. Bertin
2017-03-25 11:09:26 UTC
Permalink
rjvbb updated this revision to Diff 12788.
rjvbb added a comment.


Rebased on master; also updated the MDI page title to indicate that it has menu previews.

In terms of general improvement we might consider adding a command line argument to start with a specific page.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5111?vs=12666&id=12788

REVISION DETAIL
https://phabricator.kde.org/D5111

AFFECTED FILES
kstyle/demo/main.cpp
kstyle/demo/oxygendemodialog.cpp
kstyle/demo/oxygendemodialog.h
kstyle/demo/oxygenmdidemowidget.cpp

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: ltoscano, kde-mac, #frameworks
René J.V. Bertin
2017-06-24 09:01:27 UTC
Permalink
rjvbb updated this revision to Diff 15812.
rjvbb added a comment.


Updated for 5.10.2+

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5111?vs=12788&id=15812

REVISION DETAIL
https://phabricator.kde.org/D5111

AFFECTED FILES
kstyle/demo/main.cpp
kstyle/demo/oxygendemodialog.cpp
kstyle/demo/oxygendemodialog.h
kstyle/demo/oxygenmdidemowidget.cpp

To: rjvbb, hpereiradacosta, jriddell, zhigalin, anthonyfieroni
Cc: ltoscano, kde-mac, #frameworks
René J.V. Bertin
2017-06-24 09:01:53 UTC
Permalink
rjvbb retitled this revision from "Provide demo/preview for checkable menu items and colour scheme comparison" to "Provide demo/preview for checkable menu items".
rjvbb edited the summary of this revision.
rjvbb edited the test plan for this revision.
rjvbb set the repository for this revision to R113 Oxygen Theme.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, hpereiradacosta, jriddell, zhigalin, anthonyfieroni
Cc: ltoscano, kde-mac, #frameworks
Anthony Fieroni
2017-11-10 04:34:25 UTC
Permalink
anthonyfieroni added a dependent revision: D8741: [kget] Fix a crash when opening the transfer history dialog.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, hpereiradacosta, jriddell, zhigalin, anthonyfieroni
Cc: ltoscano, kde-mac, #frameworks
Anthony Fieroni
2017-11-10 04:34:36 UTC
Permalink
anthonyfieroni removed a dependent revision: D8741: [kget] Fix a crash when opening the transfer history dialog.

REPOSITORY
R113 Oxygen Theme

REVISION DETAIL
https://phabricator.kde.org/D5111

To: rjvbb, hpereiradacosta, jriddell, zhigalin, anthonyfieroni
Cc: ltoscano, kde-mac, #frameworks

Loading...