<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 January 26th, 2015, 7:05 a.m. 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 January 26th, 2015, 7:07 a.m. 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 January 26th, 2015, 11:31 a.m. 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 January 26th, 2015, 6:40 p.m. 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 January 27th, 2015, 6:54 a.m. 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 January 27th, 2015, 2:44 p.m. 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 January 27th, 2015, 7:49 p.m. 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 January 28th, 2015, 7:55 a.m. 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>
</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;"><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>
<br />
<p>- Gregor</p>
<br />
<p>On January 25th, 2015, 6:21 p.m. 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 Jan. 25, 2015, 6:21 p.m.</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">(4dc142e864d8353ceafc3a6735ffa81e48291420)</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>
</td>
</tr>
</table>
</div>
</body>
</html>