Differential D16425 Diff 56023 daemon/actions/bundled/suspendsessionconfig.cpp

Changeset View

Standalone View

View Options

daemon/actions/bundled/suspendsessionconfig.cpp

Show All 23 Lines
24 24​
25#include <QHBoxLayout> 25​#include <QHBoxLayout>
26#include <QSpinBox> 26​#include <QSpinBox>
27 27​
28#include <KComboBox> 28​#include <KComboBox>
29#include <KLocalizedString> 29​#include <KLocalizedString>
30#include <KPluginFactory> 30​#include <KPluginFactory>
31#include <KSharedConfig> 31​#include <KSharedConfig>
32​#include <QCheckBox>
32#include <QIcon> 33​#include <QIcon>
33#include <KConfig> 34​#include <KConfig>
34#include "suspendsession.h" 35​#include "suspendsession.h"
35 36​
36K_PLUGIN_FACTORY(PowerDevilSuspendSessionConfigFactory, registerPlugin<PowerDevil::BundledActions::SuspendSessionConfig>(); ) 37​K_PLUGIN_FACTORY(PowerDevilSuspendSessionConfigFactory, registerPlugin<PowerDevil::BundledActions::SuspendSessionConfig>(); )
37 38​
38namespace PowerDevil { 39​namespace PowerDevil {
39namespace BundledActions { 40​namespace BundledActions {
40 41​
41SuspendSessionConfig::SuspendSessionConfig(QObject* parent, const QVariantList&) 42​SuspendSessionConfig::SuspendSessionConfig(QObject* parent, const QVariantList&)
42 : ActionConfig(parent) 43​ : ActionConfig(parent)
43{ 44​{
44 45​
abalajiUnsubmitted
Done

Move this to the initializer

abalaji: Move this to the initializer
avaldesAuthorUnsubmitted
Done

something like this?

: ActionConfig(parent),
  m_suspendThenHibernateEnabled(nullptr)
avaldes: something like this? ``` : ActionConfig(parent)…
abalajiUnsubmitted
Done

Yeah exactly. So a subtle detail of C++ constructors is any data members which are not primitive (like structs class instances) get implicitly initialized using their default constructor if they don't have an entry in the initializer list. So if you do everything in the constructor body you risk double initializing stuff, which is unnecessary overhead. Of course this is not the case with primitives like pointers which are just integers behind the scenes, so what you have is okay, but just for the sake of consistency, better to make use of the initializer list

abalaji: Yeah exactly. So a subtle detail of C++ constructors is any data members which are not…
45} 46​}
46 47​
47SuspendSessionConfig::~SuspendSessionConfig() 48​SuspendSessionConfig::~SuspendSessionConfig()
48{ 49​{
49 50​
50} 51​}
51 52​
52void SuspendSessionConfig::save() 53​void SuspendSessionConfig::save()
53{ 54​{
54 configGroup().writeEntry< uint >("suspendType", m_comboBox->itemData(m_comboBox->currentIndex()).toUInt()); 55​ configGroup().writeEntry< uint >("suspendType", m_comboBox->itemData(m_comboBox->currentIndex()).toUInt());
55 configGroup().writeEntry("idleTime", m_idleTime->value() * 60 * 1000); 56​ configGroup().writeEntry("idleTime", m_idleTime->value() * 60 * 1000);
57​ if (m_suspendThenHibernateEnabled) {
abalajiUnsubmitted
Done

Again

configGroup().writeEntry<bool>("suspendThenHibernate", m_suspendThenHibernateEnabled != nullptr && m_suspendThenHibernateEnabled->isChecked());
abalaji: Again ``` configGroup().writeEntry<bool>("suspendThenHibernate", m_suspendThenHibernateEnabled !
58​ configGroup().writeEntry<bool>(
59​ "suspendThenHibernate", m_suspendThenHibernateEnabled->isChecked());
60​ } else {
61​ configGroup().writeEntry<bool>("suspendThenHibernate", false);
62​ }
56 63​
57 configGroup().sync(); 64​ configGroup().sync();
58} 65​}
59 66​
60void SuspendSessionConfig::load() 67​void SuspendSessionConfig::load()
61{ 68​{
62 configGroup().config()->reparseConfiguration(); 69​ configGroup().config()->reparseConfiguration();
63 70​
64 uint suspendType = configGroup().readEntry< uint >("suspendType", 0); 71​ uint suspendType = configGroup().readEntry< uint >("suspendType", 0);
65 m_comboBox->setCurrentIndex(m_comboBox->findData(suspendType)); 72​ m_comboBox->setCurrentIndex(m_comboBox->findData(suspendType));
66 m_idleTime->setValue((configGroup().readEntry<int>("idleTime", 600000) / 60) / 1000); 73​ m_idleTime->setValue((configGroup().readEntry<int>("idleTime", 600000) / 60) / 1000);
74​ if (m_suspendThenHibernateEnabled) {
75​ m_suspendThenHibernateEnabled->setChecked(configGroup().readEntry<bool>("suspendThenHibernate", false));
76​ }
67} 77​}
68 78​
69QList< QPair< QString, QWidget* > > SuspendSessionConfig::buildUi() 79​QList< QPair< QString, QWidget* > > SuspendSessionConfig::buildUi()
70{ 80​{
71 QWidget *tempWidget = new QWidget; 81​ QWidget *tempWidget = new QWidget;
72 QHBoxLayout *hlay = new QHBoxLayout; 82​ QHBoxLayout *hlay = new QHBoxLayout;
ngrahamUnsubmitted
Done
  1. The first argument is the context, which doesn't need to be the same string as the actual user-displayed string itself. It's mainly used for very short strings where the string itself may not illustrate on its own what the context is and how it should be translated. Since this string is descriptive enough, I'd just use a regular old i18n("blablabla")
  1. We now use the word "Sleep" instead of "Suspend", and now that I look at this some more, I think a better string would be, "Sleep for 3 hours, then hibernate"
ngraham: 1. The first argument is the context, which doesn't need to be the same string as the actual…
83​ m_suspendThenHibernateEnabled = new QCheckBox(i18n("Sleep for 3 hours, then hibernate"));
73 m_comboBox = new KComboBox; 84​ m_comboBox = new KComboBox;
74 m_idleTime = new QSpinBox; 85​ m_idleTime = new QSpinBox;
75 m_idleTime->setMaximumWidth(150); 86​ m_idleTime->setMaximumWidth(150);
76 m_idleTime->setMinimum(1); 87​ m_idleTime->setMinimum(1);
77 m_idleTime->setMaximum(360); 88​ m_idleTime->setMaximum(360);
78 m_idleTime->setValue(0); 89​ m_idleTime->setValue(0);
79 m_idleTime->setSuffix(i18n(" min")); 90​ m_idleTime->setSuffix(i18n(" min"));
80 91​
81 if (PowerManagement::instance()->canSuspend()) { 92​ if (PowerManagement::instance()->canSuspend()) {
82 m_comboBox->addItem(QIcon::fromTheme("system-suspend"), i18nc("Suspend to RAM", "Sleep"), (uint)SuspendSession::ToRamMode); 93​ m_comboBox->addItem(QIcon::fromTheme("system-suspend"), i18nc("Suspend to RAM", "Sleep"), (uint)SuspendSession::ToRamMode);
83 } 94​ }
84 if (PowerManagement::instance()->canHibernate()) { 95​ if (PowerManagement::instance()->canHibernate()) {
85 m_comboBox->addItem(QIcon::fromTheme("system-suspend-hibernate"), i18n("Hibernate"), (uint)SuspendSession::ToDiskMode); 96​ m_comboBox->addItem(QIcon::fromTheme("system-suspend-hibernate"), i18n("Hibernate"), (uint)SuspendSession::ToDiskMode);
86 } 97​ }
87 if (PowerManagement::instance()->canHybridSuspend()) { 98​ if (PowerManagement::instance()->canHybridSuspend()) {
88 m_comboBox->addItem(QIcon::fromTheme("system-suspend-hybrid"), i18n("Hybrid sleep"), (uint)SuspendSession::SuspendHybridMode); 99​ m_comboBox->addItem(QIcon::fromTheme("system-suspend-hybrid"), i18n("Hybrid sleep"), (uint)SuspendSession::SuspendHybridMode);
89 } 100​ }
90 m_comboBox->addItem(QIcon::fromTheme("system-shutdown"), i18n("Shut down"), (uint)SuspendSession::ShutdownMode); 101​ m_comboBox->addItem(QIcon::fromTheme("system-shutdown"), i18n("Shut down"), (uint)SuspendSession::ShutdownMode);
91 m_comboBox->addItem(QIcon::fromTheme("system-lock-screen"), i18n("Lock screen"), (uint)SuspendSession::LockScreenMode); 102​ m_comboBox->addItem(QIcon::fromTheme("system-lock-screen"), i18n("Lock screen"), (uint)SuspendSession::LockScreenMode);
92 103​
93 hlay->addWidget(m_idleTime); 104​ hlay->addWidget(m_idleTime);
94 hlay->addWidget(m_comboBox); 105​ hlay->addWidget(m_comboBox);
95 hlay->addStretch(); 106​ hlay->addStretch();
abalajiUnsubmitted
Done

Stray line swap

abalaji: Stray line swap
avaldesAuthorUnsubmitted
Done

Done on purpose to change the UI:

Automatically, m_comboBox, m_idleTime

avaldes: Done on purpose to change the UI: {F6822188} Automatically, m_comboBox, m_idleTime
abalajiUnsubmitted
Done

Oh i see, that actually makes sense

abalaji: Oh i see, that actually makes sense
96 107​
97 tempWidget->setLayout(hlay); 108​ tempWidget->setLayout(hlay);
98 109​
99 QList< QPair< QString, QWidget* > > retlist; 110​ QList< QPair< QString, QWidget* > > retlist;
100 retlist.append(qMakePair< QString, QWidget* >(i18n("After"), tempWidget)); 111​ retlist.append(qMakePair< QString, QWidget* >(i18n("After"), tempWidget));
101 112​
102 connect(m_comboBox, SIGNAL(currentIndexChanged(int)), this, SLOT(setChanged())); 113​ connect(m_comboBox, SIGNAL(currentIndexChanged(int)), this, SLOT(setChanged()));
103 connect(m_idleTime, SIGNAL(valueChanged(int)), this, SLOT(setChanged())); 114​ connect(m_idleTime, SIGNAL(valueChanged(int)), this, SLOT(setChanged()));
104 115​
116​ if (PowerManagement::instance()->canSuspendThenHibernate()) {
117​ connect(m_suspendThenHibernateEnabled, SIGNAL(changed(bool)), this, SLOT(onChanged(bool)));
118​ retlist.append(qMakePair< QString, QWidget* >(QLatin1Literal("NONE"), m_suspendThenHibernateEnabled));
119​ } else {
120​ m_suspendThenHibernateEnabled->deleteLater();
abalajiUnsubmitted
Done

Not sure if this is necessary, just delete m_suspendThenHibernateEnabled might be enough.

abalaji: Not sure if this is necessary, just `delete m_suspendThenHibernateEnabled` might be enough.
avaldesAuthorUnsubmitted
Done

yeah it's necessary, without it powerdevil crashes at launch

avaldes: yeah it's necessary, without it powerdevil crashes at launch
abalajiUnsubmitted
Done

Yeah it crashes because you're probably not doing a null check somewhere. I would suggest initializing m_suspendThenHibernateEnabled to nullptr in the ctor, moving line 79 into this if block right above, and getting rid of this else block altogether, that way you don't wastefully allocate the object. After this, check for nullptr in every place you use m_suspendThenHibernateEnabled.

abalaji: Yeah it crashes because you're probably not doing a null check somewhere. I would suggest…
121​ m_suspendThenHibernateEnabled = nullptr;
122​ }
123​
124​ int comboBoxMaxWidth = 300;
125​ if (m_suspendThenHibernateEnabled) {
abalajiUnsubmitted
Done

After doing the above, this if block can be merged with the above if block, if you know what I mean.

abalaji: After doing the above, this if block can be merged with the above if block, if you know what I…
126​ comboBoxMaxWidth = qMax(comboBoxMaxWidth, m_suspendThenHibernateEnabled->sizeHint().width());
127​ m_suspendThenHibernateEnabled->setMinimumWidth(300);
128​ m_suspendThenHibernateEnabled->setMaximumWidth(comboBoxMaxWidth);
129​ }
130​
105 return retlist; 131​ return retlist;
106} 132​}
107 133​
108} 134​}
109} 135​}
110 136​
111#include "suspendsessionconfig.moc" 137​#include "suspendsessionconfig.moc"