Differential D20007 Diff 54638 src/lib/util/kprocesslist.h

Changeset View

Standalone View

View Options

src/lib/util/kprocesslist.h

  • This file was added.
1​/**************************************************************************
hallasAuthorUnsubmitted
Not Done

Is this license ok?

hallas: Is this license ok?
davidedmundsonUnsubmitted
Not Done

I think so, someone who's into licenses probably should check

davidedmundson: I think so, someone who's into licenses probably should check
2​**
3​** This file is part of the KDE Frameworks
4​**
5​** Copyright (c) 2011 Nokia Corporation and/or its subsidiary(-ies).
6​** Copyright (c) 2019 David Hallas <david@davidhallas.dk>
7​**
8​** GNU Lesser General Public License Usage
9​**
10​** This file may be used under the terms of the GNU Lesser General Public
11​** License version 2.1 as published by the Free Software Foundation and
12​** appearing in the file LICENSE.LGPL included in the packaging of this file.
13​** Please review the following information to ensure the GNU Lesser General
14​** Public License version 2.1 requirements will be met:
15​** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
16​**
17​** In addition, as a special exception, Nokia gives you certain additional
18​** rights. These rights are described in the Nokia Qt LGPL Exception
19​** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
20​**
21​** Other Usage
22​**
23​** Alternatively, this file may be used in accordance with the terms and
24​** conditions contained in a signed written agreement between you and Nokia.
25​**
26​** If you have questions regarding the use of this file, please contact
27​** Nokia at info@qt.nokia.com.
28​**
29​**************************************************************************/
30​
31​#ifndef KPROCESSLIST_H
32​#define KPROCESSLIST_H
33​
34​#include <QString>
35​#include <QList>
36​
hallasAuthorUnsubmitted
Not Done

I modified the interface slightly so that it fits better with other KDE APIs, I don't know what you guys think about it?

hallas: I modified the interface slightly so that it fits better with other KDE APIs, I don't know what…
davidedmundsonUnsubmitted
Done

Generally fine, I would put the method in a namespace

davidedmundson: Generally fine, I would put the method in a namespace
37​/**
38​ * @brief Contains information about a process
39​ * @since 5.57
40​ */
41​struct KProcessInfo {
davidedmundsonUnsubmitted
Done

This isn't future proof.

We probably need something with Pimpl (maybe a implicitly shared class)

davidedmundson: This isn't future proof. We probably need something with Pimpl (maybe a implicitly shared…
42​ unsigned int pid; ///< The pid of the process
hallasAuthorUnsubmitted
Not Done

I changed the pid to be a unsigned int instead, since both Windows and Unix use a number as the pid

hallas: I changed the pid to be a unsigned int instead, since both Windows and Unix use a number as the…
davidedmundsonUnsubmitted
Done

Needs to be qint64 to cover windows

davidedmundson: Needs to be qint64 to cover windows
43​ QString name; ///< The name of the process - this is not the full path to the executable
hallasAuthorUnsubmitted
Not Done

What is the exact semantics of this field?

hallas: What is the exact semantics of this field?
davidedmundsonUnsubmitted
Done

Reading the code back this is:

the full command line if available (full /proc/$pid/cmdline)
failing that the executable name (/proc/$pid/stat)
failing that the executable name (from ps)

(I have no idea about windows)

davidedmundson: Reading the code back this is: the full command line if available (full /proc/$pid/cmdline)…
44​ QString image; ///< What is this?
hallasAuthorUnsubmitted
Not Done

This appears to be a Windows only field, should we really keep this?

hallas: This appears to be a Windows only field, should we really keep this?
45​ QString state; ///< Running state - should we have this?
hallasAuthorUnsubmitted
Not Done

Do we really need this? Should we document all the valid states?

hallas: Do we really need this? Should we document all the valid states?
davidedmundsonUnsubmitted
Done

Definitely needs some docs. An enum might be better when we figure out what we want.

(or maybe just kill it?)

davidedmundson: Definitely needs some docs. An enum might be better when we figure out what we want. (or maybe…
46​ QString user; ///< Owner of the process
47​};
48​
49​using KProcessList = QList<KProcessInfo>;
50​
51​/**
52​ * @brief Retrieves the list of currently active processes
davidedmundsonUnsubmitted
Done

qint64 pid, const QString &name, const QString &user

it saves a string shallow copy

davidedmundson: qint64 pid, const QString &name, const QString &user it saves a string shallow copy
53​ * @return Returns the list of currently active processes
54​ * @since 5.57
55​ */
56​KProcessList GetProcessList();
hallasAuthorUnsubmitted
Not Done

Is this the interface we want?

hallas: Is this the interface we want?
davidedmundsonUnsubmitted
Done

Based on the use cases I've seen I think we want:

KProcessList GetProcessList(); <-- for the kdevelop etc case

KProcess GetProcess(qint64 pid); <-- for the device manager / plasma vaults case

davidedmundson: Based on the use cases I've seen I think we want: KProcessList GetProcessList(); <-- for the…
hallasAuthorUnsubmitted
Not Done

I haven't written any tests for the code as I am not sure how I can do that in a portable and reproduceable way.

hallas: I haven't written any tests for the code as I am not sure how I can do that in a portable and…
davidedmundsonUnsubmitted
Done

As for unit tests, maybe something like:

  • spawn an app with qprocess
  • find it in the list

We'll know the PID from qprocess, we know our own user id, we know the name

davidedmundson: As for unit tests, maybe something like: - spawn an app with qprocess - find it in the list…
elvisangelaccioUnsubmitted
Done

This method should follow the Qt naming style, i.e. processList().

elvisangelaccio: This method should follow the Qt naming style, i.e. `processList()`.
57​
58​#endif // KPROCESSLIST_H