<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/101520/">http://git.reviewboard.kde.org/r/101520/</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 7th, 2011, 4:28 p.m., <b>Michael Jansen</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;">Why don't you fix KKeyServer to return the correct results instead of fixing the wrong ones here?</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;">Yes, my first thought was to fix this by changing KKeyServer::keyQtToModX to add Shift to the modifiers if it is needed to get the symbol. This could be done using the (currently unused) function getModsRequired(uint sym) in kkeyserver_x11.cpp. But this is public API and I don't know what other users there are and what the expected behavior is, changing it to do add modifiers that were not there based on the behavior of some widget for entering shortcuts could be considered unexpected:)
Same goes for KKeyServer::xEventToQt(), that would need to be changed to strip Shift modifier in the cases where KKeySequenceWidget has removed it.
The positive thing about doing it this way would be that isShiftAsModifierAllowed() would not need to be exported, it could be in a private header as Aaron suggested in the other review request.</pre>
<br />
<p>- Simon</p>
<br />
<p>On June 6th, 2011, 11:02 a.m., Simon Persson 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 KDE Runtime and Michael Jansen.</div>
<div>By Simon Persson.</div>
<p style="color: grey;"><i>Updated June 6, 2011, 11:02 a.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;">Second patch to fix this bug, depends on patch in review request 101515.
KKeySequenceWidget used to enter shortcuts removes shift from the recorded shortcut if the symbol produced from that physical key is different when shift is used (upper/lowercase letters doesn't count). kglobalaccel needs to include shift in the grab in order to be triggered on this class of shortcuts, and then in the keypress event handler it also needs to strip the shift again before checking which shortcut was just triggered.</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;">Tested using the master branch, running in a Xephyr session using gb,us and se keyboard layouts. The tested layout needs to be the first entry in the list of layouts in the keyboard layout switcher, otherwise it will not work. Works for all the !"£$%^&*()_+ etc, no regression on shortcuts with only a number, only a letter or shift+letter. Also no regression on alt+shift+tab or ctrl+shift+esc.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=179504">179504</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=197548">197548</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=215030">215030</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kglobalaccel/kglobalaccel_x11.cpp <span style="color: grey">(2576d2e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101520/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>