DevHeads.net

Review Request 121831: ksysguard: process.h: encapsulate private fields

Review request for KDE Base Apps and John Tapsell.

Repository: libksysguard

Description
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

Diffs
processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21
.reviewboardrc PRE-CREATION
processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Comments

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/12/2015 - 09:24

(Updated Jan. 12, 2015, 2:24 p.m.)

Review request for KDE Base Apps and John Tapsell.

Changes
In process.h the class Process is declared Q_DECL_EXPORT. Is libksysguard used somewhere else except in ksysguard? I ask because the public API would change. In ksysguard the change has no impact, as far as I can see.

Summary (updated)
libksysguard: process.h: encapsulate private fields

Repository: libksysguard

Description
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

Diffs
processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21
.reviewboardrc PRE-CREATION
processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/12/2015 - 09:30

(Updated Jan. 12, 2015, 2:30 p.m.)

Review request for KDE Base Apps and John Tapsell.

Changes
question about C++ macros

Repository: libksysguard

Description (updated)
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs
processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21
.reviewboardrc PRE-CREATION
processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/15/2015 - 12:43

processcore/process.h
<https://git.reviewboard.kde.org/r/121831/#comment51460>

Requires SOVERSION bump.

And: wouldn't it be better to move the now private variables behind a d_ptr?

- Gregor Mi

On Jan. 12, 2015, 2:30 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/18/2015 - 18:03

(Updated Jan. 18, 2015, 11:03 p.m.)

Review request for KDE Base Apps, Eike Hein and John Tapsell.

Changes
Updated to use d-ptr as suggested. I added you to this RR, Eike. Maybe you can take a look if the way I started to use the d-ptr is the correct one.

Repository: libksysguard

Description
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/19/2015 - 13:17

(Updated Jan. 19, 2015, 6:17 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Repository: libksysguard

Description
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Aleix Pol at 01/19/2015 - 13:24

processcore/process.h
<https://git.reviewboard.kde.org/r/121831/#comment51593>

Maybe you want to prefix the attribute variables with m_ then?

- Aleix Pol Gonzalez

On Jan. 19, 2015, 7:17 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Dominik Haumann at 01/21/2015 - 05:11

Please dont prefix with m_.
Is is quite common that member variables "m_something" become "d->something" when hidden behind a d-pointer. Having "d->m_" would communicate this twice.

- Dominik

On Jan. 19, 2015, 9:37 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/19/2015 - 14:52

Thanks for the hint. I will move this member and the ones below it also to the ProcessPrivate class. There the m_ can (should?) be omitted, can't it?

- Gregor

On Jan. 19, 2015, 6:17 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/19/2015 - 16:33

(Updated Jan. 19, 2015, 9:33 p.m.)

Review request for KDE Base Apps and John Tapsell.

Changes
Process: d-ptr for the next 11 fields (more to come)

Repository: libksysguard

Description
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes.cpp 6c0effc903b7792a078e68d2ac6f7ac29dbbcc31
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/19/2015 - 16:37

(Updated Jan. 19, 2015, 9:37 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Changes
readd lost people

Repository: libksysguard

Description
In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes.cpp 6c0effc903b7792a078e68d2ac6f7ac29dbbcc31
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/24/2015 - 19:46

(Updated Jan. 25, 2015, 12:46 a.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Changes
Process: d-ptr for 32 more fields
rebase on latest master

Repository: libksysguard

Description (updated)
In process.h there are several public fields. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown. No error.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 07:01

(Updated Jan. 25, 2015, 12:01 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Changes
Process: d-ptr for 6 more fields, add namespace to cpp file
Process: d-ptr for the last two fields
Ready to commit.

Repository: libksysguard

Description
In process.h there are several public fields. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
.reviewboardrc PRE-CREATION
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing (updated)
Compiles and runs. Data is still shown; no visible error. Unit tests succeed.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 17:08

(Updated Jan. 25, 2015, 10:08 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Changes
- process.h: consistent method naming, move getters before setters
- scripting: remove/rename macro

Repository: libksysguard

Description
In process.h there are several public fields. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown; no visible error. Unit tests succeed.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 17:20

(Updated Jan. 25, 2015, 10:20 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Changes
Process: codestyle: write getters and setters in pairs to better keep track

Repository: libksysguard

Description
In process.h there are several public fields. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown; no visible error. Unit tests succeed.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 17:27

(Updated Jan. 25, 2015, 10:27 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Changes
more consistent order of methods

Repository: libksysguard

Description
In process.h there are several public fields. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?

Diffs (updated)
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown; no visible error. Unit tests succeed.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 02/15/2015 - 11:35

(Updated Feb. 15, 2015, 4:35 p.m.)

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Repository: libksysguard

Description (updated)
This is a follow-up to <a href="https://git.reviewboard.kde.org/r/121717/:" title="https://git.reviewboard.kde.org/r/121717/:">https://git.reviewboard.kde.org/r/121717/:</a>

In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

Diffs
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown; no visible error. Unit tests succeed.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 02/20/2015 - 16:46

(Updated Feb. 20, 2015, 9:46 p.m.)

Status
This change has been marked as submitted.

Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.

Repository: libksysguard

Description
This is a follow-up to <a href="https://git.reviewboard.kde.org/r/121717/:" title="https://git.reviewboard.kde.org/r/121717/:">https://git.reviewboard.kde.org/r/121717/:</a>

In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr.

(In a separate commit I would add the .reviewboardrc file)

Diffs
processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9
processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0
processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889
processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8
.reviewboardrc PRE-CREATION
CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636
processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a
processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab

Diff: <a href="https://git.reviewboard.kde.org/r/121831/diff/" title="https://git.reviewboard.kde.org/r/121831/diff/">https://git.reviewboard.kde.org/r/121831/diff/</a>

Testing
Compiles and runs. Data is still shown; no visible error. Unit tests succeed.

Thanks,

Gregor Mi

Re: Review Request 121831: libksysguard: process.h: encapsulate

By hrvoje.senjan at 02/20/2015 - 21:46

can you check what needs to be adjusted in plasma-workspace? it fails to build with your change:

[ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:471:43: error: 'proc->KSysGuard::Process::command' does not have class type
[ 451s] QString cmdline = proc ? proc->command.simplified() : QString(); // proc->command has a trailing space???
[ 451s] ^
[ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: error: no matching function for call to 'KService::KService(<unresolved overloaded function type>, QString&, QString)'
[ 451s] services << QExplicitlySharedDataPointer<KService>(new KService(proc->name, cmdline, QString()));
[ 451s] ^
[ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: note: candidates are:
[ 451s] In file included from /usr/include/KF5/KService/KService:1:0,
[ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32:
[ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: KService::KService(QDataStream&, int)
[ 451s] KService(QDataStream &str, int offset);
[ 451s] ^
[ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: candidate expects 2 arguments, 3 provided
[ 451s] In file included from /usr/include/KF5/KService/KService:1:0,
[ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32:
[ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: KService::KService(const KDesktopFile*, const QString&)
[ 451s] explicit KService(const KDesktopFile *config, const QString &entryPath = QString());
[ 451s] ^
[ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: candidate expects 2 arguments, 3 provided
[ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: KService::KService(const QString&)
[ 451s] explicit KService(const QString &fullpath);
[ 451s] ^
[ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: candidate expects 1 argument, 3 provided
[ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: KService::KService(const QString&, const QString&, const QString&)
[ 451s] KService(const QString &name, const QString &exec, const QString &icon);
[ 451s] ^
[ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: no known conversion for argument 1 from '<unresolved overloaded function type>' to 'const QString&'
[ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp: In function 'QUrl TaskManager::getServiceLauncherUrl(int, const QString&, const QStringList&)':
[ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:516:43: error: 'proc->KSysGuard::Process::command' does not have class type
[ 451s] QString cmdline = proc ? proc->command.simplified() : QString(); // proc->command has a trailing space???
[ 451s] ^

- Hrvoje Senjan

On Feb. 20, 2015, 10:46 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Sebastian =?utf... at 02/21/2015 - 07:02

Aleix and me have fixed plasma-workspace's build. Update the master branch.

- Sebastian

On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Bhushan Shah at 02/21/2015 - 06:01

Yes its master branch

- Bhushan

On Feb. 21, 2015, 3:16 a.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 02/21/2015 - 05:56

How can I find out if which branch was compiled? I assume it is the master branch.

- Gregor

On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 02/21/2015 - 05:46

Probably `proc->command` must be replace with `proc->command()`. I'll check that.

- Gregor

On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By david at 02/17/2015 - 17:22

Ship it!

- David Edmundson

On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 02/20/2015 - 16:35

Thx for looking into it. Since it is quite a large change I'll keep my intermediate commits and push them now.

- Gregor

On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Alexander Richardson at 01/25/2015 - 12:28

Other than these two issues looks good to me, but someone else should approve

processcore/process.h
<https://git.reviewboard.kde.org/r/121831/#comment51786>

All other methods are camelCase, so I would call this method parentPid() instead.
Same with the setter

processui/scripting.h
<https://git.reviewboard.kde.org/r/121831/#comment51787>

Isn't just changing the PROPERTY macro enough?
Or is it used in some other file?

- Alex Richardson

On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 17:08

fixed now

- Gregor

On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 13:41

I did the refactoring in several steps so both methods were needed on the way. Now the macro can be renamed back as you noted.

- Gregor

On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Dominik Haumann at 01/21/2015 - 05:10

I think it's a good idea to hide the impl behind a d-pointer.

It's yet unclear to me, whether the additional setter/getter calls are really a performance issue. If required, we could save storing local variables by adding reference-getters, see comments below. But I'd only do this if we have input that this is really necessary.

Comments from the KSysGuard authors would be very much appreciated :-)

processcore/process.h
<https://git.reviewboard.kde.org/r/121831/#comment51643>

Is virtual needed here?

processcore/process.h
<https://git.reviewboard.kde.org/r/121831/#comment51644>

You can do it like this. Q_DECLARE_PRIVATE does something along these lines:

#define Q_DECLARE_PRIVATE(Class) \
inline Class##Private* d_func() { return reinterpret_cast<Class##Private *>(qGetPtrHelper(d_ptr)); } \
inline const Class##Private* d_func() const { \
return reinterpret_cast<const Class##Private *>(qGetPtrHelper(d_ptr)); } \
friend class Class##Private;

Furtheri, Q_D looks like this:
#define Q_D(Class) Class##Private * const d = d_func()

So you can either use:
Q_D(Process);
d->...
or
d_ptr->...
or
d_func()->...

Personally, I prefer declaring a d-pointer directly, since it does not require any macro an das such you can directly see what happens, without any Q_D or d_func() magic.

So in essence: the way you do it is correct, it's just a matter of taste. Right now, you always need the extra line "Q_D(const Process);" in the getters. You could save these by either writing d_ptr->, or by going the pure d-route. Would be a bit less code (which imho is good), but as said, matter of taste.

Please remove the comment :-)

processcore/process.cpp
<https://git.reviewboard.kde.org/r/121831/#comment51646>

Can you write:

KSysGuard::Process::Process()
: d_ptr(new ProcessPrivate())
{
clear();
}

Although the curly brace often is in the same line in this class, the kde coding style is more the above.

processcore/process.cpp
<https://git.reviewboard.kde.org/r/121831/#comment51647>

Same here.

processcore/process.cpp
<https://git.reviewboard.kde.org/r/121831/#comment51648>

Yes, needed.

processcore/process.cpp
<https://git.reviewboard.kde.org/r/121831/#comment51649>

This line is wrong:
(d->tracerpid == d->tracerpid) is always true.

processcore/processes_linux_p.cpp
<https://git.reviewboard.kde.org/r/121831/#comment51650>

In theorey, if we wanted to avoid the local varialbes here, we could add reference-getters, e.g.:

qlonglong & process->ruid();

These reference getters could be used as parameters to store the data directly in the desired varialbles, which would equal the current behavior.

Not sure it's worth it, would be cool to have input from true KSysGuard developers here.

processcore/processes_linux_p.cpp
<https://git.reviewboard.kde.org/r/121831/#comment51651>

Don't you change behavior here?

Before, we just wrote into the varialbes.

Now, we use the setters, which also sets 'changes |= Process::Gids;'

Is that maybe an issue? I myself don't know the code well enough to see this here.

- Dominik Haumann

On Jan. 19, 2015, 9:37 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Dominik Haumann at 01/26/2015 - 12:06

Oh, and I just recognized that this is about Process and not ProcessPrivate. My previous comment was about ProcessPrivate, so pretty much off-topic.

Ok, then I agree that it's ok to have a virtual destructor, since maybe someone wants to inherit from this class... sorry for the noise :)

- Dominik

On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By =?utf-8?Q?Thoma... at 01/26/2015 - 07:24

sorry - from the discussion i just assumed this was a leaf to qobject.
no. introducing a vtable to a private class makes no sense and indeed does harm (performance)
and q_decl_override is of course entirely wrong in a base class.

- Thomas

On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By =?utf-8?Q?Thoma... at 01/26/2015 - 12:52

Hehe =)

Ok, looked up the patch in a real browser.

For an exported public root class, a virtual destructor is a very good idea (unless you *really* want to enforce this being a leaf)
It then must have the virtual keyword (Q_DECL_OVERRIDE still makes no sense, that would be for the inheritors)

- Thomas

On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By =?utf-8?Q?Thoma... at 01/26/2015 - 06:35

No. But it's not required either.
It's a matter of preference to indicate the virtuality in a non-root class.
The better solution is Q_DECL_OVERRIDE

- Thomas

On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Dominik Haumann at 01/26/2015 - 06:55

How is Q_DECL_OVERRIDE related to having a virtual destructor in the base class?

Typically, you make the pimpl class FooPrivate virtual if there are other classes Bar that derive from Foo and also require the pimpl idion, i.e. class BarPrivate : public FooPrivate. This way you still have only one d-Pointer allocation independent of the deepness of the class hierarchy. In KDE, we seldom need that, and therefore we can put FooPrivate into the cpp file and make it have a non-virtual destructor.
However, in Qt itselv the d-pointer classes inherit other d-pointer classes, therefore you typically have a e.g. a qpushbutton_p.h, which itself probably includes qwidget_p.h etc...

I dont think we need a virtual destructor here. only adds a vtable that is not required ;)

- Dominik

On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/30/2015 - 14:02

I did some analysis. There is no visible change in behaviour ksysguard.

ProcessesLocal::Private::readProcStatus where the change in discussion was made (setter method with change detection instead of direct assignment).

is called by ProcessesLocal::updateProcessInfo in the same file:

```
bool ProcessesLocal::updateProcessInfo( long pid, Process *process)
{
bool success = true;
QString dir = "/proc/" + QString::number(pid) + '/';
if(!d->readProcStat(dir, process)) success = false;
if(!d->readProcStatus(dir, process)) success = false; <------------
...
```

Then there is

```
bool Processes::updateProcess( Process *ps, long ppid)
...
...

bool success = updateProcessInfo(ps); <--------
emit processChanged(ps, false); <-------

return success;
}
```

```
bool Processes::updateOrAddProcess( long pid)
...
...
Process *ps = d->mProcesses.value(pid);
if(!ps)
return addProcess(pid, ppid);
else
return updateProcess(ps, ppid); <---------
}
```

which is called e.g. every second.

The emitted processChanged signal is connected here:

```
connect( mProcesses, SIGNAL(processChanged(KSysGuard::Process*,bool)), this, SLOT(processChanged(KSysGuard::Process*,bool)));
```

void ProcessModelPrivate::processChanged(KSysGuard::Process *process, bool onlyTotalCpu):

Each change on a field that was recored in ProcessesLocal::Private::readProcStatus will emit dataChanged signal, e.g.:

```
if(process->changes() & KSysGuard::Process::Uids) {
totalUpdated++;
QModelIndex index = q->createIndex(row, ProcessModel::HeadingUser, process);
emit q->dataChanged(index, index);
}
```

The dataChanged signal is from QAbstractItemModel. So as far as I can see the worst thing could happen that the GUI is updated at more places than needed.

When I start ksysguard the processChanged method is called about 300 times (1 time for each process in the list) in the update interval (every second).

I tried to update everything at once and circument the dataChanged of individual items with
```
QModelIndex index1 = q->createIndex(row, 0, process);
QModelIndex index2 = q->createIndex(row, q->columnCount() - 1, process);
emit q->dataChanged(index1, index2);
return;
```
which had not visible effect.

So what now? Leave the setters and have maybe a performance penalty? Remove the setters again and use references getters? Change the Process API otherwise?

- Gregor

On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/26/2015 - 13:45

In the current RR state there are some reference-getters left because it made the porting easier. Should they be converted to non-reference getters, too?

- Gregor

On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/30/2015 - 12:40

I drop the issue. The potential removal of remaining reference-getters can be done in another commit.

- Gregor

On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 02/15/2015 - 11:35

any new input?

- Gregor

On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/25/2015 - 16:52

Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used.

True. Thanks for noticing. The changes will be read in ProcessModelPrivate::processChanged when a change is signaled by KSysGuard::Process::processChanged. Then GUI model updates are triggered. So probably the worst that can happen is one additional GUI update when processChanged is emitted.

- Gregor

On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote:

Re: Review Request 121831: libksysguard: process.h: encapsulate

By Gregor Mi at 01/15/2015 - 15:27

see also last comments in <a href="https://git.reviewboard.kde.org/r/122072/" title="https://git.reviewboard.kde.org/r/122072/">https://git.reviewboard.kde.org/r/122072/</a> about redesigning it with d_ptr to make process.h future-proof

- Gregor Mi

On Jan. 12, 2015, 2:30 p.m., Gregor Mi wrote: