D5186: Add a widget style chooser to oxygen-demo

[Hugo Pereira Da Costa]

hpereiradacosta requested changes to this revision.
hpereiradacosta added a comment.
This revision now requires changes to proceed.


  Implementation wise, perfect !
  Some minor comments to be fixed, then ship it !
  
  Thanks for the patch

INLINE COMMENTS

> oxygenstylechooser.cpp:37
> +#include <kconfiggroup.h>
> +#include <klocalizedstring.h>
> +

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

> oxygenstylechooser.cpp:47
> +{
> +}
> +

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

> oxygenstylechooser.h:29
> +class QIcon;
> +class QAction;
> +class KActionMenu;

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.

REPOSITORY
  R113 Oxygen Theme

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

To: rjvbb, hpereiradacosta
Cc: plasma-devel, #plasma, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170424/aceaa624/attachment.html>