D24743: Update GTK settings according to Plasma settings

[Kai Uwe Broulik]

broulik added a comment.


  Pretty cool!
  Some minor nitpicks.
  Once all the KCMs have been ported to KConfigXT (XML config description, currently ongoing) we'll have a compile-time checked way to read KDE settings and easier way to get the default values.

INLINE COMMENTS

> CMakeLists.txt:9
>  find_package(Qt5 REQUIRED NO_MODULE COMPONENTS Widgets Svg Test)
> -find_package(KF5 REQUIRED COMPONENTS I18n KIO ConfigWidgets NewStuff Archive KCMUtils IconThemes)
> +find_package(KF5 REQUIRED COMPONENTS I18n KIO ConfigWidgets NewStuff Archive KCMUtils IconThemes KDELibs4Support)
>  find_package(X11 REQUIRED)

Please don't use `KDELibs4Support` in new code

> configeditor.cpp:34
> +
> +#undef signals
> +#include <gio/gio.h>

Alternatively you could compile the project with `QT_NO_KEYWORDS`

> configeditor.cpp:105
> +{
> +    QRegularExpression regExp(paramName + "=[^\n]*($|\n)");
> +

make `static` so it doesn't have to recreate it every time?

> configeditor.cpp:170
> +    char line[512];
> +    FILE *cmd(popen("pidof -s xsettingsd", "r"));
> +    fgets(line, 512, cmd);

I would prefer using more higher level `QProcess` here

> configeditor.h:22
> +
> +#include <csignal>
> +

Perhaps `<unistd.h>`?

> configvalueprovider.cpp:38
> +{
> +    KIconTheme *newIconTheme(KIconLoader::global()->theme());
> +    return newIconTheme->internalName();

`theme()` can return null

> configvalueprovider.cpp:53
> +    KConfigGroup configGroup(config->group(QStringLiteral("KDE")));
> +    QString kdeConfigValue(configGroup.readEntry(QStringLiteral("ShowIconsOnPushButtons")));
> +

Pass `true` as second argument (which is the default), then it will return you a `bool` directly.

> configvalueprovider.cpp:79
> +    KConfigGroup configGroup(config->group(QStringLiteral("Toolbar style")));
> +    QString kdeConfigValue(configGroup.readEntry(QStringLiteral("ToolButtonStyle")));
> +    return getToolbarStyleInDesiredNotation(kdeConfigValue, notation);

Default value `TextBesideIcon`

> configvalueprovider.h:1
> +#pragma once
> +

Missing copyright header

> configvalueprovider.h:10
> +    enum class ToolbarStyleNotation {
> +        XSETTINGSD = 0,
> +        SETTINGS_INI,

We typically don't use all caps for enums

> configvalueprovider.h:17-22
> +    QString getConfigFontName(const QFont &font);
> +    QString getConfigIconThemeName();
> +    QString getConfigCursorThemeName();
> +    QString getIconsOnButtonsConfigValue();
> +    QString getIconsInMenusConfigValue();
> +    QString getToolbarStyle(ToolbarStyleNotation notation);

All of those `const`?

> gtkconfig.cpp:37
> +GtkConfig::GtkConfig(QObject *parent, const QVariantList&) :
> +    KDEDModule(parent), configValueProvider(new ConfigValueProvider()), configEditor(new ConfigEditor())
> +{

One variable per line, please

> gtkconfig.cpp:47
> +                                          SLOT(onGlobalSettingsChange(int,int)));
> +}
> +

Should this initially apply all the settings?

> gtkconfig.cpp:110
> +{
> +    using ChangeType = KGlobalSettings::ChangeType;
> +    using SettingsCategory = KGlobalSettings::SettingsCategory;

You'll have to copy those enum values in since `KGlobalSettings` is deprecated, sorry.

> gtkconfig.cpp:116
> +    } else if (changeType == ChangeType::SettingsChanged && arg == SettingsCategory::SETTINGS_STYLE) {
> +        // Since KGlobalSettings::ChangeType::ToolbarStyleChanged is not working,
> +        // we use the style settings category as a whole to change the respective settings

In what way? There's a comment in the style KCM saying

  // ##### FIXME - Doesn't apply all settings correctly due to bugs in KApplication/KToolbar
  KGlobalSettings::self()->emitChange(KGlobalSettings::ToolbarStyleChanged);

> gtkconfig.h:24
> +
> +#include <QFile>
> +

Unused

> gtkconfig.h:33
> +{
> +    Q_CLASSINFO("D-Bus Interface", "org.kde.gtkconfig")
> +    Q_OBJECT

Unused

REPOSITORY
  R99 KDE Gtk Configuration Tool

REVISION DETAIL
  https://phabricator.kde.org/D24743

To: gikari, #plasma, #vdg
Cc: broulik, nicolasfella, plasma-devel, LeGast00n, The-Feren-OS-Dev, cblack, konkinartem, ian, jguidon, hannahk, Ghost6, jraleigh, MrPepe, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, ragreen, mglb, crozbo, ndavis, ZrenBot, firef, ngraham, alexeymin, skadinna, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, mbohlender, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191018/8754c3da/attachment-0001.html>