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

Changeset View

Standalone View

View Options

daemon/actions/bundled/suspendsessionconfig.cpp

Show All 36 Lines
37K_PLUGIN_FACTORY(PowerDevilSuspendSessionConfigFactory, registerPlugin<PowerDevil::BundledActions::SuspendSessionConfig>(); ) 37​K_PLUGIN_FACTORY(PowerDevilSuspendSessionConfigFactory, registerPlugin<PowerDevil::BundledActions::SuspendSessionConfig>(); )
38 38​
39namespace PowerDevil { 39​namespace PowerDevil {
40namespace BundledActions { 40​namespace BundledActions {
41 41​
42SuspendSessionConfig::SuspendSessionConfig(QObject* parent, const QVariantList&) 42​SuspendSessionConfig::SuspendSessionConfig(QObject* parent, const QVariantList&)
43 : ActionConfig(parent) 43​ : ActionConfig(parent)
44{ 44​{
45 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…
46} 46​}
47 47​
48SuspendSessionConfig::~SuspendSessionConfig() 48​SuspendSessionConfig::~SuspendSessionConfig()
49{ 49​{
50 50​
51} 51​}
52 52​
53void SuspendSessionConfig::save() 53​void SuspendSessionConfig::save()
54{ 54​{
55 configGroup().writeEntry< uint >("suspendType", m_comboBox->itemData(m_comboBox->currentIndex()).toUInt()); 55​ configGroup().writeEntry< uint >("suspendType", m_comboBox->itemData(m_comboBox->currentIndex()).toUInt());
56 configGroup().writeEntry("idleTime", m_idleTime->value() * 60 * 1000); 56​ configGroup().writeEntry("idleTime", m_idleTime->value() * 60 * 1000);
57 if (m_suspendThenHibernateEnabled) { 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>( 58​ configGroup().writeEntry<bool>(
59 "suspendThenHibernate", m_suspendThenHibernateEnabled->isChecked()); 59​ "suspendThenHibernate", m_suspendThenHibernateEnabled->isChecked());
60 } else { 60​ } else {
61 configGroup().writeEntry<bool>("suspendThenHibernate", false); 61​ configGroup().writeEntry<bool>("suspendThenHibernate", false);
62 } 62​ }
63 63​
64 configGroup().sync(); 64​ configGroup().sync();
65} 65​}
66 66​
67void SuspendSessionConfig::load() 67​void SuspendSessionConfig::load()
68{ 68​{
69 configGroup().config()->reparseConfiguration(); 69​ configGroup().config()->reparseConfiguration();
70 70​
71 uint suspendType = configGroup().readEntry< uint >("suspendType", 0); 71​ uint suspendType = configGroup().readEntry< uint >("suspendType", 0);
72 m_comboBox->setCurrentIndex(m_comboBox->findData(suspendType)); 72​ m_comboBox->setCurrentIndex(m_comboBox->findData(suspendType));
73 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) { 74​ if (m_suspendThenHibernateEnabled) {
75 m_suspendThenHibernateEnabled->setChecked(configGroup().readEntry<bool>("suspendThenHibernate", false)); 75​ m_suspendThenHibernateEnabled->setChecked(configGroup().readEntry<bool>("suspendThenHibernate", false));
76 } 76​ }
77} 77​}
78 78​
79QList< QPair< QString, QWidget* > > SuspendSessionConfig::buildUi() 79​QList< QPair< QString, QWidget* > > SuspendSessionConfig::buildUi()
80{ 80​{
81 QWidget *tempWidget = new QWidget; 81​ QWidget *tempWidget = new QWidget;
82 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")); 83​ m_suspendThenHibernateEnabled = new QCheckBox(i18n("While asleep, hibernate after 3 hours"));
84 m_comboBox = new KComboBox; 84​ m_comboBox = new KComboBox;
85 m_idleTime = new QSpinBox; 85​ m_idleTime = new QSpinBox;
86 m_idleTime->setMaximumWidth(150); 86​ m_idleTime->setMaximumWidth(150);
87 m_idleTime->setMinimum(1); 87​ m_idleTime->setMinimum(1);
88 m_idleTime->setMaximum(360); 88​ m_idleTime->setMaximum(360);
89 m_idleTime->setValue(0); 89​ m_idleTime->setValue(0);
90 m_idleTime->setSuffix(i18n(" min")); 90​ m_idleTime->setSuffix(i18n(" min"));
91​ m_idleTime->setPrefix(i18n("after "));
91 92​
92 if (PowerManagement::instance()->canSuspend()) { 93​ if (PowerManagement::instance()->canSuspend()) {
93 m_comboBox->addItem(QIcon::fromTheme("system-suspend"), i18nc("Suspend to RAM", "Sleep"), (uint)SuspendSession::ToRamMode); 94​ m_comboBox->addItem(QIcon::fromTheme("system-suspend"), i18nc("Suspend to RAM", "Sleep"), (uint)SuspendSession::ToRamMode);
94 } 95​ }
95 if (PowerManagement::instance()->canHibernate()) { 96​ if (PowerManagement::instance()->canHibernate()) {
96 m_comboBox->addItem(QIcon::fromTheme("system-suspend-hibernate"), i18n("Hibernate"), (uint)SuspendSession::ToDiskMode); 97​ m_comboBox->addItem(QIcon::fromTheme("system-suspend-hibernate"), i18n("Hibernate"), (uint)SuspendSession::ToDiskMode);
97 } 98​ }
98 if (PowerManagement::instance()->canHybridSuspend()) { 99​ if (PowerManagement::instance()->canHybridSuspend()) {
99 m_comboBox->addItem(QIcon::fromTheme("system-suspend-hybrid"), i18n("Hybrid sleep"), (uint)SuspendSession::SuspendHybridMode); 100​ m_comboBox->addItem(QIcon::fromTheme("system-suspend-hybrid"), i18n("Hybrid sleep"), (uint)SuspendSession::SuspendHybridMode);
100 } 101​ }
101 m_comboBox->addItem(QIcon::fromTheme("system-shutdown"), i18n("Shut down"), (uint)SuspendSession::ShutdownMode); 102​ m_comboBox->addItem(QIcon::fromTheme("system-shutdown"), i18n("Shut down"), (uint)SuspendSession::ShutdownMode);
102 m_comboBox->addItem(QIcon::fromTheme("system-lock-screen"), i18n("Lock screen"), (uint)SuspendSession::LockScreenMode); 103​ m_comboBox->addItem(QIcon::fromTheme("system-lock-screen"), i18n("Lock screen"), (uint)SuspendSession::LockScreenMode);
103 104​
104 hlay->addWidget(m_idleTime); ​
105 hlay->addWidget(m_comboBox); 105​ hlay->addWidget(m_comboBox);
106​ hlay->addWidget(m_idleTime);
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
106 hlay->addStretch(); 107​ hlay->addStretch();
107 108​
108 tempWidget->setLayout(hlay); 109​ tempWidget->setLayout(hlay);
109 110​
110 QList< QPair< QString, QWidget* > > retlist; 111​ QList< QPair< QString, QWidget* > > retlist;
111 retlist.append(qMakePair< QString, QWidget* >(i18n("After"), tempWidget)); 112​ retlist.append(qMakePair< QString, QWidget* >(i18n("Automatically"), tempWidget));
112 113​
113 connect(m_comboBox, SIGNAL(currentIndexChanged(int)), this, SLOT(setChanged())); 114​ connect(m_comboBox, SIGNAL(currentIndexChanged(int)), this, SLOT(setChanged()));
114 connect(m_idleTime, SIGNAL(valueChanged(int)), this, SLOT(setChanged())); 115​ connect(m_idleTime, SIGNAL(valueChanged(int)), this, SLOT(setChanged()));
115 116​
116 if (PowerManagement::instance()->canSuspendThenHibernate()) { 117​ if (PowerManagement::instance()->canSuspendThenHibernate()) {
117 connect(m_suspendThenHibernateEnabled, SIGNAL(changed(bool)), this, SLOT(onChanged(bool))); 118​ connect(m_suspendThenHibernateEnabled, SIGNAL(changed(bool)), this, SLOT(onChanged(bool)));
118 retlist.append(qMakePair< QString, QWidget* >(QLatin1Literal("NONE"), m_suspendThenHibernateEnabled)); 119​ retlist.append(qMakePair< QString, QWidget* >(QLatin1Literal("NONE"), m_suspendThenHibernateEnabled));
119 } else { 120​ } else {
120 m_suspendThenHibernateEnabled->deleteLater(); 121​ 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​ m_suspendThenHibernateEnabled = nullptr;
122 } 123​ }
123 124​
124 int comboBoxMaxWidth = 300; 125​ int comboBoxMaxWidth = 300;
125 if (m_suspendThenHibernateEnabled) { 126​ 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​ comboBoxMaxWidth = qMax(comboBoxMaxWidth, m_suspendThenHibernateEnabled->sizeHint().width());
127 m_suspendThenHibernateEnabled->setMinimumWidth(300); 128​ m_suspendThenHibernateEnabled->setMinimumWidth(300);
128 m_suspendThenHibernateEnabled->setMaximumWidth(comboBoxMaxWidth); 129​ m_suspendThenHibernateEnabled->setMaximumWidth(comboBoxMaxWidth);
129 } 130​ }
130 131​
131 return retlist; 132​ return retlist;
132} 133​}
133 134​
134} 135​}
135} 136​}
136 137​
137#include "suspendsessionconfig.moc" 138​#include "suspendsessionconfig.moc"