D15645: [WIP] Add scheme selection menu with a "System" entry.
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.
shubham added inline comments.
INLINE COMMENTS
> kcolorschememanager.cpp:39
>
> +KActionMenu* KColorSchemeManagerPrivate::createSchemeMenu(const QIcon ,
> const QString , const QString , bool defaultEntry,
> QObject *parent)
> +{
KActionMenu* -> KActionMenu *
REPOSITORY
R265 KConfigWidgets
REVISION DETAIL
https://phabricator.kde.org/D15645
To: amhndu, #frameworks
Cc: shubham, ngraham, broulik, kde-frameworks-devel, michaelh, bruns
D15645: [WIP] Add scheme selection menu with a "System" entry.
ngraham added a comment. Very much +1 on the end goal! INLINE COMMENTS > amhndu wrote in kcolorschememanager.h:127 > `createSchemeSelectionMenuWithDefaultEntry` ? > won't it be too big ? I think it's okay. Pixels and characters are cheap. :) 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.
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.
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.
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.
broulik added inline comments.
INLINE COMMENTS
> kcolorschememanager.cpp:203
> +QAction *const resetAction = new
> QAction(index.data(Qt::DecorationRole).value(),
> + QStringLiteral("System
> (%1)").arg(systemScheme),
> + menu);
This needs translation, I also think "System" isn't a very good name for this.
How about "System Default" or just "Default"?
> kcolorschememanager.cpp:214-228
> +connect(group, ::triggered, [this](QAction *action) {
> +activateScheme(action->data().toModelIndex());
> +});
> +
> +for (int i = 0; i < d->model->rowCount(); ++i) {
> +QModelIndex index = d->model->index(i);
> +QAction *action = new
> QAction(index.data(Qt::DecorationRole).value(),
> index.data().toString(), menu);
All of this is duplicated from`createSchemeSelectionMenu`, it should be split
into a separate method so it can be reused
> kcolorschememanager.h:127
> + */
> +KActionMenu *createSchemeSelectionMenuWithDefault(const QIcon ,
> const QString , const QString , QObject *parent);
> +
`WithDefaultEntry`?
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.
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.
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
