<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118783/">https://git.reviewboard.kde.org/r/118783/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 16th, 2014, 7:10 p.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;">I would like to get the opinion from people who designed kglobalaccel. Personally I do not understand what the default shortcuts are for and why they are needed.
>From reading the documentation my understanding is that your patch is wrong and instead the applications need to be fixed (it's just the default as what one can click in the config interface, but not the loaded global shortcut).</pre>
</blockquote>
<p>On June 16th, 2014, 10:20 p.m. UTC, <b>Vishesh Handa</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;">The difference between an "active" and "default" shortcut is that the user cannot configure a default shortcut. Have a look at the systemsettings for global shortcuts. You'll get a better idea.
About the application being wrong, I disagree. Most people would think that calling 'setDefaultShortcut(..)' would actually set the shortcut and not just write it to a config file. The way I see it, we either except this patch, or we change kglobalaccel (the daemon) to actually utilize the default shortcut. If you want, I can submit a patch for that. It's just about 4-5 lines.</pre>
</blockquote>
<p>On June 17th, 2014, 5:40 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;">I don't know. Given the documentation I understand it as "this is just the default shortcut", if one wants to have the global shortcut, the setShortcut method needs to be used. Whether it's a valid use case to only have the default shortcut, I'm not sure. But I assume that this was intended behavior by the people who implemented it.
Overall my feeling is that default is not intended to be used at all. But as said: we need input here from people more experienced with kglobalaccel</pre>
</blockquote>
<p>On June 17th, 2014, 11:10 a.m. UTC, <b>Vishesh Handa</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;">I've been running this patch for a day now, and it is buggy. Custom shortcuts seem to get overwritten with the default one. I'm not bothering to fix it, as it would be best to figure out what course of action we want to take.</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;">As mentioned during the meeting today: if should set the default and active shortcut.</pre>
<br />
<p>- Kevin</p>
<br />
<p>On June 16th, 2014, 4:49 p.m. UTC, Vishesh Handa wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and Martin Gräßlin.</div>
<div>By Vishesh Handa.</div>
<p style="color: grey;"><i>Updated June 16, 2014, 4:49 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kglobalaccel
</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;"> When a client calls setDefaultShortcut, only the default shortcut is
set. This makes sense, however kglobalaccel doesn't actually do anything
with the global shortcut, it just writes it to the configuration and
reads it back.
All the actual logic is implemented behind "active" or "normal" shortcuts.
In kdelibs4, most applications would call KAction::setGlobalShorcut which
had a default of setting BOTH the active and the default shortcut. Again,
I'm not sure what they point of this was if the default shortcut does not
actually do anything.
This fixes bugs such as the brightness key not working because
Powerdevil only sets the "default" shortcut.
</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>src/kglobalaccel.cpp <span style="color: grey">(54d18ec)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118783/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>