[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 118783: Set the normal shortcut when setting the default shortcut as well
From: "Kevin Ottens" <ervin () kde ! org>
Date: 2014-06-17 16:34:08
Message-ID: 20140617163408.3734.21964 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On June 16, 2014, 7:10 p.m., Martin Gräßlin wrote:
> > 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).
>
> Vishesh Handa wrote:
> 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.
> Martin Gräßlin wrote:
> 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
> Vishesh Handa wrote:
> 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.
As mentioned during the meeting today: if should set the default and active shortcut.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118783/#review60209
-----------------------------------------------------------
On June 16, 2014, 4:49 p.m., Vishesh Handa wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118783/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 4:49 p.m.)
>
>
> Review request for KDE Frameworks and Martin Gräßlin.
>
>
> Repository: kglobalaccel
>
>
> Description
> -------
>
> 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.
>
>
> Diffs
> -----
>
> src/kglobalaccel.cpp 54d18ec
>
> Diff: https://git.reviewboard.kde.org/r/118783/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vishesh Handa
>
>
[Attachment #5 (text/html)]
<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>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic