D27509: Introduce ProcessDataModel

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200228/01cddd0f/attachment-0001.html>