D10297: Add new "Tools" button above System Monitor's process list

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.
This revision is now accepted and ready to land.


  Nice, accepting this for now (there's still one simplification you could make, though, see inline comment).
  
  That said:
  
  - I only checked that the code looks good and it works fine. I'm still not too happy with how this is a button instead of a menu in KSysGuard (D10297#207127 <https://phabricator.kde.org/D10297#207127>). However, that's for #Plasma <https://phabricator.kde.org/tag/plasma/> to decide.
  - I'd appreciate if someone from the long list of #Plasma <https://phabricator.kde.org/tag/plasma/> reviewers (on Phab and from ReviewBoard where this patch was submitted in September 2016) could also approve the final version.
  
  If no one speaks up within a week, I'd say you can land the patch.

INLINE COMMENTS

> ksysguardprocesslist.cpp:430-432
> +    if (runCommandShortcutList.size() > 0) {
> +        runCommandAction->setShortcut(runCommandShortcutList[0].toString(QKeySequence::NativeText));
> +    }

Meanwhile I learned (while checking why you kept `toString(QKeySequence::NativeText)` compared to my suggestion) that this can be written even simpler, no need for the `if` at all:

  runCommandAction->setShortcuts(runCommandShortcutList);

> gregormi wrote in ksysguardprocesslist.cpp:421-427
> It was supposed to serve as reminder of how the parameters for the globalShortcut method were determined. To help debugging later. Should it be removed?

At least elsewhere <https://lxr.kde.org/source/kde/workspace/plasma-workspace/containmentactions/contextmenu/menu.cpp#0099> there is no such comment and I doubt the object's name will change in the future, but if you want to keep it, that's also fine with me.

> gregormi wrote in ksysguardprocesslist.cpp:429
> Yes, is this much better.
> 
> Might there be a chance of a some kind of keyboard shortcut conflict because we now set it also locally?

> Might there be a chance of a some kind of keyboard shortcut conflict because we now set it also locally?

Not sure whether there are any guarantees, but at least swapping the shortcuts between Run Command and Kill a Window, we can observe that in case of (silent) conflicts the global shortcut has precedence, i.e. "it works"™.

Perhaps only setting the string is cleaner, but then due to `\t` we'd have RTL problems again like in your other patch. I'd go the shortcut way, unless anybody from #Plasma <https://phabricator.kde.org/tag/plasma/> comes up with good reasons not to.

REPOSITORY
  R111 KSysguard Library

BRANCH
  arcpatch-D10297_2

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

To: gregormi, #plasma, colomar, broulik, mart, hein, rkflx
Cc: apol, anthonyfieroni, andreaska, rkflx, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180630/d45ee251/attachment-0001.html>