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

Authored by gregormi on Feb 4 2018, 3:07 PM.

Details

Summary

This adds a new "Tools" button to the libksysguard widget which opens a menu that contains tools that help with system diagnostics.

Screenshots:

Original (English):

Localized (German):

Some tools not installed / old menu version:

Test Plan
  • Verify that each action in the menu works.
  • Verify that if the global keyboard shortcut for Kill Window is changed, that it is updated accordingly in the menu item.

Diff Detail

Repository
R111 KSysguard Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
anthonyfieroni added inline comments.
processui/ksysguardprocesslist.cpp
200

new QMenu(q) or delete in destructor.

macOS uses a little gear icon for these kinds of menus:

+1, if Breeze had offered it (or did I just fail to find it so far?), I would've chosen it for Spectacle.

To me it seems quite common to have the need for such a button (now that we have less toolbar + menubar style apps), it might be worth to create a new icon.

+2, wanna open a new bug for that? @andreaska can probably whip it up really quickly; he's a wizard with icons.

rkflx added a comment.Feb 11 2018, 8:05 PM

+2, wanna open a new bug for that? @andreaska can probably whip it up really quickly; he's a wizard with icons.

@ngraham Sorry, was busy working on Spectacle and Gwenview so did not get around to this yet. Would you mind filing the bug?

@gregormi I think in the meantime we could just use application-menu?

@ngraham Sorry, was busy working on Spectacle and Gwenview so did not get around to this yet. Would you mind filing the bug?

https://bugs.kde.org/show_bug.cgi?id=390285

@gregormi I think in the meantime we could just use application-menu?

+1

gregormi updated this revision to Diff 27221.Feb 15 2018, 10:15 AM
gregormi marked an inline comment as done.
  • fix mem leak
  • Set application-menu icon for Tools button
gregormi updated this revision to Diff 27222.Feb 15 2018, 10:17 AM
  • Merge remote-tracking branch 'origin/master'
gregormi updated this revision to Diff 27223.Feb 15 2018, 10:27 AM
  • add missing include
gregormi edited the summary of this revision. (Show Details)Feb 15 2018, 10:28 AM
gregormi updated this revision to Diff 27231.Feb 15 2018, 10:55 AM
gregormi marked an inline comment as done.
  • remove unrelated changes
gregormi marked 2 inline comments as done.Feb 15 2018, 10:55 AM

Ready from my side.

processui/ksysguardprocesslist.cpp
361

does this comment still apply?

Unrelated whitespace changes here: https://phabricator.kde.org/D10535

rkflx added a comment.Feb 15 2018, 4:07 PM

Just tried the patch. I think for System Activity / Ctrl+ the button is fine (annoyingly with a slightly larger height than the combobox, but that's life…). For KSysGuard itself it looks a bit odd, why would it appear exactly on this tab page? I could imagine the menubar would be a better place.

(Sorry this comes so late, I only reviewed in more detail because nobody from Plasma bothered so far. Ignore my comment if you want.)

I think putting on that tab seems sensible enough because it's one of the default tabs, and the one that ksysguard opens to. Anybody sho'w such an advanced user that they customize their ksysguard tabs and don't use this one isn't actually going to need these tools in the first place.

Just tried the patch. I think for System Activity / Ctrl+ the button is fine (annoyingly with a slightly larger height than the combobox, but that's life…). For KSysGuard itself it looks a bit odd, why would it appear exactly on this tab page? I could imagine the menubar would be a better place.

(Sorry this comes so late, I only reviewed in more detail because nobody from Plasma bothered so far. Ignore my comment if you want.)

Some people from Plasma reviewed the original review request on reviewboard. Should we add some more reviewers before it can go in?

rkflx added a comment.Mar 7 2018, 10:57 AM

Some people from Plasma reviewed the original review request on reviewboard. Should we add some more reviewers before it can go in?

Only looked at it briefly, but I did not spot something about KSysGuard (the multi-page app), it was all about "System Monitor" (the single page dialog) so far. Anyway, I'm not the maintainer, I only added a comment about something I noticed.

As for adding more reviewers: I guess someone from Plasma has to approve it in the end, and maybe look at the code again because the original review is quite old by now. Perhaps it's best to tag individual reviewers (from your old review, or from recent Git history).

Some people from Plasma reviewed the original review request on reviewboard. Should we add some more reviewers before it can go in?

Only looked at it briefly, but I did not spot something about KSysGuard (the multi-page app), it was all about "System Monitor" (the single page dialog) so far. Anyway, I'm not the maintainer, I only added a comment about something I noticed.

I think the reviewers are aware that the menu will also be part of KSysGuard, which is a good thing. I added Thomas from the original review request and Friedrich as one from the recent commit history as additional reviewers.

So the final question to be decided is: "For KSysGuard: is it odd that the new tools menu only appears in the System Table tab?" I would follow Nate's assessment https://phabricator.kde.org/D10297#207235 here and say no and regarding all circumstances it is a good solution.

I'm a +1, FWIW.

Any more thoughts on this patch before it is ready to land?

Sometimes silence equals consent, especially when there have been many requests for comment. If nobody else has an opinion before then, I propose landing this on March 20th.

apol added a subscriber: apol.Mar 13 2018, 6:47 PM

Here's some thoughts:

  • Why is there only one with the keyboard shortcut? many of these have (e.g. run a command)
  • maybe sort the menu alphabetically first? or there's any meaning?
In D10297#224935, @apol wrote:

Here's some thoughts:

  • Why is there only one with the keyboard shortcut? many of these have (e.g. run a command)

The initial idea of the tools menu was to make the Kill Window more discoverable. Yes, Run Command has also a global shortcut (but it is the only additional one). I didn't think of adding the shortcut because there are several ways to start a program.

  • maybe sort the menu alphabetically first? or there's any meaning?

I put Konsole first, because, in my experience, this is what most likely works in case of some unexpected Plasma behaviour.

hein added a comment.Mar 14 2018, 10:58 AM

I haven't been able to review the patch in detail myself, but the basic idea is fantastic IMHO.

rkflx added a comment.EditedMar 20 2018, 12:25 PM

The new access button is really great, but Kill a window still looks a bit unfinished to me:

  • The text should use title case: "Kill a Window"
  • The shortcut does not get localized. (In the global shortcuts KCM, it is localized correctly for me.)
  • The parentheses look a bit ugly, in normal menus the shortcuts are simply aligned to the right (not sure how that's done, though).
In D10297#224935, @apol wrote:
  • Why is there only one with the keyboard shortcut? many of these have (e.g. run a command)

FWIW, I agree with Aleix. At least for Run Command the shortcut should be shown (just like in the desktop context menu).

CMakeLists.txt
37

Is Plasma needed here? If so, I think this needs more discussion or should be made optional.

KSysGuard can also be used in other desktop environments and perhaps other apps consume this library too (KDevelop?), all of them might not want to bring in a huge dependency on Plasma.

processui/ProcessWidgetUI.ui
71

Unrelated change?

(Comment applies also to a bunch of other places with the same change.)

processui/ksysguardprocesslist.cpp
361

Yes, it still shows up as a whitespace change on Phabricator.

386

Do you really need this? Maybe it's enough if you don't change this property to true in processui/ProcessWidgetUI.ui:31.

389

What does d1 mean? Is it necessary to duplicate the d-pointer?

As far as I can see, you are already capturing this in the lambda, so you won't need to capture (this->)d.

394

Drop != nullptr.

411

Could you do an exact match on the filename, i.e. only the last part of the full path? There might be situations where "System Monitor" is developed or installed in a directory containing this string by chance.

418–419

Sweeper and KMag do not show up for me, even though I have both of them installed. Is your naming correct?

(Sidenote: This smells like it should be handled by your KMoreTools, although that would probably require some changes there first… :)

422

This looks a bit odd. I would organize this with linebreaks alone, no need for separate code blocks.

423

Please give your variable a more descriptive name.

rkflx added inline comments.Mar 21 2018, 10:27 PM
processui/ksysguardprocesslist.cpp
411

Recently I learned comparing with qApp->desktopFileName() might be even better than looking at the path of the executable…

gregormi updated this revision to Diff 34199.May 15 2018, 2:28 PM
gregormi marked 6 inline comments as done.
  • Revert unrelated changes
  • Remove not needed Plasma dependency
  • Undo unrelated whitespace change
  • Clean code
  • Use blank lines instead of blocks
gregormi marked an inline comment as done.May 15 2018, 2:29 PM
gregormi added inline comments.
CMakeLists.txt
37

No, it's not. I'll remove that.

processui/ProcessWidgetUI.ui
71

I removed those now.

processui/ksysguardprocesslist.cpp
418–419

I fixed the desktop names.

422

Done.

gregormi marked 2 inline comments as done.May 15 2018, 2:29 PM
gregormi marked 2 inline comments as done.
gregormi marked 4 inline comments as done.
gregormi added inline comments.May 15 2018, 3:16 PM
processui/ksysguardprocesslist.cpp
411

Interesting. This indeed returns "org.kde.ksysguard". Do you have an idea, how Qt knows this desktop name? The only occurrences of this string in the ksysguard project are:

  • gui/CMakeLists.txt: install( PROGRAMS org.kde.ksysguard.desktop DESTINATION ${KDE_INSTALL_APPDIR} )
  • and gui/org.kde.ksysguard.desktop itself.
gregormi updated this revision to Diff 34204.May 15 2018, 3:24 PM
  • Use qApp->desktopFileName()
gregormi marked 2 inline comments as done.May 15 2018, 3:25 PM

This now ready from my side.

@rkflx: Hi Henrik, I resolved all your code remarks. Could you look it over and give a go to have this landed.

rkflx added a comment.May 23 2018, 9:17 PM

@rkflx: Hi Henrik, I resolved all your code remarks. Could you look it over and give a go to have this landed.

Thanks, I noticed your update, but I've been busy (sorry). I hope you don't expect an answer within days if it took you almost 2 months for the fixes ;) Anyway, I'll try to check it until the weekend.

Did you have a chance to look into the shortcut issues mentioned in D10297#229850?

Great, looking much better than before. As far as I can see the inline comments are done.

  • The text should use title case: "Kill a Window"

Not done.

  • The shortcut does not get localized. (In the global shortcuts KCM, it is localized correctly for me.)
  • The parentheses look a bit ugly, in normal menus the shortcuts are simply aligned to the right (not sure how that's done, though).
  • At least for Run Command the shortcut should be shown (just like in the desktop context menu).

Are those points impossible to achieve? At least in other places they seem to work…

processui/ksysguardprocesslist.cpp
411

Interesting question ;)

After looking into Qt's sources for desktopFileName, I set a breakpoint on QGuiApplication::setDesktopFileName, and ended up in KAboutData::setApplicationData.

In ksysguard.cpp you then have

KAboutData aboutData( QStringLiteral("ksysguard"), …
…
QStringLiteral("ksysguard")aboutData.setOrganizationDomain(QByteArray("kde.org"));

…which looks related (did not check in detail, though).

gregormi updated this revision to Diff 35172.May 30 2018, 8:37 AM

The text should use title case: "Kill a Window"

DONE

The parentheses look a bit ugly, in normal menus the shortcuts are simply aligned to the right (not sure how that's done, though).

DONE: align the shortcut strings to the right using \t

At least for Run Command the shortcut should be shown (just like in the desktop context menu)

DONE: Add shortcut to 'Run Command' item

The shortcut does not get localized. (In the global shortcuts KCM, it is localized correctly for me.)

DONE: using QKeySequence::NativeText; tested with "export LANGUAGE=de_DE"

Remove seemingly useless 'class KGlobalAccel'

gregormi edited the summary of this revision. (Show Details)May 30 2018, 8:42 AM
gregormi marked 2 inline comments as done.May 30 2018, 8:48 AM
gregormi added a reviewer: rkflx.
rkflx added a comment.Jun 2 2018, 6:56 PM

Awesome, I did not expect that the fix for the shortcuts would be so simple. Thanks for your patience, we are nearly there…

processui/ksysguardprocesslist.cpp
421–427

Left-over debug code?

429

Do you think it is really necessary to display not set? For me, an empty string would also work just fine, now that you are using \t instead of (…).

Also I found a way to omit the string and \t entirely and let Qt do all the work:

auto runCommandAction = new QAction(i18nc("@action:inmenu", "Run Command"), this);
const auto runCommandShortcutList = KGlobalAccel::self()->globalShortcut(QStringLiteral("krunner"), QStringLiteral("run command"));
if (runCommandShortcutList.size() > 0) {
    runCommandAction->setShortcut(runCommandShortcutList[0]);
}
kossebau resigned from this revision.Jun 27 2018, 11:32 AM
gregormi updated this revision to Diff 36895.Jun 29 2018, 4:46 PM
gregormi marked an inline comment as done.
gregormi edited the summary of this revision. (Show Details)
  • info comment
  • Remove the now useless "not set" string
  • Set shortcut text with Qt methods and omit the translation context string and \t
This comment was removed by gregormi.
processui/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?

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?

rkflx accepted this revision.Jun 30 2018, 9:50 PM

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). However, that's for Plasma to decide.
  • I'd appreciate if someone from the long list of 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.

processui/ksysguardprocesslist.cpp
421–427

At least elsewhere 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.

429

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 comes up with good reasons not to.

430–432

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);
This revision is now accepted and ready to land.Jun 30 2018, 9:50 PM
gregormi updated this revision to Diff 37782.Jul 15 2018, 7:35 AM
gregormi marked an inline comment as done.
  • Use setShortcuts to simplify code
gregormi updated this revision to Diff 37783.Jul 15 2018, 7:36 AM
  • Use setShortcuts to simplify code
This revision was automatically updated to reflect the committed changes.

Awesome, thanks for your patience over the last months.

You're welcome. And: thanks for the patience to help improving this patch comes also from my side :).