<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="http://git.reviewboard.kde.org/r/104801/">http://git.reviewboard.kde.org/r/104801/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 10th, 2012, 8:38 a.m., <b>David Faure</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;">As documented, the shortcut(types) method allows to retrieve either the current shortcuts (like most other methods), or the *default* shortcuts, i.e. those that were set by the application developer initially, before the user could make changes.
This is done using dynamic properties on the action, so that the configuration dialog can handle both QActions and KActions, IIRC.
So it sounds to me like you'll have to change these dynamic properties, from "defaultPrimaryShortcut"/"defaultSecondaryShortcut" to a single property containing a list of default shortcuts.
I just saw your reply about plans for cleaning this up in kdelibs frameworks -- this would be MORE than welcome.
The only plan right now is http://article.gmane.org/gmane.comp.kde.devel.frameworks/350 but I think you have more details on the shortcut handling itself.
Ideally, I would like the handling of default shortcuts to go into Qt itself, so that we don't need KAction anymore in the simple case of a non-global action, and still have configurable shortcuts. Anyway, let's discuss this further on kde-frameworks-devel.
I agree with Michael Jansen that this patch alone isn't enough, but if you also plan to improve the shortcuts editor to support more than 2 alternates, and then look into Qt5/KF5, then I support this patch, as a first step in the right direction.</pre>
</blockquote>
<p>On May 10th, 2012, 9:47 a.m., <b>Mark Gaiser</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;">Yes, i am going to improve the shortcut editor for this. The shortcut editor should then simply list all shortcuts instead of primary/alternate. Just "Shortcuts". I'm not sure yet how i'm going to display that.
Lets for a second assume that i'm going to clean this up for frameworks (quite likely). Would we deprecate the primary/alternate stuff? If so, where would we deprecate it? In KDE 4.9? 4.10?</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;">Frameworks is 5.0.
I think we wouldn't deprecate individual methods. Ideally we would deprecate all of KShortcut, if we can ensure that QShortcut (or rather QAction and its list of QKeySequences, it seems?) has everything we need. Again, let's discuss that on k-f-d rather than here ;)</pre>
<br />
<p>- David</p>
<br />
<p>On May 9th, 2012, 6:21 p.m., Mark Gaiser wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Mark Gaiser.</div>
<p style="color: grey;"><i>Updated May 9, 2012, 6:21 p.m.</i></p>
<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;">So i was trying to fix this bug: https://bugs.kde.org/show_bug.cgi?id=181531 That only asked for one more shortcut. That issue seems to be a little more complicated than it looks. Till this point KActions could only have a "Primary" and a "Alternate" shortcut. 2 in total which is - in some situations - not enough.
I fixed this by roughly restructuring nearly all of the KShortcut cpp file.
The only thing i'm not quite sure about is how "KShortcut KAction::shortcut(ShortcutTypes type) const" looks right now.. If anyone has some clarification on that one..?</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">I tested this by adding the missing bindings for Dolphin's back/forward and it seems to be working just fine. I can use all available shortcuts.</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>kdeui/actions/kaction.h <span style="color: grey">(d877554)</span></li>
<li>kdeui/actions/kaction.cpp <span style="color: grey">(309cf82)</span></li>
<li>kdeui/shortcuts/kshortcut.h <span style="color: grey">(c720830)</span></li>
<li>kdeui/shortcuts/kshortcut.cpp <span style="color: grey">(e307ab0)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104801/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>