broulik added inline comments.
INLINE COMMENTS
> extended_process_list.cpp:62
> +{
> + QVector<ProcessAttribute *> rc;
> + for (auto p: qAsConst(d->m_providers)) {
Add `reserve` call
> extended_process_list.cpp:76
> + if (!factory) {
> + return;
> + }
Shouldn't this be a `continue`?
> extended_process_list.h:38
> +private:
> + void loadPlugins();
> + class Private;
Shouldn't that be in the `Private`?
> formatter.h:37
> + */
> +enum FormatOption {
> + FormatOptionNone = 0,
This would look nicer with `enum class` but I don't really mind
> process_data_provider.cpp:38
> + QHash<KSysGuard::Process *, QVariant> m_data;
> + bool m_enabled = 0;
> +};
`false`
> process_data_provider.h:21
> +
> +#include <QDateTime>
> +#include <QDebug>
Unused
> process_data_provider.h:22
> +#include <QDateTime>
> +#include <QDebug>
> +#include <QObject>
Unused
> process_data_provider.h:31
> +
> +class ProcessDataProvider;
> +
Not used before its declaration below?
> process_data_provider.h:54
> + bool enabled() const;
> + void setEnabled(const bool enable);
> +
Why `const`?
> process_data_provider.h:91
> +
> + KSysGuard::Unit unit() const;
> + void setUnit(KSysGuard::Unit unit);
Docs
> process_data_provider.h:133
> + */
> + KSysGuard::Processes* processes() const;
> +
Coding style: asterisk after space
> unit.h:24
> +#include <QMetaType>
> +#include <QString>
> +#include <QVariant>
Unused
> ProcessModel.cpp:515
> + for (int i = 0 ; i < mExtraAttributes.count(); i ++) {
> + mExtraAttributes[i]->setEnabled(true); // In future we will toggle
> this based on column visibility
> +
Store `mExtraAttributes` in a variable
> ProcessModel.cpp:518
> + connect(mExtraAttributes[i],
> &KSysGuard::ProcessAttribute::dataChanged, this, [this, i](KSysGuard::Process
> *process) {
> + const QModelIndex index = q->getQModelIndex(process->parent(),
> mHeadings.count() + i);
> + emit q->dataChanged(index, index);
Can this become out of sync, given you capture `i` as a copy into the lambda?
> ProcessModel.cpp:983
> +
> + if (section >= d->mHeadings.count() && section < columnCount()) {
> + int attr = section - d->mHeadings.count();
So when you now request a column >= `columnCount()` this cndition won't be met
and you potentially access out of bounds below somewhere
> ProcessModel.cpp:1304
> + if (value.canConvert(QMetaType::LongLong)
> + && static_cast<QMetaType::Type>(value.type()) !=
> QMetaType::QString) {
> + return Qt::AlignRight + Qt::AlignVCenter;
`value.type() == QVariant::String`?
> ProcessModel.cpp:1305
> + && static_cast<QMetaType::Type>(value.type()) !=
> QMetaType::QString) {
> + return Qt::AlignRight + Qt::AlignVCenter;
> + }
Shouldn't those be or'd together? Interestingly, Qt documentation also uses
addition.
REPOSITORY
R111 KSysguard Library
REVISION DETAIL
https://phabricator.kde.org/D23287
To: davidedmundson, #plasma
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
