D5186: Add a widget style chooser to oxygen-demo
Hugo Pereira Da Costa
noreply at phabricator.kde.org
Mon Apr 24 10:04:04 UTC 2017
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>
More information about the Plasma-devel
mailing list