D25877: [KColorschemeManager] Add option to reenable following global theme

[Friedrich W. H. Kossebau]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191229/da092378/attachment-0001.html>