D23287: Create a plugin framework for processes

[Kai Uwe Broulik]

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