D13899: KMessageWidget: use theme instead of hardcoded colours

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180705/953e7b4b/attachment-0001.html>