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
