broulik added a comment.
I like the idea INLINE COMMENTS > kmessagewidget.cpp:172 > QColor bgBaseColor; > + const QPalette palette = QGuiApplication::palette(); > Not using the widget's palette was intentional for Konsole or something I recall? > kmessagewidget.cpp:176 > + // and therefore can't depend on any other KDE Frameworks. We thus try > to get > + // the colours that interest us directly from the theme definition for > or from the > + // global settings store, using KThemeSettings. *colors > kmessagewidget.cpp:178 > + // global settings store, using KThemeSettings. > + // It that fails we use hardcoded colours (from kcolorscheme.cpp), or > the highlight > + // colour (for Information). *If *colors > kmessagewidget.cpp:200 > + const QColor windowColor = > settings.readRGB(QStringLiteral("BackgroundNormal"), > palette.window().color()); > + QColor textColor = settings.readRGB(QStringLiteral("ForegroundNormal"), > palette.text().color());; > const QColor border = bgBaseColor; Stray semi-colon > kthemesettings.cpp:28 > + // Check if the KColorSchemeManager was used to activate a custom theme > + QString themePath = qApp->property("KDE_COLOR_SCHEME_PATH").toString(); > + if (themePath.isEmpty()) { Is this explicit check really neccessary? It should set a palette on `qApp` in `frameworkintegratio or `KColorSchemeManager` > kthemesettings.cpp:30 > + if (themePath.isEmpty()) { > + themePath = QStandardPaths::locate(QStandardPaths::ConfigLocation, > QStringLiteral("kdeglobals")); > + } This doesn't cascade to system-wide settings > kthemesettings.cpp:32 > + } > + if (!themePath.isEmpty()) { > + m_settings = new QSettings(themePath, QSettings::IniFormat); return early? > kthemesettings.cpp:35 > + } > + if (m_settings && !initialGroup.isEmpty()){ > + if (m_settings->childGroups().contains(initialGroup)) { Coding style, space before brace > kthemesettings.cpp:40 > + delete m_settings; > + m_settings = nullptr; > + } Maybe also add a bool KThemeSettings::isValid() const { return m_settings != nullptr; } > kthemesettings.cpp:76 > +{ > + if (m_settings && m_settings->contains(key)) { > + QStringList rgb = m_settings->value(key).toStringList(); This `contains(key)` check looks unneccessary: > If the setting doesn't exist, returns defaultValue. so you get an invalid `QVariant` which will give you an empty `QStringList` > kthemesettings_p.h:24 > + > +#include <QStringList> > +#include <QStandardPaths> Forward-declare, include in cpp > kthemesettings_p.h:25 > +#include <QStringList> > +#include <QStandardPaths> > +#include <QSettings> Move to cpp > kthemesettings_p.h:26 > +#include <QStandardPaths> > +#include <QSettings> > +#include <QColor> Forward-declare, include in cpp > kthemesettings_p.h:85 > + */ > + QColor readRGB(const QString &key, QColor defaultValue = QColor()); > + `readRgb` > kthemesettings_p.h:88 > +private: > + QSettings *m_settings = nullptr; > +}; Use `QScopedPointer` to avoid having to manually delete REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13899 To: rjvbb, #frameworks, #vdg Cc: broulik, kde-frameworks-devel, michaelh, crozbo, firef, ngraham, bruns, skadinna, aaronhoneycutt, mbohlender