kossebau added a comment.
Some comments while flying over the code due to being triggered by phab
message, but no in-detail review and analysis of approach done, lack of
concentration currently, sorry.
I also propose to change all magic number `0`s to use some `defaultThemeRow`
from a `constexpr int defaultThemeRow = 0:` instead,
INLINE COMMENTS
> kcolorschememanager.cpp:159
> {
> + if (name.isEmpty()) {
> + return d->model->index(0);
Propose to add a short comment explaining why we return index(0) here for the
future code reader starting off at this code before having read all code and
docs. E.g.
`// empty string is mapped to "reset to default/system", which is first item in
model`
That ensures the intention of this code is clear on high level.
> kcolorschememanager.cpp:162
> + }
> for (int i = 0; i < d->model->rowCount(); ++i) {
> QModelIndex index = d->model->index(i);
Could start off at 1 now, no?
> kcolorschememanager.cpp:189
> }
> -
> + if (!group->checkedAction()) {
> + group->actions()[0]->setChecked(true);
also could get a short comment what the highlevel logic is here
`// no color theme selected? so it's the default one`
> kcolorschememanager.cpp:221
> {
> - if (!index.isValid()) {
> - return;
> - }
> - if (index.model() != d->model.data()) {
> - return;
> - }
> - // hint for the style to synchronize the color scheme with the window
> manager/compositor
> + /* hint for plasma-integration to synchronize the color scheme with the
> window manager/compositor
> + * The property needs to be set before the palette change because is is
> checked upon the
Using `//` for each comment line is more typical for explanation comments, why
the different style here?
> kcolorschememanager.h:39
> * schemes to their user. For example it is very common for photo and
> painting applications to use
> - * a dark color scheme even if the default is a light scheme.
> + * a dark color scheme even if the default is a light scheme. It also allows
> going back to following
> + * the system color scheme.
Here you might also want to state at which version this feature of "allows
going back" was added, to make it clear that with older versions of KF this was
not possible.
Same with the existing methods where behaviour was changed.
REPOSITORY
R265 KConfigWidgets
BRANCH
systemthem (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D25877
To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n,
GB_2, michaelh, bruns
