Create a plugin framework for processes
ClosedPublic

Authored by davidedmundson on Aug 20 2019, 10:47 AM.

Details

Summary

Currently everything for processes is hardcoded with a method for each
process property. This is core functionality like CPU usage and memory
usage but it's not very extensible.

Currently ProcessModel is full of extra hacks to add X11 data when
really it should be a dumb proxier of information.

We have a pending patch to show network stats, and we have a pending
patch to add powertop information, which all work in a different way
from just reading data in /proc

In order to keep it flexible a more generic format method is added which
doesn't require hardcoding knowledge of types.

This patch is part of a series, next steps are adding various plugins,
stripping proces model - and then using the ProcessAttribute class to
provide the metadata for the core process attributes so that
ProcessModel can become a very simple view with no code duplication.

Diff Detail

Repository
R111 KSysguard Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Plasma. · View Herald TranscriptAug 20 2019, 10:47 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Aug 20 2019, 10:47 AM
broulik added inline comments.
processcore/extended_process_list.cpp
63

Add reserve call

77

Shouldn't this be a continue?

processcore/extended_process_list.h
39

Shouldn't that be in the Private?

processcore/formatter.h
38

This would look nicer with enum class but I don't really mind

processcore/process_data_provider.cpp
39

false

processcore/process_data_provider.h
22

Unused

23

Unused

32

Not used before its declaration below?

55

Why const?

92

Docs

134

Coding style: asterisk after space

processcore/unit.h
25

Unused

processui/ProcessModel.cpp
519

Store mExtraAttributes in a variable

522

Can this become out of sync, given you capture i as a copy into the lambda?

998

So when you now request a column >= columnCount() this cndition won't be met and you potentially access out of bounds below somewhere

1329

value.type() == QVariant::String?

1330

Shouldn't those be or'd together? Interestingly, Qt documentation also uses addition.

davidedmundson marked 7 inline comments as done.Aug 20 2019, 4:53 PM
davidedmundson added inline comments.
processcore/extended_process_list.cpp
63

I can't add a meaningful one.

p->attributes() is itself another vector of size 0 to N.

processcore/process_data_provider.h
55

Simply why not?
Doesn't really make a difference.

processui/ProcessModel.cpp
522

If mExtraAttributes changed during the lifespan, it would, but that currently doesn't happen.

We show all columns and then enable or disable them to populate updates.

1329

from qt docs:

Returns the storage type of the value stored in the variant. Although this function is declared as returning QVariant::Type, the return value should be interpreted as QMetaType::Type. In particular, QVariant::UserType is returned here only if the value is equal or greater than QMetaType::User.

1330

For independent flags it's the same thing.

Can change if you like, but it matches the docs.

alexde added a subscriber: alexde.Aug 20 2019, 5:42 PM
ahiemstra requested changes to this revision.Wed, Aug 21, 10:05 AM
ahiemstra added a subscriber: ahiemstra.

Just a few small things I came across when working on the network plugin.

processcore/extended_process_list.cpp
83

It would probably be nice to have some categorized logging here that reports the loaded plugins. It would make it easier to figure out if/when your plugin does not load.

processcore/formatter.h
49

I don't think KSignalPlotter is very relevant here. :) (Also in the next comment block.)

processcore/process_data_provider.h
67

I'm missing the implementation for this method in process_data_provider.cpp

This revision now requires changes to proceed.Wed, Aug 21, 10:05 AM
davidedmundson marked 4 inline comments as done.Wed, Aug 21, 10:30 AM

Split ProcessAttribute into a new file
Install headers correctly

ahiemstra accepted this revision.Fri, Aug 23, 8:57 AM
This revision is now accepted and ready to land.Fri, Aug 23, 8:57 AM
zzag added a subscriber: zzag.Fri, Aug 23, 9:22 AM
zzag added inline comments.
processcore/extended_process_list.cpp
32

Symbol of the PIMPL is leaked. Use Q_DECL_HIDDEN.

processcore/process_data_provider.h
43–53

Coding style inconsistency: pointer alignment.

Such inconsistency can be noticed in other parts of this patch too, e.g. QVector<KSysGuard::ProcessAttribute*> and QVector<KSysGuard::ProcessAttribute *>, etc.

q_decl_hidden
ran clang-format over new files

meven added a subscriber: meven.Sun, Sep 1, 6:31 AM
meven added inline comments.
processcore/process_data_provider.cpp
24

Missing #include <QVector>

Results in build error :

kde/src/libksysguard/processcore/process_data_provider.cpp:30:33: error: field ‘m_attributes’ has incomplete type ‘
       │ QVector<KSysGuard::ProcessAttribute*>’
  60   │      QVector<ProcessAttribute *> m_attributes;
  61   │                                  ^~~~~~~~~~~~

Rebase, add include

meven accepted this revision.Fri, Sep 13, 2:42 PM

Build with QT 5.12

This revision was automatically updated to reflect the committed changes.