Differential D25877 Diff 71976 src/kcolorschememanager.cpp

Changeset View

Standalone View

View Options

src/kcolorschememanager.cpp

Show All 16 Lines
17 * Boston, MA 02110-1301, USA. 17​ * Boston, MA 02110-1301, USA.
18 */ 18​ */
19#include "kcolorschememanager.h" 19​#include "kcolorschememanager.h"
20#include "kcolorschememanager_p.h" 20​#include "kcolorschememanager_p.h"
21 21​
22#include <kactionmenu.h> 22​#include <kactionmenu.h>
23#include <kconfiggroup.h> 23​#include <kconfiggroup.h>
24#include <kcolorscheme.h> 24​#include <kcolorscheme.h>
25​#include <klocalizedstring.h>
25#include <ksharedconfig.h> 26​#include <ksharedconfig.h>
26 27​
27#include <QApplication> 28​#include <QApplication>
28#include <QDir> 29​#include <QDir>
29#include <QFileInfo> 30​#include <QFileInfo>
30#include <QMenu> 31​#include <QMenu>
31#include <QPainter> 32​#include <QPainter>
32#include <QStandardPaths> 33​#include <QStandardPaths>
34​#include <QStyle>
33 35​
34KColorSchemeManagerPrivate::KColorSchemeManagerPrivate() 36​KColorSchemeManagerPrivate::KColorSchemeManagerPrivate()
35 : model(new KColorSchemeModel()) 37​ : model(new KColorSchemeModel())
36{ 38​{
37} 39​}
38 40​
39KColorSchemeModel::KColorSchemeModel(QObject *parent) 41​KColorSchemeModel::KColorSchemeModel(QObject *parent)
40 : QAbstractListModel(parent) 42​ : QAbstractListModel(parent)
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Line(s)97 for (const QString &schemeFile : qAsConst(schemeFiles)) {
97 KConfigGroup group(config, QStringLiteral("General")); 99​ KConfigGroup group(config, QStringLiteral("General"));
98 const QString name = group.readEntry("Name", QFileInfo(schemeFile).baseName()); 100​ const QString name = group.readEntry("Name", QFileInfo(schemeFile).baseName());
99 const KColorSchemeModelData data = {name, schemeFile, QIcon()}; 101​ const KColorSchemeModelData data = {name, schemeFile, QIcon()};
100 m_data.append(data); 102​ m_data.append(data);
101 } 103​ }
102 std::sort(m_data.begin(), m_data.end(), [](const KColorSchemeModelData & first, const KColorSchemeModelData & second) { 104​ std::sort(m_data.begin(), m_data.end(), [](const KColorSchemeModelData & first, const KColorSchemeModelData & second) {
103 return first.name < second.name; 105​ return first.name < second.name;
104 }); 106​ });
107​ m_data.prepend({i18n("System color scheme"), QString(), QIcon::fromTheme("edit-undo")});
asemkeUnsubmitted
Done

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?

asemke: Many applications like kdevelop, digikam, labplot, etc. create a menu "Color Scheme" in the…
ngrahamUnsubmitted
Done

"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.

ngraham: "Default" is probably fine. FWIW the parent menu item is actually mis-named, at least in Kate.
asemkeUnsubmitted
Not Done

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...

asemke: Yes, was also wrong in LabPlot. Just corrected https://invent.kde.
ngrahamUnsubmitted
Not Done

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

That's what I was thinking of, yeah.

ngraham: > Or this is menu is automatically created for KXmlGuiWindow applications... That's what I was…
105 endResetModel(); 108​ endResetModel();
106} 109​}
107 110​
108QIcon KColorSchemeModel::createPreview(const QString &path) const 111​QIcon KColorSchemeModel::createPreview(const QString &path) const
109{ 112​{
110 KSharedConfigPtr schemeConfig = KSharedConfig::openConfig(path, KConfig::SimpleConfig); 113​ KSharedConfigPtr schemeConfig = KSharedConfig::openConfig(path, KConfig::SimpleConfig);
111 QIcon result; 114​ QIcon result;
112 115​
Show All 35 Lines
148 151​
149QAbstractItemModel *KColorSchemeManager::model() const 152​QAbstractItemModel *KColorSchemeManager::model() const
150{ 153​{
151 return d->model.data(); 154​ return d->model.data();
152} 155​}
153 156​
154QModelIndex KColorSchemeManager::indexForScheme(const QString &name) const 157​QModelIndex KColorSchemeManager::indexForScheme(const QString &name) const
155{ 158​{
159​ if (name.isEmpty()) {
kossebauUnsubmitted
Done

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.

kossebau: Propose to add a short comment explaining why we return index(0) here for the future code…
160​ return d->model->index(0);
161​ }
156 for (int i = 0; i < d->model->rowCount(); ++i) { 162​ for (int i = 0; i < d->model->rowCount(); ++i) {
kossebauUnsubmitted
Done

Could start off at 1 now, no?

kossebau: Could start off at 1 now, no?
157 QModelIndex index = d->model->index(i); 163​ QModelIndex index = d->model->index(i);
158 if (index.data().toString() == name) { 164​ if (index.data().toString() == name) {
159 return index; 165​ return index;
160 } 166​ }
161 } 167​ }
162 return QModelIndex(); 168​ return QModelIndex();
163} 169​}
164 170​
165KActionMenu *KColorSchemeManager::createSchemeSelectionMenu(const QIcon &icon, const QString &name, const QString &selectedSchemeName, QObject *parent) 171​KActionMenu *KColorSchemeManager::createSchemeSelectionMenu(const QIcon &icon, const QString &name, const QString &selectedSchemeName, QObject *parent)
166{ 172​{
167 KActionMenu *menu = new KActionMenu(icon, name, parent); 173​ KActionMenu *menu = new KActionMenu(icon, name, parent);
168 QActionGroup *group = new QActionGroup(menu); 174​ QActionGroup *group = new QActionGroup(menu);
169 connect(group, &QActionGroup::triggered, [](QAction * action) { 175​ connect(group, &QActionGroup::triggered, this, [this](QAction * action) {
170 // hint for the style to synchronize the color scheme with the window manager/compositor 176​ activateScheme(d->model->index(action->data().toInt()));
171 qApp->setProperty("KDE_COLOR_SCHEME_PATH", action->data()); ​
172 qApp->setPalette(KColorScheme::createApplicationPalette(KSharedConfig::openConfig(action->data().toString()))); ​
173 }); 177​ });
174 for (int i = 0; i < d->model->rowCount(); ++i) { 178​ for (int i = 0; i < d->model->rowCount(); ++i) {
175 QModelIndex index = d->model->index(i); 179​ QModelIndex index = d->model->index(i);
176 QAction *action = new QAction(index.data(Qt::DisplayRole).toString(), menu); 180​ QAction *action = new QAction(index.data(Qt::DisplayRole).toString(), menu);
177 action->setData(index.data(Qt::UserRole)); 181​ action->setData(index.row());
178 action->setActionGroup(group); 182​ action->setActionGroup(group);
179 action->setCheckable(true); 183​ action->setCheckable(true);
180 if (index.data().toString() == selectedSchemeName) { 184​ if (index.data().toString() == selectedSchemeName) {
181 action->setChecked(true); 185​ action->setChecked(true);
182 } 186​ }
183 menu->addAction(action); 187​ menu->addAction(action);
184 } 188​ }
185 189​ if (!group->checkedAction()) {
190​ group->actions()[0]->setChecked(true);
191​ }
kossebauUnsubmitted
Done

also could get a short comment what the highlevel logic is here
// no color theme selected? so it's the default one

kossebau: also could get a short comment what the highlevel logic is here `// no color theme selected? so…
186 connect(menu->menu(), &QMenu::aboutToShow, group, [this, group] { 192​ connect(menu->menu(), &QMenu::aboutToShow, group, [this, group] {
187 const auto actions = group->actions(); 193​ const auto actions = group->actions();
188 for (QAction *action : actions) { 194​ for (QAction *action : actions) {
189 if (action->icon().isNull()) { 195​ if (action->icon().isNull()) {
190 action->setIcon(d->model->createPreview(action->data().toString())); 196​ action->setIcon(d->model->index(action->data().toInt()).data(Qt::DecorationRole).value<QIcon>());
191 } 197​ }
192 } 198​ }
193 }); 199​ });
194 200​
195 return menu; 201​ return menu;
196} 202​}
197 203​
198KActionMenu *KColorSchemeManager::createSchemeSelectionMenu(const QString &text, const QString &selectedSchemeName, QObject *parent) 204​KActionMenu *KColorSchemeManager::createSchemeSelectionMenu(const QString &text, const QString &selectedSchemeName, QObject *parent)
199{ 205​{
200 return createSchemeSelectionMenu(QIcon(), text, selectedSchemeName, parent); 206​ return createSchemeSelectionMenu(QIcon(), text, selectedSchemeName, parent);
201} 207​}
202 208​
203KActionMenu *KColorSchemeManager::createSchemeSelectionMenu(const QString &selectedSchemeName, QObject *parent) 209​KActionMenu *KColorSchemeManager::createSchemeSelectionMenu(const QString &selectedSchemeName, QObject *parent)
204{ 210​{
205 return createSchemeSelectionMenu(QIcon(), QString(), selectedSchemeName, parent); 211​ return createSchemeSelectionMenu(QIcon(), QString(), selectedSchemeName, parent);
206} 212​}
207 213​
214​KActionMenu *KColorSchemeManager::createSchemeSelectionMenu (QObject *parent)
215​{
216​ return createSchemeSelectionMenu(QIcon(),QString(), QString(), parent);
asemkeUnsubmitted
Done

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.

asemke: why not to use reasonable default values here like QIcon::fromTheme(QStringLiteral("preferences…
davidreAuthorUnsubmitted
Not Done

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 &selectedSchemeName, QObject *parent) probably if the custom scheme is saved between launches.

davidre: Should we also do this for the other overloads? Or would that behavior change be a blocker?
asemkeUnsubmitted
Not Done

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.

asemke: I'm not the author of this code but I don't see why this should be a blocker. With this you…
217​}
218​
208void KColorSchemeManager::activateScheme(const QModelIndex &index) 219​void KColorSchemeManager::activateScheme(const QModelIndex &index)
209{ 220​{
210 if (!index.isValid()) { 221​ if (!index.isValid()) {
211 return; 222​ return;
212 } 223​ }
213 if (index.model() != d->model.data()) { 224​ if (index.model() != d->model.data()) {
214 return; 225​ return;
215 } 226​ }
216 // hint for the style to synchronize the color scheme with the window manager/compositor 227​ // hint for the style to synchronize the color scheme with the window manager/compositor
kossebauUnsubmitted
Done

Using // for each comment line is more typical for explanation comments, why the different style here?

kossebau: Using `//` for each comment line is more typical for explanation comments, why the different…
217 qApp->setProperty("KDE_COLOR_SCHEME_PATH", index.data(Qt::UserRole)); 228​ qApp->setProperty("KDE_COLOR_SCHEME_PATH", index.data(Qt::UserRole));
229​ if (index.data(Qt::UserRole).toString().isNull()) {
asemkeUnsubmitted
Done

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

asemke: if (index.row() == 0) would also do the job and is simpler and faster.
davidreAuthorUnsubmitted
Not Done

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

davidre: Because I didn't see another multiline comment in this file to compare to ;)
230​ qApp->setPalette(qApp->style()->standardPalette());
231​ } else {
218 qApp->setPalette(KColorScheme::createApplicationPalette(KSharedConfig::openConfig(index.data(Qt::UserRole).toString()))); 232​ qApp->setPalette(KColorScheme::createApplicationPalette(KSharedConfig::openConfig(index.data(Qt::UserRole).toString())));
219} 233​ }
234​}
235​
236​void KColorSchemeManager::followSystemScheme()
237​{
238​ activateScheme(d->model->index(0));
239​}