Add a widget style chooser to oxygen-demo
AbandonedPublic

Authored by rjvbb on Mar 26 2017, 5:30 PM.

Details

Reviewers
hpereiradacosta
Summary

This adds the widget style chooser evoked during the colour scheme chooser review (D5113).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.Mar 26 2017, 5:30 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 26 2017, 5:30 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

To be honest, I think all the dealing with default style, desktop style, bold font, etc is superfluous and in fact misleading:
The "default" style can actually depend not only on the OS, but on the distribution, etc.
Also the return of "cg.readEntry( "widgetStyle", fallback );" is actually the "current" style not the default. And should not be different from "QApplication()::style()".
In any case, none of this is really necessary here: IMHO the menu should just have the list of available styles (StyleFactory::keys()) and the "correct" radiobox selected at startup.
Wdyt ?

rjvbb added a comment.Apr 22 2017, 3:14 PM

In any case, none of this is really necessary here: IMHO the menu should just have the list of available styles (StyleFactory::keys()) and the "correct" radiobox selected at startup.
Wdyt ?

I don't disagree; it's how I would have implemented the widget when writing it from scratch for this application.

rjvbb updated this revision to Diff 13702.Apr 22 2017, 3:33 PM

Simplified style selector widget that just shows the available styles and "ticks" the currently active style.

rjvbb set the repository for this revision to R113 Oxygen Theme.Apr 22 2017, 3:34 PM
hpereiradacosta requested changes to this revision.Apr 24 2017, 10:03 AM

Implementation wise, perfect !
Some minor comments to be fixed, then ship it !

Thanks for the patch

kstyle/demo/oxygenstylechooser.cpp
37

I "think" the policy is
#include <KActionMenu>, #include<KSharedConfig> etc.
At least it is done so in all other files in oxygen (and breeze)

47

if the destructor does nothing (is the default), just remove it from the header and here.

kstyle/demo/oxygenstylechooser.h
29

Not sure there is an official policy here, but my preference goes for _not_ forward-declare classes on which I have no direct control. I would rather add the include explicitely.

This revision now requires changes to proceed.Apr 24 2017, 10:03 AM
rjvbb updated this revision to Diff 13755.Apr 24 2017, 5:32 PM
rjvbb edited edge metadata.

Final version. Most of the forward declarations were actually redundant.

rjvbb set the repository for this revision to R113 Oxygen Theme.Apr 24 2017, 5:33 PM