[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyb
From: Thomas_Lübking <thomas.luebking () gmail ! com>
Date: 2015-02-21 21:58:39
Message-ID: 20150221215839.4989.93799 () probe ! kde ! org
[Download RAW message or body]
--===============7486659233336068425==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> 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?
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 ;-)
- 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
>
>
--===============7486659233336068425==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/122249/">https://git.reviewboard.kde.org/r/122249/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On Januar 26th, 2015, 7:05 vorm. UTC, <b>Martin \
Gräßlin</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">If libksysguard wants \
to offer the functionality to kill a window, it should implement it itself.</p></pre> \
</blockquote>
<p>On Januar 26th, 2015, 7:07 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> </blockquote>
<p>On Januar 26th, 2015, 11:31 vorm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> </blockquote>
<p>On Januar 26th, 2015, 6:40 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">About invoking Kill Window:</p> \
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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]</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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?</p> <blockquote \
style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p> </blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Understood. But would it then be possible at all to \
get the current shortcut to display it to the user?</p></pre> </blockquote>
<p>On Januar 27th, 2015, 6:54 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, \
so this addresses two issues using one solution: exposing KWin's internal shortcut. \
This is bad as outlined above.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre> </blockquote>
<p>On Januar 27th, 2015, 2:44 nachm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ad 2) \
I'd have said to rather <em style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: normal;">move</em> 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)</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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)</p></pre> </blockquote>
<p>On Januar 27th, 2015, 7:49 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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)</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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:</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div \
class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span \
style="color: #408080; font-style: italic">/*!</span> <span style="color: #408080; \
font-style: italic"> Kill Window feature, similar to xkill</span> <span \
style="color: #408080; font-style: italic"> */</span> <span style="color: #008000; \
font-weight: bold">void</span> Workspace<span style="color: \
#666666">::</span>slotKillWindow() {
<span style="color: #008000; font-weight: bold">if</span> \
(m_windowKiller.isNull()) {
m_windowKiller.reset(<span style="color: #008000; font-weight: \
bold">new</span> KillWindow()); }
m_windowKiller<span style="color: #666666">-></span>start();
}
<span style="color: #008000; font-weight: bold">and</span>
<span style="color: #408080; font-style: italic">/**</span>
<span style="color: #408080; font-style: italic"> * Kills the window via XKill</span>
<span style="color: #408080; font-style: italic"> */</span>
<span style="color: #008000; font-weight: bold">void</span> Client<span style="color: \
#666666">::</span>killWindow() {
qCDebug(<span style="color: #880000">KWIN_CORE</span>) <span style="color: \
#666666"><<</span> <span style="color: \
#BA2121">"Client::killWindow():"</span> <span style="color: \
#666666"><<</span> caption(); killProcess(false);
m_client.kill(); <span style="color: #408080; font-style: italic">// Always kill \
this client at the server</span> destroyClient();
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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?</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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?</p></pre>
</blockquote>
<p>On Januar 28th, 2015, 7:55 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The \
first code is the one which triggers the killer, you find it actually in \
killwindow.[h|cpp].</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Overall I think a trigger is easier to \
implement in a cross platform way.</p></pre> </blockquote>
<p>On Januar 28th, 2015, 8:30 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote \
style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Overall I think a trigger is easier to implement in a cross platform \
way.</p> </blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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?</p></pre> </blockquote>
<p>On Januar 29th, 2015, 8:09 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">neither nor: I meant a client message to the root window (that's the way \
one talks to window managers ;-)</p></pre> </blockquote>
<p>On Januar 29th, 2015, 6:44 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Good \
to know. :-)</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">To summarize 1) display shortcut: the proposed method \
via kglobalaccel has its disadvantages but there is not better solution so far.</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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?</p></pre> \
</blockquote>
<p>On Februar 2nd, 2015, 7:41 vorm. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While \
I do not like 1, I guess it's the only possibility.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> </blockquote>
<p>On Februar 20th, 2015, 11:20 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
tried to copy the killwindow code from KWin which would mean to copy</p> <h1 \
style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">include "killwindow.h"</h1> <h1 style="font-size: \
100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">include "client.h"</h1> <h1 style="font-size: 100%;text-rendering: \
inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">include \
"cursor.h"</h1> <h1 style="font-size: 100%;text-rendering: inherit;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">include "workspace.h"</h1> <h1 \
style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">include "xcbutils.h"</h1> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">plus \
cpp files plus further dependencies which seems like overkill.</p></pre> \
</blockquote>
<p>On Februar 21st, 2015, 4:02 nachm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">s/copy/adapt/g - you would not need half of the stuff, client.h is <em \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">very</em> kwin specific - you'd have to copy the kwin eventfilter to build a \
client list in the first place.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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)</p></pre> </blockquote>
<p>On Februar 21st, 2015, 7:07 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote \
style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Personally I'd however (still) say that if 2 or more (basic) clients need \
it, it should move to a lib</p> </blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">+1 but currently there is only libksysguard, isn't \
it?</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KWin? \
:-P</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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 \
;-)</p></pre> <br />
<p>- Thomas</p>
<br />
<p>On Februar 20th, 2015, 11:35 nachm. UTC, Gregor Mi wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Base Apps, Martin Gräßlin and John Tapsell.</div>
<div>By Gregor Mi.</div>
<p style="color: grey;"><i>Updated Feb. 20, 2015, 11:35 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
libksysguard
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">RR: Add a drop down \
menu to the "End Process..." button with one action: i18n("Kill a specific window... \
(Global shortcut: %1)", killWindowShortcut)</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>processui/CMakeLists.txt <span style="color: \
grey">(7f87b85e0201e63d69070a71203bbb34851a79c6)</span></li>
<li>processui/ProcessWidgetUI.ui <span style="color: \
grey">(e50f55cf1813b00d49b1716023df487ffbd536e3)</span></li>
<li>processui/keyboardshortcututil.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>processui/keyboardshortcututil.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>processui/ksysguardprocesslist.cpp <span style="color: \
grey">(450ca600b8aed7ca611ec638610b6c524c96080c)</span></li>
<li>tests/CMakeLists.txt <span style="color: \
grey">(967b03fae1e460bfb22e1a07ef05cf7b49412546)</span></li>
<li>tests/keyboardshortcututiltest.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>tests/keyboardshortcututiltest.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122249/diff/" style="margin-left: \
3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments \
</h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/01/28/16301e88-e21b-4358-9a63-a85dae5722bd__screenshot_default1.png">New \
End Process button with drop down arrow</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/01/28/58df12c5-7350-4bb0-b602-c5716caa9836__screenshot_default2.png">Drop \
down shows Kill Window</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============7486659233336068425==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic