D27509: Introduce ProcessDataModel
This revision was automatically updated to reflect the committed changes. Closed by commit R111:834de9a585c6: Introduce ProcessDataModel (authored by davidedmundson). REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27509?vs=78091=78346 REVISION DETAIL https://phabricator.kde.org/D27509 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_attribute_model.cpp processcore/process_attribute_model.h processcore/process_data_model.cpp processcore/process_data_model.h processui/ProcessModel.cpp To: davidedmundson, #plasma, broulik Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
davidedmundson updated this revision to Diff 78091. davidedmundson added a comment. cache kuser REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27509?vs=78089=78091 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27509 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_attribute_model.cpp processcore/process_attribute_model.h processcore/process_data_model.cpp processcore/process_data_model.h processui/ProcessModel.cpp To: davidedmundson, #plasma, broulik Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
davidedmundson updated this revision to Diff 78089. davidedmundson added a comment. cache kuser REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27509?vs=78025=78089 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27509 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_attribute_model.cpp processcore/process_attribute_model.h processcore/process_data_model.cpp processcore/process_data_model.h processui/ProcessModel.cpp To: davidedmundson, #plasma, broulik Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
davidedmundson updated this revision to Diff 78025. davidedmundson marked 3 inline comments as done. davidedmundson added a comment. lambdas everywhere REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27509?vs=76653=78025 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27509 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_attribute_model.cpp processcore/process_attribute_model.h processcore/process_data_model.cpp processcore/process_data_model.h processui/ProcessModel.cpp To: davidedmundson, #plasma Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
davidedmundson added inline comments. INLINE COMMENTS > broulik wrote in extended_process_list.cpp:394 > This could have used a `reserve` call :) not a meaningful one, each provider can add provide many attributes REPOSITORY R111 KSysguard Library REVISION DETAIL https://phabricator.kde.org/D27509 To: davidedmundson, #plasma Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
davidedmundson updated this revision to Diff 76653. davidedmundson marked 15 inline comments as done. davidedmundson added a comment. some but not all review comments REPOSITORY R111 KSysguard Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27509?vs=76011=76653 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27509 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_attribute_model.cpp processcore/process_attribute_model.h processcore/process_data_model.cpp processcore/process_data_model.h processui/ProcessModel.cpp To: davidedmundson, #plasma Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
ahiemstra added inline comments. INLINE COMMENTS > broulik wrote in process_data_model.cpp:278 > Superfluous; or have it return `name()` instead Oops. Both this one and the one in data() should return name() if shortName() is empty. REPOSITORY R111 KSysguard Library REVISION DETAIL https://phabricator.kde.org/D27509 To: davidedmundson, #plasma Cc: ahiemstra, 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, mart
D27509: Introduce ProcessDataModel
davidedmundson added inline comments. INLINE COMMENTS > broulik wrote in process_data_model.h:53 > should this just be `Qt::DisplayRole` given you return for both in `data`? It breaks our roleNames as you can't have two names for the same value. Then on the QML side we can't interchange with display and formattedValue. which is what we were trying to achieve 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
D27509: Introduce ProcessDataModel
broulik added inline comments.
INLINE COMMENTS
> extended_process_list.cpp:62
> +if (m_changeFlag != 0) {
> +connect(parent, ::processChanged, this,
> [=](KSysGuard::Process *process) {
> +if (!process->changes().testFlag(m_changeFlag)) {
Capture only `[this]`
> extended_process_list.cpp:93
>
> +auto pidSensor = new ProcessSensor(this,
> QStringLiteral("pid"), i18n("PID"), ::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 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, ::Processes::beginAddProcess, this,
> ::beginInsertRow);
> +connect(m_processes, ::Processes::endAddProcess, this,
> ::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
> )
> +{
> +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();
> 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 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
D27509: Introduce ProcessDataModel
davidedmundson created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidedmundson requested review of this revision. REVISION SUMMARY Last release a plugin system was introduced that allowed plugins to provide columns of process data in a way that included enough metadata to allow displaying of said data appropriately without the client needing to be aware of the semantics of what that column represents. This patch provides all process information in that new format. This is then exposed as new, much simler, model. This new model is designed to be consumable from a QML API for any potential process data viewer. Existing models are unchanged for maximum compatibility. TEST PLAN Used in another project REPOSITORY R111 KSysguard Library BRANCH master REVISION DETAIL https://phabricator.kde.org/D27509 AFFECTED FILES CMakeLists.txt processcore/CMakeLists.txt processcore/extended_process_list.cpp processcore/extended_process_list.h processcore/process_attribute.cpp processcore/process_attribute.h processcore/process_attribute_model.cpp processcore/process_attribute_model.h processcore/process_data_model.cpp processcore/process_data_model.h processui/ProcessModel.cpp To: davidedmundson Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
