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
