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
