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

[Henrik Fehlauer]

rkflx added a comment.


  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).

INLINE COMMENTS

> CMakeLists.txt:37
>  
> -find_package(KF5 REQUIRED COMPONENTS CoreAddons Config I18n WindowSystem Completion Auth WidgetsAddons IconThemes ConfigWidgets Service)
> +find_package(KF5 REQUIRED COMPONENTS CoreAddons Config I18n WindowSystem Completion Auth WidgetsAddons IconThemes ConfigWidgets Service Plasma GlobalAccel KIO)
>  find_package(KF5 OPTIONAL_COMPONENTS Plasma)

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.

> ProcessWidgetUI.ui:71
>         </property>
> -       <property name="clearButtonEnabled" stdset="0">
>          <bool>true</bool>

Unrelated change?

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

> ksysguardprocesslist.cpp:386
>  
> +    d->mUi->btnKillProcess->setEnabled(false);
>      d->mUi->btnKillProcess->setIcon(QIcon::fromTheme(QStringLiteral("process-stop")));

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

> ksysguardprocesslist.cpp:390
> +
> +    auto d1 = d;
> +    auto addByDesktopName = [this, d1](const QString& desktopName)

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`.

> ksysguardprocesslist.cpp:395
> +        auto kService = KService::serviceByDesktopName(desktopName);
> +        if (kService != nullptr) {
> +            auto action = new QAction(QIcon::fromTheme(kService->icon()),

Drop `!= nullptr`.

> ksysguardprocesslist.cpp:412
> +    // And in this case we do not add the KSysGuard item to the menu.
> +    if (!QCoreApplication::applicationFilePath().contains(QStringLiteral("ksysguard"))) {
> +        addByDesktopName(QStringLiteral("org.kde.ksysguard"));

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.

> ksysguardprocesslist.cpp:419-420
> +    addByDesktopName(QStringLiteral("org.kde.filelight"));
> +    addByDesktopName(QStringLiteral("sweeper"));
> +    addByDesktopName(QStringLiteral("kmag"));
> +    addByDesktopName(QStringLiteral("htop"));

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… :)

> ksysguardprocesslist.cpp:423
> +
> +    {
> +        auto action = new QAction(i18n("Run Command"), this);

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

> ksysguardprocesslist.cpp:424
> +    {
> +        auto action = new QAction(i18n("Run Command"), this);
> +        action->setIcon(QIcon::fromTheme(QStringLiteral("system-run")));

Please give your variable a more descriptive name.

> gregormi wrote in ksysguardprocesslist.cpp:360
> does this comment still apply?

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

REPOSITORY
  R111 KSysguard Library

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

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