> On Jan. 26, 2015, 8: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) > > Gregor Mi wrote: > 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? > > Martin Gräßlin wrote: > The first code is the one which triggers the killer, you find it actually > in killwindow.[h|cpp]. > > Concerning moving the functionality directly into KWindowsystem I have > two concerncs: cross platform (the solution is X11 only and any killing in > Wayland needs to be done in the compositor, not to mention OSX and > Windows...) and licence: we would have to hunt down all people who committed > to it to change to LGPL. > > Overall I think a trigger is easier to implement in a cross platform way. > > Gregor Mi wrote: > > Overall I think a trigger is easier to implement in a cross platform > way. > > By that you mean it is easier 1) to copy the killwindow.h|cpp to > libksysguard or 2) to keep the RR's dbus call that triggers KWin's killWindow > method?
neither nor: I meant a client message to the root window (that's the way one talks to window managers ;-) - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review74740 ----------------------------------------------------------- On Jan. 28, 2015, 9:35 p.m., Gregor Mi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122249/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2015, 9:35 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 > ------- > > > File Attachments > ---------------- > > New End Process button with drop down arrow > > https://git.reviewboard.kde.org/media/uploaded/files/2015/01/28/16301e88-e21b-4358-9a63-a85dae5722bd__screenshot_default1.png > Drop down shows Kill Window > > https://git.reviewboard.kde.org/media/uploaded/files/2015/01/28/58df12c5-7350-4bb0-b602-c5716caa9836__screenshot_default2.png > > > Thanks, > > Gregor Mi > >