broulik added inline comments.
INLINE COMMENTS
> extended_process_list.cpp:62
> + if (m_changeFlag != 0) {
> + connect(parent, &ExtendedProcesses::processChanged, this,
> [=](KSysGuard::Process *process) {
> + if (!process->changes().testFlag(m_changeFlag)) {
Capture only `[this]`
> extended_process_list.cpp:93
>
> + auto pidSensor = new ProcessSensor<qlonglong>(this,
> QStringLiteral("pid"), i18n("PID"), &KSysGuard::Process::pid,
> KSysGuard::Process::Status, ForwardFirstEntry);
> + pidSensor->setDescription(i18n("The unique Process ID that identifies
> this process."));
Isn't the `typename` auto-deduced/implied? Doesn't hurt though.
> extended_process_list.cpp:109
> + this, QStringLiteral("username"), i18n("Username"),
> [](KSysGuard::Process *p) {
> + KUser user(p->uid());
> + QString username;
Does this need caching?
> extended_process_list.cpp:122
> + K_UID uid = p->uid();
> + if (uid == 65534) {
> + return false;
Add a comment that this magic number is `nobody`
> extended_process_list.cpp:354
> + ioCharactersActuallyReadRateSensor->setUnit(KSysGuard::UnitByteRate);
> + ioCharactersActuallyReadRateSensor->setShortName(i18n("Read"));
> + ioCharactersActuallyReadRateSensor->setDescription(i18n("The rate of
> data being read from disk."));
Do any of the sensor names need / used to have i18n context?
> extended_process_list.cpp:394
> {
> QVector<ProcessAttribute *> rc;
> for (auto p : qAsConst(d->m_providers)) {
This could have used a `reserve` call :)
> process_data_model.cpp:1
> +#include "process_data_model.h"
> +#include "formatter.h"
Missing copyright header
> process_data_model.cpp:56
> +{
> + connect(m_processes, &KSysGuard::Processes::beginAddProcess, this,
> &ProcessDataModelPrivate::beginInsertRow);
> + connect(m_processes, &KSysGuard::Processes::endAddProcess, this,
> &ProcessDataModelPrivate::endInsertRow);
Does `ProcessDataModelPrivate` really need to be a `QObject`? Can't you connect
it to `q` with a lambda?
> process_data_model.cpp:63
> + for (auto attr : attributes) {
> + m_availableAttributes[attr->id()] = attr;
> + }
Add `reserve()`
> process_data_model.cpp:145
> +void ProcessDataModel::setEnabledAttributes(const QStringList
> &enabledAttributes)
> +{
> + beginResetModel();
if (d->enabledAttributes == enabledAttributes) {
return;
}
?
> process_data_model.cpp:183
> +{
> + if (row < 0)
> + return QModelIndex();
Coding style
> process_data_model.cpp:190
> + return QModelIndex();
> + if (d->m_processes->processCount() <= row)
> + return QModelIndex();
Remove equals, or better turn it around to match everone else around it:
if (row >= d->m_processes->processCount()) {
return QModelIndex();
}
> process_data_model.cpp:254
> +
> + QMetaEnum e =
> metaObject()->enumerator(metaObject()->indexOfEnumerator("AdditionalRoles"));
> +
There is:
QMetaEnum::fromType<AdditionalRoles>();
> process_data_model.cpp:278
> + case ShortName: {
> + if (!attribute->shortName().isEmpty()) {
> + return attribute->shortName();
Superfluous; or have it return `name()` instead
> process_data_model.h:33
> +/**
> + * This class contains contains a model of all running processes
> + * Rows represent processes
Remove second "contains"
> process_data_model.h:48
> + Q_PROPERTY(QStringList enabledAttributes READ enabledAttributes WRITE
> setEnabledAttributes NOTIFY enabledAttributesChanged)
> + Q_PROPERTY(QObject *attributesModel READ attributesModel CONSTANT)
> +
would it hurt to make this a `QAbstractItemModel *`?
> process_data_model.h:53
> + Value = Qt::UserRole, /// The raw value of the attribute. This is
> unformatted and could represent an int, real or string
> + FormattedValue, /// A string containing the value in a locale
> friendly way with appropriate suffix "eg. 5Mb" "20%"
> +
should this just be `Qt::DisplayRole` given you return for both in `data`?
> process_data_model.h:66
> +
> + ProcessDataModel(QObject *parent = nullptr);
> + ~ProcessDataModel() override;
`explicit`
> process_data_model.h:104
> +private:
> + QScopedPointer<ProcessDataModelPrivate> d;
> + friend class ProcessDataModelPrivate;
Why is this not a nested `Private` class like in the extended processes list?
REPOSITORY
R111 KSysguard Library
REVISION DETAIL
https://phabricator.kde.org/D27509
To: davidedmundson, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack,
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
