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

[David Redondo]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:a9e1079eba40: [KColorschemeManager] Add option to 
reenable following global theme (authored by davidre).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=73079=73734

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Semke]

asemke added a comment.


  +1

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

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

[David Redondo]

davidre edited the summary of this revision.

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

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

[David Redondo]

davidre updated this revision to Diff 73079.
davidre added a comment.


  Forgot to save file

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=73077=73079

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[David Redondo]

davidre edited the summary of this revision.
davidre edited the test plan for this revision.

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

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

[David Redondo]

davidre updated this revision to Diff 73077.
davidre added a comment.


  @since 5.67

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=73075=73077

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 73075.
davidre added a comment.


  Also provide defaults for the other overloads

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=72623=73075

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> davidre wrote in kcolorschememanager.cpp:220
> Should we also do this for the other overloads? Or would that behavior change 
> be a blocker? ALso an application would want to call 
> `KColorSchemeManager::createSchemeSelectionMenu(const QString 
> , QObject *parent)` probably if the custom scheme is saved 
> between launches.

I'm not the author of this code but I don't see why this should be a blocker. 
With this you wouldn't break anything and would simply add more consistency 
across different applications.

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

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

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kcolorschememanager.cpp:220
> +{
> +return createSchemeSelectionMenu(QIcon(),QString(), QString(), parent);
> +}

Should we also do this for the other overloads? Or would that behavior change 
be a blocker? ALso an application would want to call 
`KColorSchemeManager::createSchemeSelectionMenu(const QString 
, QObject *parent)` probably if the custom scheme is saved 
between launches.

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

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

[David Redondo]

davidre updated this revision to Diff 72623.
davidre marked an inline comment as done.
davidre added a comment.


  Default values

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=72337=72623

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> kcolorschememanager.cpp:220
> +{
> +return createSchemeSelectionMenu(QIcon(),QString(), QString(), parent);
> +}

why not to use reasonable default values here like 
QIcon::fromTheme(QStringLiteral("preferences-desktop-color") and i18n("Color 
Scheme")? This function should be used then by all applications and this will 
help to get a more consistent behavior across the different applications.

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

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

[David Redondo]

davidre updated this revision to Diff 72337.
davidre marked 5 inline comments as done.
davidre added a comment.


  - note the version in which behavior was changed in the documentation
  - constexpr variable for default scheme row
  - more comments

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=72334=72337

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kcolorschememanager.cpp:227
> +// The property needs to be set before the palette change because is is 
> checked upon the
> +// ApplicationPaletteChange event.
>  qApp->setProperty("KDE_COLOR_SCHEME_PATH", index.data(Qt::UserRole));

Because I didn't see another multiline comment in this file to compare to ;)

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

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

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

[David Redondo]

davidre updated this revision to Diff 72334.
davidre added a comment.


  Fix test

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=72332=72334

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  autotests/kcolorschemetest.cpp
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 72332.
davidre added a comment.


  typo

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=72331=72332

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[David Redondo]

davidre marked 4 inline comments as done.

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

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

[David Redondo]

davidre updated this revision to Diff 72331.
davidre marked 4 inline comments as done.
davidre added a comment.


  - Remove function
  - Select system scheme on invalid model index
  - fix and expand comment

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=72251=72331

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

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

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> davidre wrote in kcolorschememanager.h:111
> Would I need to add @since for a new overload?

"Would I need"... put yourself in the shoes of a user of this class: you  would 
want to know at which version this new overload is available when using it in 
your code which sets itself a certain version of KF as minimal expected, right? 
:)

So yes, you want to help the users of the API to know at which versions of KF 
they can expect which methods and classes to be present. And thus you want to 
ensure "@since" with any API changes, incl. new methods added, overloaded or 
not.

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

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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kcolorschememanager.h:111
> I would say yes (your question made me search for an answer since I wanted 
> too:)), from 
> https://community.kde.org/Guidelines_and_HOWTOs/API_Documentation:
> 
> > The @since tag tells users since when the class has existed. It is usual to 
> > put a KDE release number here.
> 
> 
> 
> > Method documentation: We can use @author and @since just like we do for 
> > classes.

s/wanted/wanted to know/ (phabricator doesn't let us edit inline comments 
weird).

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

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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> davidre wrote in kcolorschememanager.h:111
> Would I need to add @since for a new overload?

I would say yes (your question made me search for an answer since I wanted 
too:)), from https://community.kde.org/Guidelines_and_HOWTOs/API_Documentation:

> The @since tag tells users since when the class has existed. It is usual to 
> put a KDE release number here.



> Method documentation: We can use @author and @since just like we do for 
> classes.

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

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

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kcolorschememanager.h:111
>  KActionMenu *createSchemeSelectionMenu(const QString 
> , QObject *parent);
> +KActionMenu *createSchemeSelectionMenu(QObject *parent);
>  

Would I need to add @since for a new overload?

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kcolorschememanager.h:130
> +void followSystemScheme();
> +
>  private:

I like that idea with an empty ModelIndex.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> davidre wrote in kcolorschememanager.h:130
> I thought it would be nice to have as a convenience function if an 
> application has changed the scheme to easily go back to the system color 
> scheme. I don't know if it should be a slot. I put it there because 
> activateScheme is here.

I think the only communication channel with KColorSchemeManager is via the menu 
created in createSchemeSelectionMenu(). Setting back the default scheme will 
also be done via this menu. I don't see why an application would need such a 
helper function - there is simply no other handling of the color schemes in the 
applications except of that menu.

Also, if somebody would still need such a helper function, he/she could use 
activateScheme(QModelIndex()) instead of followSystemScheme(). activateScheme() 
exists already and an empty QModelIndex() could be interpreted as the default 
color scheme.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kcolorschememanager.h:130
> +void followSystemScheme();
> +
>  private:

I thought it would be nice to have as a convenience function if an application 
has changed the scheme to easily go back to the system color scheme. I don't 
know if it should be a slot. I put it there because activateScheme is here.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[David Redondo]

davidre updated this revision to Diff 72251.
davidre marked 3 inline comments as done.
davidre added a comment.


  - Use "Default" as string
  - index.row() == 0

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71976=72251

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> asemke wrote in kcolorschememanager.cpp:107
> Yes, was also wrong in LabPlot. Just corrected 
> https://invent.kde.org/kde/labplot/commit/262f37b59193ed88bf680b155dc6ecd37bd11419.
> 
> We should have maybe in this class only one public function KActionMenu 
> *createSchemeSelectionMenu(QObject *parent) which internally sets the string 
> to "Color Scheme" and the icon to "preferences-desktop-color". With this we'd 
> enforce a consistent look. Or this is menu is automatically created for 
> KXmlGuiWindow applications...

> Or this is menu is automatically created for KXmlGuiWindow applications...

That's what I was thinking of, yeah.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> ngraham wrote in kcolorschememanager.cpp:107
> "Default" is probably fine.
> 
> FWIW the parent menu item is actually mis-named, at least in Kate. It's 
> called "Color Theme" when it should be "Color Scheme"
> 
> Also this menu should be universal, and not re-implemented on a per-app basis.

Yes, was also wrong in LabPlot. Just corrected 
https://invent.kde.org/kde/labplot/commit/262f37b59193ed88bf680b155dc6ecd37bd11419.

We should have maybe in this class only one public function KActionMenu 
*createSchemeSelectionMenu(QObject *parent) which internally sets the string to 
"Color Scheme" and the icon to "preferences-desktop-color". With this we'd 
enforce a consistent look. Or this is menu is automatically created for 
KXmlGuiWindow applications...

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> asemke wrote in kcolorschememanager.cpp:107
> Many applications like kdevelop, digikam, labplot, etc. create a menu "Color 
> Scheme" in the main menu bar and add then menu item via KColorSchemeManager. 
> By using "System color scheme" here we'd have "Color Scheme" -> "System color 
> scheme" with this repeated "color scheme" string. Can we simply use "Default" 
> or "System" or "Desktop" here?

"Default" is probably fine.

FWIW the parent menu item is actually mis-named, at least in Kate. It's called 
"Color Theme" when it should be "Color Scheme"

Also this menu should be universal, and not re-implemented on a per-app basis.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> kcolorschememanager.cpp:107
>  });
> +m_data.prepend({i18n("System color scheme"), QString(), 
> QIcon::fromTheme("edit-undo")});
>  endResetModel();

Many applications like kdevelop, digikam, labplot, etc. create a menu "Color 
Scheme" in the main menu bar and add then menu item via KColorSchemeManager. By 
using "System color scheme" here we'd have "Color Scheme" -> "System color 
scheme" with this repeated "color scheme" string. Can we simply use "Default" 
or "System" or "Desktop" here?

> kcolorschememanager.cpp:229
>  qApp->setProperty("KDE_COLOR_SCHEME_PATH", index.data(Qt::UserRole));
> -
> qApp->setPalette(KColorScheme::createApplicationPalette(KSharedConfig::openConfig(index.data(Qt::UserRole).toString(;
> +if (index.data(Qt::UserRole).toString().isNull()) {
> +qApp->setPalette(qApp->style()->standardPalette());

if (index.row() == 0) would also do the job and is simpler and faster.

> kcolorschememanager.h:129
> + */
> +void followSystemScheme();
> +

What should be the use-case for this new function and should it really be a 
slot?

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

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

[Nathaniel Graham]

ngraham added a comment.


  I agree. If and when this lands, you could commandeer and close D15645 


REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  In D25877#581355 , @kossebau wrote:
  
  > D15645  tried something similar from 
what I understood without looking at details, you might want to compare at 
least :) Sorry, no time myself to look at things currently beyond this comment.
  
  
  Thanks for the pointer! Looking at that diff what it tried to do was reading 
the currently active color scheme from kdeglobals. That approach has two 
problems imo. First it only is correct on Plasma, secondly it sets the wrong 
scheme when the global color scheme changes. To fix the second issue one could 
listen to the settings changed signal on DBus but that also only works on 
Plasma. Just using the `standardPalette()` is much simpler and more reliable.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  D15645  tried something similar from what 
I understood without looking at details, you might want to compare at least :) 
Sorry, no time myself to look at things currently beyond this comment.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71976.
davidre added a comment.


  remove qdebug

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71975=71976

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71975.
davidre added a comment.


  Remove stuff that was just for testing and included in the diff by mistake

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71974=71975

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71974.
davidre added a comment.
This revision is now accepted and ready to land.


  remove comment

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71355=71974

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h
  src/kcolorschememanager_p.h

To: davidre, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  Nevermind confused myself, I was not changing the `UserRole` data but the 
data of the actions of the menu. For that there are no guarantees documented.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre planned changes to this revision.
davidre added a comment.


  Changing the `UserRole` is not the best

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

To: davidre, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  Ping

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  I agree.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  So KConfigWatcher didn't work because the KCM doesn't write with the notify 
flag. Even if we did we can't depend on a specific version of plasma. Thinking 
a bit more about this:
  
  This is in frameworks and getting the name probably works only on plasma 
byreading kdeglobals and listening for the dbus change signal. This means I 
need to find out if we are on plasma and not on some other desktop. (Maybe it 
has it's own color scheme support and reading kdeglobals (that still could 
exist) is wrong), 
  We set the KDE_COLOR_SCHEME_PATH for plasma-integration. Another idea would 
be that plasma-integration sets a property that would tell us the default color 
scheme. Then we don't need to care if we are on plasma we just check if this 
property exists and is valid.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Yeah, I guess my idea precludes the user forcing a specific color scheme that 
happens to be equal to the current system default color scheme.
  
  If that's something we think we should support, then feel free to ignore me.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  Also if then the color scheme is changed while the application is running it 
would display "Breeze (system default)" checked, breeze dark is unchecked but 
the app is dark because breeze dark is the global color scheme.
  
  I think it should be possible to use KConfigWatcher to monitor the global 
theme.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  But isn't that going away from the problem that this is trying to solve? To 
have an option to go back from a fixed colorscheme to the system one.
  Also I'm sure it can be done I just need to figure out how stuff works :D

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Then maybe we could even do it like so:
  
  Instead of having a separate "use system default" option, we could make the 
color scheme that has the same name as the default into the "use system default 
option, and present it thusly in the menu:
  
( ) Arc
( ) Arc Dark
(o) Breeze (System default)
( ) Breeze Dark

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a comment.


  In D25877#576205 , @ngraham wrote:
  
  > Is there actually a way for this to see the name of the system color scheme?
  
  
  I think so. We can certainly see the name of the system color scheme at 
startup. The tricky thing is when it changes but I will try to do it.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Is there actually a way for this to see the name of the system color scheme?

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a task: T12147: KConfigWidgets.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71355.
davidre added a comment.


  Really fix typo

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71354=71355

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71354.
davidre added a comment.


  Typo

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71353=71354

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71353.
davidre added a comment.


  Fix things, new api + documentation

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71347=71353

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre updated this revision to Diff 71347.
davidre added a comment.


  Update

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25877?vs=71261=71347

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre planned changes to this revision.
davidre added a comment.


  Make it a standalone diff

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Lovely. Any chance you could actually add the name of the current system 
color scheme into the menu item? Like "System color scheme (Breeze)"

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[David Redondo]

davidre added a dependency: D25875: [KColorschemeManager] Prevent changes to 
the palette if a custom one is set.

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Redondo]

davidre created this revision.
davidre added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
davidre requested review of this revision.

REVISION SUMMARY
  Because the style will always follow the global scheme (see D25875 
) we need only
  to only change the titlebar hint back to "kdeglobals" and we simply don't 
react
  to palette changes.

TEST PLAN
  Select "System color scheme" in Kate, change scheme in colors kcm

REPOSITORY
  R265 KConfigWidgets

BRANCH
  systemthem (branched from master)

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

AFFECTED FILES
  src/kcolorschememanager.cpp

To: davidre, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns