D12827: Added process details in the process list context menu

[Méven Car]

meven added a comment.


  In D12827#528767 , @chrisx wrote:
  
  > @meven Sorry I've totally forgotten about your suggestions when I was 
updating the patch...
  >  I have renamed `DetailsDlg` to `ProcessDetailsDialog`. But I've got some 
minor issues with code indentation. I have tried out `uncrustify-kf5` (which 
didn't even work out-of-box. Instead of formatting the files in-place, it 
produced `*.uncrustify` files instead. I have uncrustify 0.69, and I worked 
around the issue by adding `--replace` to the arguments for uncrystify in 
`uncrustify-kf5`). But the script produces way too many changes in some files I 
changed. One of those files is `processes_linux_p.cpp`, which uses both spaces 
and tabs for indentation. So at this moment I've only committed some of the 
indentation changes.
  >  Any further help with pushing this patch forward would be greatly 
appreciated.
  
  
  Sorry I should have mentioned and better explained.
  The uncrustify-kf5 is located kde-dev-scripts and depends on uncrustify.
  To use it only for a few files a workaround is to run it git add only the 
files you mean to commit and then git checkout the files you don't want to 
chaneg.
  If too many lines are concerned better skip this here then.
  
  Please note I am no ksysguard maintainer, and the Ksysguard app is going 
through a UI renew.

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx, broulik, davidedmundson, mart
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx updated this revision to Diff 65769.
chrisx set the repository for this revision to R111 KSysguard Library.
chrisx added a comment.


  @meven Sorry I've totally forgotten about your suggestions when I was 
updating the patch...
  I have renamed `DetailsDlg` to `ProcessDetailsDialog`. But I've got some 
minor issues with code indentation. I have tried out `uncrustify-kf5` (which 
didn't even work out-of-box. Instead of formatting the files in-place, it 
produced `*.uncrustify` files instead. I have uncrustify 0.69, and I worked 
around the issue by adding `--replace` to the arguments for uncrystify in 
`uncrustify-kf5`). But the script produces way too many changes in some files I 
changed. One of those files is `processes_linux_p.cpp`, which uses both spaces 
and tabs for indentation. So at this moment I've only committed some of the 
indentation changes.
  Any further help with pushing this patch forward would be greatly appreciated.

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12827?vs=65628=65769

REVISION DETAIL
  https://phabricator.kde.org/D12827

AFFECTED FILES
  lsofui/lsof.h
  processcore/process.cpp
  processcore/process.h
  processcore/processes_linux_p.cpp
  processui/CMakeLists.txt
  processui/ProcessDetailsDialog.cpp
  processui/ProcessDetailsDialog.h
  processui/ProcessDetailsDialogUI.ui
  processui/ksysguardprocesslist.cpp
  processui/ksysguardprocesslist.h

To: chrisx, broulik, davidedmundson, mart
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Méven Car]

meven added a comment.


  In D12827#527480 , @chrisx wrote:
  
  > Patch updated. Any suggestions?
  
  
  You have a few code indentation issues, that you can take care of.
  https://phabricator.kde.org/D12827#inline-130952
  
  And I think you should rename `DetailsDlg` to `DetailsDialog`, we prefer to 
avoid abbreviations in classes names as a convention.
  
  Also the patch does not apply on master but I guess it depends on D23287 
 which also needs to be rebased on master...

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx, broulik, davidedmundson, mart
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx added a comment.


  Patch updated. Any suggestions?

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx, broulik, davidedmundson, mart
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx updated this revision to Diff 65628.
chrisx added a reviewer: mart.
chrisx added a comment.


  Rebased to master. Removed uses of deprecated class (QRegExp). Tidied the 
code up a bit.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12827?vs=35891=65628

REVISION DETAIL
  https://phabricator.kde.org/D12827

AFFECTED FILES
  lsofui/lsof.h
  processcore/process.cpp
  processcore/process.h
  processcore/processes_linux_p.cpp
  processui/CMakeLists.txt
  processui/DetailsDlg.cpp
  processui/DetailsDlg.h
  processui/DetailsDlgUI.ui
  processui/ksysguardprocesslist.cpp
  processui/ksysguardprocesslist.h

To: chrisx, broulik, davidedmundson, mart
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx added a comment.


  In D12827#513118 , @meven wrote:
  
  > I quite like this feature @chrisx
  >  I hope you can still push it forward after all this time.
  
  
  I'll definitely update the patch if the maintainers show more interest in 
merging. After all I don't really like the idea of having WebKit (back when the 
patch was authored) or Chromium in my system monitor in order to view memory 
details.
  ... and I have to admit that my code style is a complete mess. I'll fix it in 
the next patch (if there is one).

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx, broulik, davidedmundson
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Méven Car]

meven added a comment.


  I quite like this feature @chrisx
  I hope you can still push it forward after all this time.

INLINE COMMENTS

> DetailsDlg.cpp:1
> +/*
> + KSysGuard, the KDE System Guard

Personally I would favor a class name with no abbreviation, here DetailsDialog, 
and here perhaps make it more explicit like ProcessDetailsDialog.

> ksysguardprocesslist.cpp:205
>  renice->setShortcut(Qt::Key_F8);
> + details = new QAction(i18n("Process Details..."), q);
>  selectParent = new QAction(i18n("Jump to Parent Process"), q);

Indentation seems off.

> ksysguardprocesslist.cpp:1225
> + QList processes = selectedProcesses();
> + if(processes.size()!=1)return;
> + QPointer detailsdlg=new 
> DetailsDlg(processes.first()->pid(),processModel(),d->mUi->treeView);

if (processes.size() != 1) {

  return;

}
You can try to use uncrustify-kf5 to 
(https://github.com/KDE/kde-dev-scripts/blob/master/uncrustify-kf5) to solve 
code style and indentation issues.
It works on whole directories only, so you might want to use `uncrustify-kf5; 
git commit --amend --no-edit [files part you are adding/touching] ; git 
checkout .`

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx, broulik, davidedmundson
Cc: meven, garywang, broulik, davidedmundson, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx added a comment.


  Any updates on whether this patch could be merged? It has been a long wait. 
If there are any improvements I can make to the patch I will be more than happy 
to put more work into it.

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx, broulik, davidedmundson
Cc: garywang, broulik, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx updated this revision to Diff 35891.
chrisx added a comment.


  F5902596: Screenshot_20180609_224923.png 

  
  F5902597: Screenshot_20180609_224936.png 

  
  Added detailed memory information to the new process details dialog. Like the 
original implementation, it displays a message if some of the items are not 
available.
  
  (Also TIL ksysguard has scripting support... which didn't show up in the 
handbook whatsoever. It is pretty powerful indeed, but it seems an overkill to 
implement detailed memory info with scripting.)

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12827?vs=33991=35891

REVISION DETAIL
  https://phabricator.kde.org/D12827

AFFECTED FILES
  lsofui/lsof.h
  processcore/process.cpp
  processcore/process.h
  processcore/processes_linux_p.cpp
  processui/CMakeLists.txt
  processui/DetailsDlg.cpp
  processui/DetailsDlg.h
  processui/DetailsDlgUI.ui
  processui/ksysguardprocesslist.cpp
  processui/ksysguardprocesslist.h

To: chrisx
Cc: broulik, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Kai Uwe Broulik]

broulik added a comment.


  That crash should be fixed by [1] which I just saw forgot to merge to master
  
  [1] 
https://cgit.kde.org/breeze.git/commit/?h=Plasma/5.13=6d886e9f75d04eb34cd34cac668606d027384f96

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx
Cc: broulik, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx added a comment.


  In D12827#275187 , @broulik wrote:
  
  > > The existing "Show detailed memory information" and "Process details". I 
think they can be merged.
  >
  > The show detailed memory information is a QtWebKit-powered "plugin" (I 
would love to get rid of tbh)
  
  
  Yeah I would personally agree with that. Actually the "Show detailed memory 
information" dialog crashes ksysguard on my machine immediately (the back trace 
ends up with libQt5WebKit.so.5). I would try adding memory info to the new 
dialog and update the diff then.

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx
Cc: broulik, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Kai Uwe Broulik]

broulik added a comment.


  > The existing "Show detailed memory information" and "Process details". I 
think they can be merged.
  
  The show detailed memory information is a QtWebKit-powered "plugin" (I would 
love to get rid of tbh)

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx
Cc: broulik, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[David Edmundson]

davidedmundson added a comment.


  As for this patch.
  
  I'm not particulalry convinced context switches is a very useful thing, 
ksysguard isn't catering to be a perf replacement. 
  But if it's hidden away in a dialog, meh, no real objections.
  
  I don't think we need to have two independent dialogs. The existing "Show 
detailed memory information" and "Process details". I think they can be merged.

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[David Edmundson]

davidedmundson added a comment.


  Relevant screenshot
  
  F5895510: image.png 

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

To: chrisx
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12827: Added process details in the process list context menu

[Chris Xiong]

chrisx created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
chrisx requested review of this revision.

REVISION SUMMARY
  The process details dialog displays more information on a process such as 
number of page faults and context switches. It also makes use of the 
(seemingly) unused lsofui library to display a list of files opened by the 
process. Currently it's only implemented for Linux.

TEST PLAN
  Not quite sure how to test this but we can always right click on any process 
in the process list and choose process details. lsofui does not work for other 
users' processes.

REPOSITORY
  R111 KSysguard Library

REVISION DETAIL
  https://phabricator.kde.org/D12827

AFFECTED FILES
  lsofui/lsof.h
  processcore/process.cpp
  processcore/process.h
  processcore/processes_linux_p.cpp
  processui/CMakeLists.txt
  processui/DetailsDlg.cpp
  processui/DetailsDlg.h
  processui/DetailsDlgUI.ui
  processui/ksysguardprocesslist.cpp
  processui/ksysguardprocesslist.h

To: chrisx
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart