Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- 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 QExplicitlySharedDataPointerKService(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: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 10:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 2:46 a.m., Hrvoje Senjan wrote: 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 QExplicitlySharedDataPointerKService(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]^ Probably `proc-command` must be replace with `proc-command()`. I'll check that. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 2:46 a.m., Hrvoje Senjan wrote: 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 QExplicitlySharedDataPointerKService(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]^ Gregor Mi wrote: Probably `proc-command` must be replace with `proc-command()`. I'll check that. How can I find out if which branch was compiled? I assume it is the master branch. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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 https://git.reviewboard.kde.org/r/121717/: 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Feb. 17, 2015, 10:22 p.m., David Edmundson wrote: Thx for looking into it. Since it is quite a large change I'll keep my intermediate commits and push them now. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76201 --- On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 15, 2015, 4:35 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Feb. 21, 2015, 8:16 a.m., Hrvoje Senjan wrote: 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 QExplicitlySharedDataPointerKService(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]^ Gregor Mi wrote: Probably `proc-command` must be replace with `proc-command()`. I'll check that. Gregor Mi wrote: How can I find out if which branch was compiled? I assume it is the master branch. How can I find out if which branch was compiled? I assume it is the master branch. Yes its master branch - Bhushan --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 21, 2015, 3:16 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 21, 2015, 3:16 a.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 2:46 a.m., Hrvoje Senjan wrote: 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 QExplicitlySharedDataPointerKService(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]^ Gregor Mi wrote: Probably `proc-command` must be replace with `proc-command()`. I'll check that. Gregor Mi wrote: How can I find out if which branch was compiled? I assume it is the master branch. Bhushan Shah wrote: How can I find out if which branch was compiled? I assume it is the master branch. Yes its master branch Aleix and me have fixed plasma-workspace's build. Update the master branch. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76201 --- Ship it! - David Edmundson On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 15, 2015, 4:35 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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 https://git.reviewboard.kde.org/r/121717/: 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote:
processcore/processes_linux_p.cpp, line 171
https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line171
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.
Gregor Mi wrote:
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 Mi wrote:
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?
any new input?
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
---
On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/
---
(Updated Feb. 15, 2015, 4:35 p.m.)
Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John
Tapsell.
Repository: libksysguard
Description
---
This is a follow-up to https://git.reviewboard.kde.org/r/121717/:
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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote: processcore/processes_linux_p.cpp, line 159 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line159 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. Gregor Mi wrote: 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? I drop the issue. The potential removal of remaining reference-getters can be done in another commit. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 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. 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 - 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote:
processcore/processes_linux_p.cpp, line 171
https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line171
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.
Gregor Mi wrote:
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.
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
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
---
On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/
---
(Updated Jan. 25, 2015, 10:27 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. 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
-
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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 21, 2015, 10:10 vorm., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Gregor Mi wrote: Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. Thomas Lübking wrote: 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 Dominik Haumann wrote: 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 ;) Thomas Lübking wrote: 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. Dominik Haumann wrote: 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 :) 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 nachm.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. 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 - 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 vorm., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Gregor Mi wrote: Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 nachm.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. 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 - 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 vorm., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Gregor Mi wrote: Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. Thomas Lübking wrote: 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 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 nachm.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. 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 - 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 vorm., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Gregor Mi wrote: Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. Thomas Lübking wrote: 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 Dominik Haumann wrote: 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 ;) 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 nachm.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. 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 - 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 vorm., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Gregor Mi wrote: Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. Thomas Lübking wrote: 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 Dominik Haumann wrote: 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 ;) Thomas Lübking wrote: 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. 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 nachm.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. 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 - 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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote: processcore/processes_linux_p.cpp, line 171 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line171 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. 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:01 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. 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 - .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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 25, 2015, 5:28 p.m., Alex Richardson wrote: processui/scripting.h, line 74 https://git.reviewboard.kde.org/r/121831/diff/5/?file=344700#file344700line74 Isn't just changing the PROPERTY macro enough? Or is it used in some other file? Gregor Mi wrote: 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. fixed now - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74713 --- On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:01 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. 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 - .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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 25, 2015, 5:28 p.m., Alex Richardson wrote: processui/scripting.h, line 74 https://git.reviewboard.kde.org/r/121831/diff/5/?file=344700#file344700line74 Isn't just changing the PROPERTY macro enough? Or is it used in some other file? 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74713 --- On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:01 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. 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 - .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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ 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 private fields
On Jan. 19, 2015, 6:24 nachm., Aleix Pol Gonzalez wrote: processcore/process.h, line 103 https://git.reviewboard.kde.org/r/121831/diff/2/?file=342813#file342813line103 Maybe you want to prefix the attribute variables with m_ then? Gregor Mi wrote: 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? 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74353 --- On Jan. 19, 2015, 9:37 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 19, 2015, 9:37 nachm.) 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.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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
---
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_castClass##Private *(qGetPtrHelper(d_ptr)); } \
inline const Class##Private* d_func() const { \
return reinterpret_castconst 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:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/
---
(Updated Jan. 19, 2015, 9:37 nachm.)
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.cpp 6c0effc903b7792a078e68d2ac6f7ac29dbbcc31
processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601
processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5
processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
processui/ProcessModel.cpp
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 19, 2015, 6:24 p.m., Aleix Pol Gonzalez wrote: processcore/process.h, line 103 https://git.reviewboard.kde.org/r/121831/diff/2/?file=342813#file342813line103 Maybe you want to prefix the attribute variables with m_ then? 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 --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74353 --- On Jan. 19, 2015, 6:17 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74353 --- 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: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 19, 2015, 7: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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74096 --- 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: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 12, 2015, 2:30 p.m.) 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) 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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74100 --- see also last comments in https://git.reviewboard.kde.org/r/122072/ 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: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 12, 2015, 2:30 p.m.) 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) 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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (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: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
