D23287: Create a plugin framework for processes
This revision was automatically updated to reflect the committed changes. Closed by commit R111:064688415a1e: Create a plugin framework for processes (authored by davidedmundson). REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=65997=65998 REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma, ahiemstra, meven Cc: meven, zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
meven accepted this revision. meven added a comment. Build with QT 5.12 REPOSITORY R111 KSysguard Library BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 To: davidedmundson, #plasma, ahiemstra, meven Cc: meven, zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 65997. davidedmundson added a comment. update REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=65993=65997 BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma, ahiemstra Cc: meven, zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 65993. davidedmundson added a comment. Rebase, add include REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64404=65993 BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma, ahiemstra Cc: meven, zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
meven added inline comments. INLINE COMMENTS > process_data_provider.cpp:23 > +#include "process_attribute.h" > + > +using namespace KSysGuard; Missing `#include ` Results in build error : kde/src/libksysguard/processcore/process_data_provider.cpp:30:33: error: field ‘m_attributes’ has incomplete type ‘ │ QVector’ 60 │ QVector m_attributes; 61 │ ^~~~ REPOSITORY R111 KSysguard Library BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 To: davidedmundson, #plasma, ahiemstra Cc: meven, zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 64404. davidedmundson added a comment. q_decl_hidden ran clang-format over new files REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64291=64404 BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma, ahiemstra Cc: zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
zzag added inline comments.
INLINE COMMENTS
> extended_process_list.cpp:31
> +
> +class ExtendedProcesses::Private
> +{
Symbol of the PIMPL is leaked. Use Q_DECL_HIDDEN.
> process_data_provider.h:42-52
> +/**
> + * Accessors for process information matching
> + */
> +KSysGuard::Processes* processes() const;
> +
> +/**
> + * Returns a new process object for a given PID
Coding style inconsistency: pointer alignment.
Such inconsistency can be noticed in other parts of this patch too, e.g.
QVector and QVector, etc.
REPOSITORY
R111 KSysguard Library
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D23287
To: davidedmundson, #plasma, ahiemstra
Cc: zzag, ahiemstra, alexde, broulik, plasma-devel, LeGast00n,
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot,
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 64291. davidedmundson added a comment. Split ProcessAttribute into a new file Install headers correctly REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64189=64291 BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma, ahiemstra Cc: ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 64189. davidedmundson added a comment. update REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64155=64189 REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma, ahiemstra Cc: ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
ahiemstra requested changes to this revision.
ahiemstra added a comment.
This revision now requires changes to proceed.
Just a few small things I came across when working on the network plugin.
INLINE COMMENTS
> extended_process_list.cpp:82
> +for (auto plugin: pluginObjects) {
> +auto factory = qobject_cast(plugin);
> +if (!factory) {
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.
> formatter.h:48
> +/**
> + * Returns the scale factor suitable for KSignalPlotter.
> + *
I don't think KSignalPlotter is very relevant here. :) (Also in the next
comment block.)
> process_data_provider.h:66
> +QString shortName() const;
> +void setShortName(const QString );
> +
I'm missing the implementation for this method in process_data_provider.cpp
REPOSITORY
R111 KSysguard Library
REVISION DETAIL
https://phabricator.kde.org/D23287
To: davidedmundson, #plasma, ahiemstra
Cc: ahiemstra, alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev,
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 64155. davidedmundson added a comment. update REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64136=64155 REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma Cc: alexde, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 64136. davidedmundson added a comment. update REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64100=64136 BRANCH master REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h 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
D23287: Create a plugin framework for processes
davidedmundson marked 7 inline comments as done. davidedmundson added inline comments. INLINE COMMENTS > broulik wrote in extended_process_list.cpp:62 > Add `reserve` call I can't add a meaningful one. p->attributes() is itself another vector of size 0 to N. > broulik wrote in process_data_provider.h:54 > Why `const`? Simply why not? Doesn't really make a difference. > broulik wrote in ProcessModel.cpp:518 > Can this become out of sync, given you capture `i` as a copy into the lambda? 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. > broulik wrote in ProcessModel.cpp:1304 > `value.type() == QVariant::String`? 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. > broulik wrote in ProcessModel.cpp:1305 > Shouldn't those be or'd together? Interestingly, Qt documentation also uses > addition. For independent flags it's the same thing. Can change if you like, but it matches the docs. 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
D23287: Create a plugin framework for processes
broulik added inline comments.
INLINE COMMENTS
> extended_process_list.cpp:62
> +{
> +QVector 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 m_data;
> +bool m_enabled = 0;
> +};
`false`
> process_data_provider.h:21
> +
> +#include
> +#include
Unused
> process_data_provider.h:22
> +#include
> +#include
> +#include
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
> +#include
> +#include
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],
> ::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(value.type()) !=
> QMetaType::QString) {
> +return Qt::AlignRight + Qt::AlignVCenter;
`value.type() == QVariant::String`?
> ProcessModel.cpp:1305
> +&& static_cast(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
D23287: Create a plugin framework for processes
davidedmundson updated this revision to Diff 64100. davidedmundson added a comment. Update REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23287?vs=64097=64100 REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D23287: Create a plugin framework for processes
davidedmundson created this revision. davidedmundson added a reviewer: Plasma. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidedmundson requested review of this revision. REVISION 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. REPOSITORY R111 KSysguard Library REVISION DETAIL https://phabricator.kde.org/D23287 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/formatter.cpp processcore/formatter.h processcore/process_data_provider.cpp processcore/process_data_provider.h processcore/processes.cpp processcore/processes.h processcore/unit.cpp processcore/unit.h processui/ProcessModel.cpp processui/ProcessModel_p.h To: davidedmundson, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
