<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/122981/">https://git.reviewboard.kde.org/r/122981/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On març 22nd, 2015, 3:56 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/122981/diff/2/?file=356138#file356138line260" style="color: black; font-weight: bold; text-decoration: underline;">src/kglobalaccel.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">260</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * follwing internal settings will be used (see also implementation of</span></pre></td>
</tr>
</tbody>
</table>
<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;">typo in following</p></pre>
</blockquote>
<p>On març 22nd, 2015, 4:12 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;">About why it is needed: see KGlobalShortcutTest::testLoadShortcutFromGlobalSettings: shortcut() does not behave as expected when the new method loadShortcutFromGlobalSettings() is not called</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't know if always calling loadShortcutFromGlobalSettings() before shortcut() would break existing code. E.g. in kwin/effects/desktopgrid/desktopgrid_config.cpp shortcut() is used without prior call of loadShortcutFromGlobalSettings().</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On the other hand, a call to loadShortcutFromGlobalSettings() might be harmless but I don't have a good overview of the KGlobalAccel usage patterns.</p></pre>
</blockquote>
<p>On març 22nd, 2015, 4:24 p.m. UTC, <b>Albert Astals Cid</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;">Honestly i have no idea on how this kglobalaccel thing works, but it seems to me you're just adding a way to workaround a bug instead of fixing it.</p></pre>
</blockquote>
<p>On març 22nd, 2015, 4:39 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;">Me neither :-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I was hoping that someone with more experience with kglobalaccel can say more about the proposed solution.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Like you Thomas suggested to put the loading method into shortcut() (which is essentially a one-liner: d->updateGlobalShortcut(action, KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);).</p></pre>
</blockquote>
<p>On març 23rd, 2015, 7:56 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;">well we could try it: add the one-line change and try whether the unit tests pass and then try whether the case in DesktopGridConfig works.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unfortunately I'm also not that into KGloalAccel code yet to be 100 % whether it's a valid change :-(</p></pre>
</blockquote>
<p>On abril 2nd, 2015, 2:52 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;">I tried to call loadShortcutFromGlobalSettings() from within shortcut() and found that this violates const constraints:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">shortcut() is const
d->updateGlobalShortcut is not const</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So this is not possible.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How to proceed now?</p></pre>
</blockquote>
<p>On abril 3rd, 2015, 12:25 p.m. UTC, <b>Albert Astals Cid</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;">make mutable the members that updateGlobalShortcut modifies</p></pre>
</blockquote>
<p>On abril 3rd, 2015, 6:04 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;">I tried again and the call was even possible with only introducing some more consts. But the call of loadShortcutFromGlobalSettings() from within shortcut() makes more unit tests fail than without.
=> the general call of loadShortcutFromGlobalSettings() from within shortcut() seems to harm more than it helps.
Currently, I have a problem with getting the unit tests to all pass (which is independet of this change; it happens also in master branch). See my analysis below and comment of the unit test itself:
/* These tests could be better. They don't include actually triggering actions,
and we just choose very improbable shortcuts to avoid conflicts with real
applications' shortcuts. */
It would be nice to document some instructions of how to reset the global shortcuts to a defined state.</pre>
</blockquote>
<p>On abril 3rd, 2015, 8:49 p.m. UTC, <b>Albert Astals Cid</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;">Use strace to see which files are being read.</p></pre>
</blockquote>
<p>On abril 4th, 2015, 8:48 a.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;">I did</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%">strace -o strace1.txt ./kglobalshortcuttest testFindActionByKey
sed -n '/open/p' strace1.txt > strace1.open.txt
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is the complete output http://paste.opensuse.org/97828 and filtered by everything that contains "open": http://paste.opensuse.org/54497725.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">List all filenames:
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">sed -n 's/open("\(.*\)".*/\1/p' strace1.open.txt > strace1.files.txt</code></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Remove duplicates:
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">sort strace1.files.txt | uniq -u > strace1.files.nodups.txt</code></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Manually remove /dev/urandom from that list.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Grep for testLoadShortcutFromGlobalSettings in the list.</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%">for f in `cat strace1.files.nodups.txt`; do grep testLoadShortcutFromGlobalSettings $<span style="color: #19177C">f</span>; done
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No result.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The shortcuts seem to be retrieved via DBUS. Is this covered by strace as well?</p></pre>
</blockquote>
<p>On abril 4th, 2015, 8:35 p.m. UTC, <b>Albert Astals Cid</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;">No, strace doesn't cover dbus.</p></pre>
</blockquote>
<p>On abril 4th, 2015, 8:38 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;">But "dbus-monitor" does ;-)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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, i'll make it easy, </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">rm ~/.config/kglobalshortcutsrc</p></pre>
<br />
<p>- Albert</p>
<br />
<p>On abril 3rd, 2015, 5:50 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 Frameworks, Martin Gräßlin and Thomas Lübking.</div>
<div>By Gregor Mi.</div>
<p style="color: grey;"><i>Updated abr. 3, 2015, 5:50 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In some cases you need to call loadShortcutFromGlobalSettings() in order not to get a an empty list when calling shortcut() (which is const).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">See discussion in https://git.reviewboard.kde.org/r/122249/ ("libksysguard: add Kill Window to End Process button and show correct keyboard shortcut").</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>autotests/kglobalshortcuttest.h <span style="color: grey">(b1122a8f5ca2f3f7afbe78f8edba87325426c1a6)</span></li>
<li>autotests/kglobalshortcuttest.cpp <span style="color: grey">(3b661bbb612807a3bbbe34835d4ae712c2ec74da)</span></li>
<li>src/kglobalaccel.h <span style="color: grey">(3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16)</span></li>
<li>src/kglobalaccel.cpp <span style="color: grey">(1b6b3f5cb6d42401d684e6a491d12a6e57248fd1)</span></li>
<li>src/kglobalaccel_p.h <span style="color: grey">(eca7c52378ad60d0d5806561214b9788dd46a11e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122981/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>