D24743: Update GTK settings according to Plasma settings

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> configeditor.cpp:49
> +    QString configLocation(QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation));
> +    QString gtk3ConfigPath(configLocation + "/gtk-3.0/settings.ini");
> +

Better wrap the const char* in a QStringLiteral.

Also, but more of a nitpick this time: style wise I'd favor using = for those initialization (and others in the file) than parenthesis. It gets too close the most vexing parse territory, I'd rather not potentially expose a future developer touching that file to it.

> configeditor.cpp:81
> +{
> +    QString gtkrcPath(QDir::homePath() + "/.gtkrc-2.0");
> +    QFile gtkrc(gtkrcPath);

QStringLiteral please.

> configeditor.cpp:96
> +    } else {
> +        return "";
> +    }

QString() or {} instead of ""

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

QStringLiteral please

There are more of those in the file, I'll stop pointing them out. ;-)

> configeditor.h:45
> +    QString readFileContents(QFile &gtkrc) const;
> +    pid_t getPidOfXSettingsd() const;
> +};

We don't prefix getters with get.

> configvalueprovider.cpp:35
> +
> +    KSharedConfigPtr config(KSharedConfig::openConfig(QStringLiteral("kdeglobals")));
> +    KConfigGroup configGroup(config->group(QStringLiteral("General")));

I will contradict my previous comment about making those functions static or turning into a namespace. I think it'd be better to avoid creating and ditching those KSharedConfig instances at each call, so why not instantiate it when ConfigValueProvider is created?

You might have to load() from the config at right point in times though.

> configvalueprovider.h:37
> +
> +    QString getConfigFontName() const;
> +    QString getConfigIconThemeName() const;

We don't prefix getters with get

Beside none of those have any state AFAICT, why not make them static or turn this into a namespace?

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

To: gikari, #plasma, #vdg
Cc: ervin, ngraham, 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, 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/20191030/cfe844f1/attachment-0001.html>