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

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

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

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

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,

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

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

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

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,

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

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

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

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

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

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 >

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

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

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

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

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

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

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

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

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

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));

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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"

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

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,

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

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

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

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

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

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

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

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

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

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)

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

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,

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

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,

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

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,

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

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

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

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

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

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

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

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

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

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

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

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 ( )

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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

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