> On Jan. 26, 2015, 7:05 vorm., 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?
>
> Martin Gräßlin wrote:
> neither nor: I meant a client message to the root window (that's the way
> one talks to window managers ;-)
>
> Gregor Mi wrote:
> Good to know. :-)
>
> To summarize
> 1) display shortcut: the proposed method via kglobalaccel has its
> disadvantages but there is not better solution so far.
>
> 2) trigger killing action: I currently see different methods with
> different advantages/disadvantages with no clear view which one is best. So
> should I leave it with the dbus call or go in another direction?
>
> Martin Gräßlin wrote:
> While I do not like 1, I guess it's the only possibility.
>
> For 2) I suggest to absolutely not go for triggering the shortcut due to
> the reasons I mentioned in the first comment here: it ties it to KWin in a
> strange way. Please go for an approach either in KWindowSystem or copy the
> code from KWin to ksysguard.
>
> Gregor Mi wrote:
> I tried to copy the killwindow code from KWin which would mean to copy
> #include "killwindow.h"
> #include "client.h"
> #include "cursor.h"
> #include "workspace.h"
> #include "xcbutils.h"
> plus cpp files plus further dependencies which seems like overkill.
>
> Thomas Lübking wrote:
> s/copy/adapt/g - you would not need half of the stuff, client.h is *very*
> kwin specific - you'd have to copy the kwin eventfilter to build a client
> list in the first place.
>
> Personally I'd however (still) say that if 2 or more (basic) clients need
> it, it should move to a lib (and there we would not require additional
> dependencies at all) - and calling kwin is wonky (at least you would have to
> test the WM before offering the feature, imo that could never go into a
> public API)
>
> Gregor Mi wrote:
> > Personally I'd however (still) say that if 2 or more (basic) clients
> need it, it should move to a lib
>
> +1 but currently there is only libksysguard, isn't it?
>
> Thomas Lübking wrote:
> KWin? :-P
>
> Martin, if you should generally be fine with this but lack the time, I
> volunteer moving the code.
> (I'd object exposing "proprietary" WM functionality in public API and
> also don't think that "kill the process for this window or disconnect the
> client" is a WM specific task either - proof: xkill ;-)
>
> Martin Gräßlin wrote:
> concerning splitting it out I must ask the obvious question: what about
> Wayland? Such a workflow cannot work outside the compositor. So splitting out
> of KWin would mean moving it back into KWin for Wayland (granted it needs new
> code anyway which is less X11 specific) and the library would directly break
> with Wayland.
>
> Given that I'd prefer to have a property on the root window to indicate
> it's supported and send client message to root window. Yes it would be kwin
> specific at the moment, but we could take it to the EWMH spec.
> what about Wayland?
I assume we'd require a common compositor (dbus?) interface for wayland anyway?
Eg. obtaining the PID by picking a window might be interesting for ksysguard to
only show the resource load for that window.
And while the compositor would reasonably dump a window image, it should
certainly not provide a screenshot util, that may eg. allow to do basic
postprocessing (crop, blur, circle, splot, arrow annotate, ...) or upload to
flickr and whatnot - the solution should (can!) also not be that ksnapshot
works w/ kwin, but not enlightenment or whatever.
Such functionality would then reasonably go into kwindowsystem?
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review74740
-----------------------------------------------------------
On Feb. 20, 2015, 11:35 nachm., Gregor Mi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122249/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2015, 11:35 nachm.)
>
>
> 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 450ca600b8aed7ca611ec638610b6c524c96080c
> 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
>
>
