D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu abandoned this revision.
amhndu added a comment.


  Superseded by D25877 

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham, pino
Cc: pino, ngraham, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu added a comment.


  Sorry about the delay, got caught up in things.
  
  So here's the situation: Currently, the menu's actions store the scheme path 
as the data and applications that currently use this menu (eg. KDevelop and 
Kate), just store this scheme path in their config. So if we were to add 
another action whose data is the system's theme, on triggering this action, the 
path to the then current scheme will be saved and if the system theme is 
changed again, the application will continue use the old theme, going out of 
sync with the system. Thus, there needs to be a way to distinguish the reset 
action with the other actions. Since it's impossible to this without breaking 
the applications that use it, I made this as an entirely new function.
  My (admittedly not a very good) solution to distinguish the actions were to 
store the //index// instead of of the scheme path. That way, you can use the 
index to get the name of the scheme and check if it matches the action's text.
  
  I've thought of creating a subclass of KActionMenu which has two signals - 
one for reset and one for setting a scheme and returning that, but that seems a 
little too complicated.
  
  Is there a simpler solution to this that I'm overlooking?

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham, pino
Cc: pino, ngraham, broulik, kde-frameworks-devel, LeGast00n, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu added a comment.


  Thanks for picking this up again!
  I'm definitely interested to resume but I've been a little busy this week. I 
can take a look again this weekend.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham, pino
Cc: pino, ngraham, broulik, kde-frameworks-devel, LeGast00n, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu marked an inline comment as done.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham, pino
Cc: pino, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu updated this revision to Diff 44342.
amhndu added a comment.


  > kcolorschemedemo.cpp does not actually seem to build an executable.
  
  It should build `kcolorschemedemo` in `kconfigwidgets/bin` (also has 
`kcolorschemedemo` target in make)
  
  > Regardless, wouldn't it make sense to expose this new feature in the 
existing Settings → Color Theme menu? Or are you planning to do that in another 
patch?
  
  Yes, that would require a patch for each application that'd use it. The 
problem is that with the new menu,
  we need a way to distinguish between the reset action and the other actions.
  So that applications don't just save the current system scheme in their 
config, if they did,
  the application would still use the previous scheme even after the user 
changed the system scheme. While resetting should mean the app
  should now be following the scheme, just as before setting anything manually.
  The new variant has the index as the data instead of the path, this allows 
checking the index's DisplayRole
  and the action's text to distinguish the reset, this is admittedly not a very 
good solution.
  I couldnnott think of anything else that's simple and wouldn't require much 
change in client code.
  What could be an alternative solution here ?
  I've thought of creating a subclass of KActionMenu which has two signals - 
one for reset and one for setting a scheme and returning that,
  but that would again require modifying applications to use these signals 
instead.
  
  > instead of a custom variant, what about creating new versions of the 
createSchemeSelectionMenu functions that take flags instead?
  
  I created a new variant since this isn't compatible with the other overload, 
as explained above.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15645?vs=44279=44342

BRANCH
  system-default

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

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham, pino
Cc: pino, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu marked an inline comment as done.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu updated this revision to Diff 44279.
amhndu edited the summary of this revision.
amhndu added a comment.


  Fixed error in colorschemedemo test.
  Sorry about that, I must've forgotton to commit it.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15645?vs=42166=44279

BRANCH
  system-default

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

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu added a comment.


  Ping. Can someone please review ?

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu retitled this revision from "[WIP] Add scheme selection menu with a 
"System" entry." to "Add scheme selection menu with a "System" entry.".
amhndu edited the summary of this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu updated this revision to Diff 42166.
amhndu marked 4 inline comments as done.
amhndu added a comment.


  Updated with styling suggestions.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15645?vs=42108=42166

BRANCH
  system-default

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

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

To: amhndu, #frameworks
Cc: shubham, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu marked 2 inline comments as done.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu updated this revision to Diff 42108.
amhndu added a comment.


  Move duplicate code from the two createSelectionMenu methods into one
  generic createSelectionMenu in KColorSchemeMangerPrivate

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15645?vs=42049=42108

BRANCH
  system-default

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

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

To: amhndu, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu added a comment.


  Should I also add the other overloads like `createSchemeSelectionMenu` does ?

INLINE COMMENTS

> broulik wrote in kcolorschememanager.cpp:214-228
> All of this is duplicated from`createSchemeSelectionMenu`, it should be split 
> into a separate method so it can be reused

A private function in KColorSchemeModel or a static function in 
kcolorschememanager.cpp ?

> broulik wrote in kcolorschememanager.h:127
> `WithDefaultEntry`?

`createSchemeSelectionMenuWithDefaultEntry` ?
won't it be too big ?

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu added a reviewer: Frameworks.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15645: [WIP] Add scheme selection menu with a "System" entry.

[Amish Naidu]

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

REVISION SUMMARY
  The scheme selection menu does not offer any hint for the current system
  scheme. This also makes it hard to "reset" back to the system scheme, once
  the scheme has been changed.
  
  This is a preliminary patch to add an action which references the system 
scheme.
  Since, this breaks applications using the action name to save the selected
  scheme, I've made a new function in which the action data has been changed 
from
  only the path to the model index, this makes it possible to use the data to
  differentiate if the selection action is the system scheme action or not.
  
  I know this probably isn't the best implementation to handle it, but this is
  the best I could figure out as a quick patch to get some input on doing this
  better.

TEST PLAN
  Run `kcolorschemedemo` from `tests` and test the menu from the button.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  system-default

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

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

To: amhndu
Cc: kde-frameworks-devel, michaelh, ngraham, bruns