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
