> On Jan. 26, 2015, 7:05 a.m., Martin Gräßlin wrote:
> > My opinion is that this is a feature which should not be exposed in
> > libksysguard. It actually ties libksysguard to KWin, while libksysguard was
> > in the past also used in e.g. kdevelop.
> >
> > If libksysguard wants to offer the functionality to kill a window, it
> > should implement it itself.
>
> Martin Gräßlin wrote:
> In addition: KWin's global shortcut action names are not public API. We
> do not guarantee that we don't change them, we do not guarantee that they are
> exposed at all (KWin handling shortcuts internally without kglobalaccel on
> Wayland?). I do not want to run into situations that we cannot change our
> code because external usage makes it impossible.
>
> Thomas Lübking wrote:
> In case there was larger demand for invoking such action (taskbar,
> dedicated plasmoid, ...) one could move the xkill functionality into
> KWindowSystem (option for portage) - invoking a kwin shortcut through a
> kglobalaccel dbus call is a hack. Maybe sufficient for any downstream
> solution, but easily broken feature.
>
> Gregor Mi wrote:
> First of all, a clarification of this RR's intentions:
> 1) The original "End Process..." tooltip says "you can always use
> Ctrl+Alt+Esc..." which is wrong as soon as someone changes the keyboard
> shortcut exposed by KWin. So this should be fixed.
> 2) Make the Kill Window feature more discoverable. It is a seldom used
> feature which makes it harder to remember.
>
> About invoking Kill Window:
> > If libksysguard wants to offer the functionality to kill a window, it
> should implement it itself. [Martin]
> > ...one could move the xkill functionality into KWindowSystem...
> [Thomas]
>
> Without knowing the amount of xkill code I suspect that having a dbus
> call that loosly couples libksysguard to KWin is probably easier to maintain
> than 2 times the xkill code.
> Having said that, what about moving the xkill code to a common location
> as Thomas suggested?
>
> > We do not guarantee that we don't change them, we do not guarantee that
> they are exposed at all ... I do not want to run into situations that we
> cannot change our code because external usage makes it impossible.
>
> Understood. But would it then be possible at all to get the current
> shortcut to display it to the user?
>
> Martin Gräßlin wrote:
> Ok, so this addresses two issues using one solution: exposing KWin's
> internal shortcut. This is bad as outlined above.
>
> I agree that 1) needs fixing. This can be done in the way as approached
> in this review request: check whether kwin is registered on kglobalaccel and
> get the key command. If it's done that way the fault is with libksysguard in
> case KWin changes the shortcut name or doesn't use kglobalaccel any more.
> Another fix is of course to just hide the shortcut.
>
> 2) is a different issue. Whether it's needed to expose the functionality
> in addition from libksysguard is probably questionable. A better approach to
> do this would be through a method in KWindowSystem. This does not need to
> duplicate the code, it could also just send a client message to the window
> manager to start the kill window process. Through KWindowSystem we can check
> whether the feature is supported by the window manager and could exclude if
> not supported. But and that's a big but: the feature would not be able to
> work if it's triggered from a (context) menu or drop down list (it needs to
> grab mouse). Given that I'm hesistant to say that it should be added to
> kwindowsystem at all.
>
> Thomas Lübking wrote:
> ad 2)
> I'd have said to rather *move* the code to KWindowSystem and use it from
> there by any client (incl. kwin)
> This allows porting the solution (in case such is possible on other
> systems at all) as well as to invoke the feature unconditionally (ie. instead
> of "is this kwin? yes? tell kwin to trigger xkill." just trigger the xkill
> functionality)
>
> About the popupmenu:
> The issue is global, ie. as long as a popup (or other grabber) is
> around, the kwin shortcut neither works.
> It's kind of the client codes problem to deal w/ a "false" return (eg.
> invoke a timer and/or timered retries)
ad 1) (shortcut)
I could live with adapting (or remove) the shortcut retrieval as soon as it
will not be possible anymore. As long as it is, I would show it. (I suspect as
long as the shortcut is not hard-coded there will be a some way to get it)
ad 2) (invoke window kill)
I looked a Kwin's source code. For reference, here are the two methods I found
to kill a window:
```
/*!
Kill Window feature, similar to xkill
*/
void Workspace::slotKillWindow()
{
if (m_windowKiller.isNull()) {
m_windowKiller.reset(new KillWindow());
}
m_windowKiller->start();
}
and
/**
* Kills the window via XKill
*/
void Client::killWindow()
{
qCDebug(KWIN_CORE) << "Client::killWindow():" << caption();
killProcess(false);
m_client.kill(); // Always kill this client at the server
destroyClient();
}
```
About the context menu / grabber issue: Without knowing much of the details,
isn't it possible to use a timer or similar to circumvent the problem? Too
hacky?
About exposing the xkill method in ksysguard I would say from a user's
perspective:
- The killing method requires the me to click for a window kill.
- So why not be able to invoke this method using the the mouse, too?
- Gregor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review74740
-----------------------------------------------------------
On Jan. 25, 2015, 6:21 p.m., Gregor Mi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122249/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2015, 6:21 p.m.)
>
>
> Review request for KDE Base Apps, Martin Gräßlin and John Tapsell.
>
>
> Repository: libksysguard
>
>
> Description
> -------
>
> Current situation:
> The "End Process..." button has a tooltip which says "To target a specific
> window to kill, press Ctrl+Alt+Esc at any time." The keyboard shortcut is
> hardcoded.
>
> RR:
> Add a drop down menu to the "End Process..." button with one action:
> i18n("Kill a specific window... (Global shortcut: %1)", killWindowShortcut)
>
>
> Diffs
> -----
>
> processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6
> processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3
> processui/keyboardshortcututil.h PRE-CREATION
> processui/keyboardshortcututil.cpp PRE-CREATION
> processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
> tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546
> tests/keyboardshortcututiltest.h PRE-CREATION
> tests/keyboardshortcututiltest.cpp PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/122249/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gregor Mi
>
>
