Differential D12974 Diff 34652 kcms/workspaceoptions/workspaceoptions.cpp

Changeset View

Standalone View

View Options

kcms/workspaceoptions/workspaceoptions.cpp

Show All 15 Lines
16 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. 16​ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
17 */ 17​ */
18#include "workspaceoptions.h" 18​#include "workspaceoptions.h"
19 19​
20#include <KPluginFactory> 20​#include <KPluginFactory>
21#include <KAboutData> 21​#include <KAboutData>
22#include <KLocalizedString> 22​#include <KLocalizedString>
23#include <KConfigGroup> 23​#include <KConfigGroup>
24#include <ksharedconfig.h> 24​#include <KSharedConfig>
25#include <KDELibs4Support/KDE/KGlobalSettings> 25​#include <KGlobalSettings>
26#include <../migrationlib/kdelibs4config.h> 26​#include <../migrationlib/kdelibs4config.h>
27 27​
28#include <QtDBus/QDBusMessage> 28​#include <QDBusMessage>
29#include <QtDBus/QDBusConnection> 29​#include <QDBusConnection>
zzagUnsubmitted
Done

https://community.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes

zzag: https://community.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes
30 30​
31K_PLUGIN_FACTORY_WITH_JSON(KCMWorkspaceOptionsFactory, "kcm_workspace.json", registerPlugin<KCMWorkspaceOptions>();) 31​K_PLUGIN_FACTORY_WITH_JSON(KCMWorkspaceOptionsFactory, "kcm_workspace.json", registerPlugin<KCMWorkspaceOptions>();)
32 32​
33KCMWorkspaceOptions::KCMWorkspaceOptions(QObject *parent, const QVariantList& args) 33​KCMWorkspaceOptions::KCMWorkspaceOptions(QObject *parent, const QVariantList& args)
34 : KQuickAddons::ConfigModule(parent, args), 34​ : KQuickAddons::ConfigModule(parent, args),
35 m_stateToolTip(true), 35​ m_toolTipCurrentState(true),
36 m_stateVisualFeedback(true), 36​ m_visualFeedbackCurrentState(true),
37 m_stateSingleClick(true) 37​ m_singleClickCurrentState(true)
zzagUnsubmitted
Done

I think you could do something like this in the header file

class ...  {
...
    m_stateToolTip = true;
    m_stateVisualFeedback = true;
    m_stateSingleClick = true;
...
};
zzag: I think you could do something like this in the header file ```lang=cpp class ... { ...
furkantokacAuthorUnsubmitted
Done

This one is more clear imho because they are not optional initializations, they are must so we emphasize that.

furkantokac: This one is more clear imho because they are not optional initializations, they are must so we…
38{ 38​{
39 KAboutData* about = new KAboutData(QStringLiteral("kcm_workspace"), 39​ KAboutData* about = new KAboutData(QStringLiteral("kcm_workspace"),
40 i18n("Plasma Workspace global options"), 40​ i18n("Plasma Workspace global options"),
41 QStringLiteral("1.1"), 41​ QStringLiteral("1.1"),
42 i18n("System Settings module for managing global options for the Plasma Workspace."), 42​ i18n("System Settings module for managing global options for the Plasma Workspace."),
43 KAboutLicense::GPL); 43​ KAboutLicense::GPL);
44 44​
45 about->addAuthor(i18n("Furkan Tokac"), QString(), QStringLiteral("furkantokac34@gmail.com")); 45​ about->addAuthor(i18n("Furkan Tokac"), QString(), QStringLiteral("furkantokac34@gmail.com"));
46 setAboutData(about); 46​ setAboutData(about);
47 47​
48 setButtons( Default | Apply ); 48​ setButtons(Default | Apply);
49} 49​}
50 50​
51/*ConfigModule functions*/ ​
52void KCMWorkspaceOptions::load() 51​void KCMWorkspaceOptions::load()
ngrahamUnsubmitted
Done

Why all the asterisks?

ngraham: Why all the asterisks?
53{ 52​{
54 bool state = false; 53​ loadPlasmarc();
55 KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("plasmarc")); 54​ loadKdeglobals();
56 ​
57 // Load toolTip ​
58 { ​
59 const KConfigGroup cg(config, QStringLiteral("PlasmaToolTips")); ​
60 state = cg.readEntry("Delay", 0.7) > 0; ​
61 setToolTip(state); ​
62 m_ostateToolTip = getToolTip(); ​
63 } ​
64 ​
65 // Load visualFeedback ​
66 { ​
67 const KConfigGroup cg(config, QStringLiteral("OSD")); ​
68 state = cg.readEntry(QStringLiteral("Enabled"), true); ​
69 setVisualFeedback(state); ​
70 m_ostateVisualFeedback = getVisualFeedback(); ​
71 } ​
72 ​
73 // Load singleClick ​
74 { ​
75 KSharedConfig::Ptr configKdeGlobals = KSharedConfig::openConfig(QStringLiteral("kdeglobals")); ​
76 const KConfigGroup cg(configKdeGlobals, QStringLiteral("KDE")); ​
77 state = cg.readEntry(QStringLiteral("SingleClick"), true); ​
78 setSingleClick(state); ​
79 m_ostateSingleClick = getSingleClick(); ​
80 } ​
81 55​
82 setNeedsSave(false); 56​ setNeedsSave(false);
83} 57​}
84 58​
85void KCMWorkspaceOptions::save() 59​void KCMWorkspaceOptions::save()
86{ 60​{
87 KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("plasmarc")); 61​ savePlasmarc();
88 62​ saveKdeglobals();
davidedmundsonUnsubmitted
Done

We want to batch our syncs to plasmarc, which the old code did better.

I wouldn't bother trying to be clever with checking if it's the original state or not here, as KConfig will do all of that for us anyway at a more correct lower level.

davidedmundson: We want to batch our syncs to plasmarc, which the old code did better. I wouldn't bother…
romanggUnsubmitted
Done

For the sync yes.

For checking state: the old code wrote always all values to the config file, with or without the user changing them at all. It might make sense to only write config values which have been explicitly changed by the user, because this would mean that we can later on change the default of values of this user which have never been explicitly changed by the user before with an update of just the KCM code.

romangg: For the sync yes. For checking state: the old code wrote always all values to the config file…
furkantokacAuthorUnsubmitted
Not Done

Nice point. I tested the situation for the old code. When I just change single/double click option, other option files (plasmarc) are not opened. That means KConfig handles the situation. Thank you. Correction will be done now.

furkantokac: Nice point. I tested the situation for the old code. When I just change single/double click…
89 // Save toolTip ​
90 { ​
91 KConfigGroup cg(config, QStringLiteral("PlasmaToolTips")); ​
92 cg.writeEntry("Delay", getToolTip() ? 0.7 : -1); ​
93 m_ostateToolTip = getToolTip(); ​
94 } ​
95 ​
96 // Save visualFeedback ​
97 { ​
98 KConfigGroup cg(config, QStringLiteral("OSD")); ​
99 cg.writeEntry("Enabled", getVisualFeedback()); ​
100 m_ostateVisualFeedback = getVisualFeedback(); ​
101 } ​
102 ​
103 // Save singleClick ​
104 KSharedConfig::Ptr configKdeGlobals = KSharedConfig::openConfig(QStringLiteral("kdeglobals")); ​
105 KConfigGroup cg(configKdeGlobals, QStringLiteral("KDE")); ​
106 cg.writeEntry("SingleClick", getSingleClick()); ​
107 m_ostateSingleClick = getSingleClick(); ​
108 config->sync(); ​
109 configKdeGlobals->sync(); ​
110 ​
111 Kdelibs4SharedConfig::syncConfigGroup(QLatin1String("KDE"), "kdeglobals"); ​
112 ​
113 QDBusMessage message = QDBusMessage::createSignal("/KGlobalSettings", "org.kde.KGlobalSettings", "notifyChange"); ​
114 QList<QVariant> args; ​
115 args.append(KGlobalSettings::SettingsChanged); ​
116 args.append(KGlobalSettings::SETTINGS_MOUSE); ​
117 message.setArguments(args); ​
118 QDBusConnection::sessionBus().send(message); ​
119 63​
120 setNeedsSave(false); 64​ setNeedsSave(false);
121} 65​}
122 66​
123void KCMWorkspaceOptions::defaults() 67​void KCMWorkspaceOptions::defaults()
124{ 68​{
125 setToolTip(true); 69​ setToolTip(true);
126 setVisualFeedback(true); 70​ setVisualFeedback(true);
127 setSingleClick(true); 71​ setSingleClick(true);
128 72​
129 handleNeedsSave(); 73​ handleNeedsSave();
130} 74​}
131 75​
132/*ToolTip functions*/ ​
133bool KCMWorkspaceOptions::getToolTip() const 76​bool KCMWorkspaceOptions::getToolTip() const
134{ 77​{
135 return m_stateToolTip; 78​ return m_toolTipCurrentState;
136} 79​}
137 80​
138void KCMWorkspaceOptions::setToolTip(bool state) 81​void KCMWorkspaceOptions::setToolTip(bool state)
139{ 82​{
140 // Prevent from binding loop 83​ // Prevent from binding loop
141 if( m_stateToolTip == state ) { 84​ if (m_toolTipCurrentState == state) {
142 return; 85​ return;
143 } 86​ }
144 87​
145 m_stateToolTip = state; 88​ m_toolTipCurrentState = state;
146 89​
147 emit toolTipChanged(); 90​ emit toolTipChanged();
148 handleNeedsSave(); 91​ handleNeedsSave();
149} 92​}
150 93​
151/*VisualFeedback functions*/ ​
152bool KCMWorkspaceOptions::getVisualFeedback() const 94​bool KCMWorkspaceOptions::getVisualFeedback() const
153{ 95​{
154 return m_stateVisualFeedback; 96​ return m_visualFeedbackCurrentState;
155} 97​}
156 98​
157void KCMWorkspaceOptions::setVisualFeedback(bool state) 99​void KCMWorkspaceOptions::setVisualFeedback(bool state)
158{ 100​{
159 // Prevent from binding loop 101​ // Prevent from binding loop
160 if( m_stateVisualFeedback == state ) { 102​ if (m_visualFeedbackCurrentState == state) {
161 return; 103​ return;
162 } 104​ }
163 105​
164 m_stateVisualFeedback = state; 106​ m_visualFeedbackCurrentState = state;
165 107​
166 emit visualFeedbackChanged(); 108​ emit visualFeedbackChanged();
167 handleNeedsSave(); 109​ handleNeedsSave();
168} 110​}
169 111​
170/*SingleClick functions*/ ​
171bool KCMWorkspaceOptions::getSingleClick() const 112​bool KCMWorkspaceOptions::getSingleClick() const
zzagUnsubmitted
Done

I would remove it. It doesn't add any useful information. I don't see any word about commets in the kdelibs coding style(I assume Plasma follows it) but as a rule of thumb: comment things that are not obvious.

For example, see https://google.github.io/styleguide/cppguide.html#Implementation_Comments

zzag: I would remove it. It doesn't add any useful information. I don't see any word about commets in…
furkantokacAuthorUnsubmitted
Done

Actually this is optional. Not a big issue. Makes it easier to follow. Specifically for this patch, it's okay imho.

furkantokac: Actually this is optional. Not a big issue. Makes it easier to follow. Specifically for this…
172{ 113​{
173 return m_stateSingleClick; 114​ return m_singleClickCurrentState;
174} 115​}
175 116​
176void KCMWorkspaceOptions::setSingleClick(bool state) 117​void KCMWorkspaceOptions::setSingleClick(bool state)
177{ 118​{
178 // Prevent from binding loop 119​ // Prevent from binding loop
179 if( m_stateSingleClick == state ) { 120​ if (m_singleClickCurrentState == state) {
180 return; 121​ return;
181 } 122​ }
182 123​
183 m_stateSingleClick = state; 124​ m_singleClickCurrentState = state;
184 125​
185 emit singleClickChanged(); 126​ emit singleClickChanged();
186 handleNeedsSave(); 127​ handleNeedsSave();
187} 128​}
188 129​
189/*Other functions*/ 130​void KCMWorkspaceOptions::loadPlasmarc()
190// Checks if the current states are different than the first states. 131​{
191// If yes, setNeedsSave(true). 132​ KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("plasmarc"));
133​ bool state;
134​
135​ // Load toolTip
136​ {
137​ const KConfigGroup cg(config, QStringLiteral("PlasmaToolTips"));
138​
139​ state = cg.readEntry("Delay", 0.7) > 0;
140​ setToolTip(state);
141​ m_toolTipOriginalState = state;
142​ }
143​
144​ // Load visualFeedback
145​ {
146​ const KConfigGroup cg(config, QStringLiteral("OSD"));
147​
148​ state = cg.readEntry(QStringLiteral("Enabled"), true);
149​ setVisualFeedback(state);
150​ m_visualFeedbackOriginalState = state;
151​ }
152​}
153​
154​void KCMWorkspaceOptions::loadKdeglobals()
155​{
156​ KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kdeglobals"));
157​ bool state;
158​
159​ // Load singleClick
160​ {
161​ const KConfigGroup cg(config, QStringLiteral("KDE"));
162​
163​ state = cg.readEntry(QStringLiteral("SingleClick"), true);
164​ setSingleClick(state);
165​ m_singleClickOriginalState = state;
166​ }
167​}
168​
169​void KCMWorkspaceOptions::savePlasmarc()
170​{
171​ KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("plasmarc"));
172​ bool state;
173​
174​ // Save toolTip
175​ {
176​ KConfigGroup cg(config, QStringLiteral("PlasmaToolTips"));
177​
178​ state = getToolTip();
179​ cg.writeEntry("Delay", state ? 0.7 : -1);
180​ m_toolTipOriginalState = state;
181​ }
182​
183​ // Save visualFeedback
184​ {
185​ KConfigGroup cg(config, QStringLiteral("OSD"));
186​
187​ state = getVisualFeedback();
188​ cg.writeEntry("Enabled", state);
189​ m_visualFeedbackOriginalState = state;
190​ }
191​
192​ config->sync();
193​}
194​
195​void KCMWorkspaceOptions::saveKdeglobals()
196​{
197​ KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kdeglobals"));
198​ bool state;
199​
200​ // Save singleClick
201​ {
202​ KConfigGroup cg(config, QStringLiteral("KDE"));
203​
204​ state = getSingleClick();
205​ cg.writeEntry("SingleClick", state);
206​ m_singleClickOriginalState = state;
207​ }
208​
209​ config->sync();
210​
211​ Kdelibs4SharedConfig::syncConfigGroup(QLatin1String("KDE"), "kdeglobals");
212​
213​ QDBusMessage message = QDBusMessage::createSignal("/KGlobalSettings", "org.kde.KGlobalSettings", "notifyChange");
214​ QList<QVariant> args;
215​ args.append(KGlobalSettings::SettingsChanged);
216​ args.append(KGlobalSettings::SETTINGS_MOUSE);
217​ message.setArguments(args);
218​ QDBusConnection::sessionBus().send(message);
219​}
220​
221​// Checks if the current states are different than the first states. If yes, setNeedsSave(true).
192void KCMWorkspaceOptions::handleNeedsSave() 222​void KCMWorkspaceOptions::handleNeedsSave()
193{ 223​{
194 setNeedsSave(m_ostateToolTip != getToolTip() || 224​ setNeedsSave(m_toolTipOriginalState != getToolTip() ||
195 m_ostateVisualFeedback != getVisualFeedback() || 225​ m_visualFeedbackOriginalState != getVisualFeedback() ||
196 m_ostateSingleClick != getSingleClick()); 226​ m_singleClickOriginalState != getSingleClick());
197} 227​}
198 228​
199#include "workspaceoptions.moc" 229​#include "workspaceoptions.moc"