Provide demo/preview for checkable menu items
Needs ReviewPublic

Authored by rjvbb on Mar 20 2017, 11:46 AM.

Details

Summary

This is the remaining changeset of my earlier contributions that extended the oxygen-demo application with widget style and colour scheme pickers, aiming to turn it into a generic look-and-feel explorer.

There currently is no "official" cheap way to preview how checked menu items are rendered as a function of installed widget style.

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.

An earlier version added a menu action to toggle the layout direction (mostly to provide a non-exclusive checkable item) and made sure to initialise the layout direction checkbox as a function of the startup direction. The menu item was removed on request but I decided to leave the checkbox initialisation .

Test Plan

Checked on Linux and Mac OS X with Qt 5.6.0 - 5.8.0 and frameworks 5.22.0 - 5.35.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).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.Mar 20 2017, 11:46 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 20 2017, 11:46 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rjvbb removed a subscriber: plasma-devel.

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

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

  • 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?

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

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

Screenshots:

rjvbb added a comment.EditedMar 20 2017, 12:26 PM

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

rjvbb updated this revision to Diff 12644.Mar 20 2017, 2:40 PM
rjvbb edited the summary of this revision. (Show Details)

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?

rjvbb added a comment.Mar 20 2017, 3:08 PM

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

rjvbb updated this revision to Diff 12647.Mar 20 2017, 3:15 PM

stripped diff.

rjvbb updated this revision to Diff 12666.Mar 21 2017, 4:01 PM

Maintain Qt4 compatibility

Which repository is this? Oxygen? Please add it.

rjvbb set the repository for this revision to R113 Oxygen Theme.Mar 21 2017, 4:54 PM

Which repository is this? Oxygen? Please add it.

Apologies, something must have gone wrong adding it the 1st time.

rjvbb updated this revision to Diff 12788.Mar 25 2017, 11:09 AM

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.

rjvbb updated this revision to Diff 15812.Jun 24 2017, 9:01 AM

Updated for 5.10.2+

rjvbb retitled this revision from Provide demo/preview for checkable menu items and colour scheme comparison to Provide demo/preview for checkable menu items.Jun 24 2017, 9:01 AM
rjvbb edited the summary of this revision. (Show Details)
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R113 Oxygen Theme.
Restricted Application added a project: Plasma. · View Herald TranscriptMar 22 2019, 1:23 PM