<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/109019/">http://git.reviewboard.kde.org/r/109019/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There's been a small misunderstanding I think. You introduced a slot which keep emitting actionGlobalShortcutChanged for actions alright *but* you introduced it in the wrong class. This slot should be in KAction and the connect for KGlobalAccel signal be done in KAction constructor, this way it is completely transparent to KGlobalAccel what KAction is doing.
Otherwise that's inserting an extra KGlobalAccel -> KAction dependency which will be in the way of later having a completely QAction based KGlobalAccel.</pre>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109019/diff/5/?file=116034#file116034line357" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/shortcuts/kglobalaccel.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">private:</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">357</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_PRIVATE_SLOT</span><span class="p">(</span><span class="n">d</span><span class="p">,</span> <span class="kt">void</span> <span class="n">_k_emitActionGlobalShortcutChanged</span><span class="p">(</span><span class="n">KAction</span><span class="o">*</span><span class="p">,</span> <span class="k">const</span> <span class="n">QKeySequence</span><span class="o">&</span><span class="p">))</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This should go in kaction.h</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109019/diff/5/?file=116035#file116035line126" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/shortcuts/kglobalaccel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">KGlobalAccelPrivate::KGlobalAccelPrivate(KGlobalAccel *q)</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">126</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">q</span><span class="o">-></span><span class="n">connect</span><span class="p">(</span><span class="n">q</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">globalShortcutChanged</span><span class="p">(</span><span class="n">KAction</span><span class="o">*</span><span class="p">,</span><span class="k">const</span> <span class="n">QKeySequence</span><span class="o">&</span><span class="p">)),</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This should go in KAction ctor.</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109019/diff/5/?file=116035#file116035line481" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/shortcuts/kglobalaccel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<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">480</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">KGlobalAccelPrivate</span><span class="o">::</span><span class="n">_k_emitActionGlobalShortcutChanged</span><span class="p">(</span><span class="n">KAction</span><span class="o">*</span> <span class="n">action</span><span class="p">,</span> <span class="k">const</span> <span class="n">QKeySequence</span><span class="o">&</span> <span class="n">seq</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It should go in kaction.cpp. Of course it will become KAction::_k_emitActionGlobalShortcutChanged and the emit will occur only if action == this.</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109019/diff/5/?file=116036#file116036line75" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/shortcuts/kglobalaccel_p.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">74</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">_k_emitActionGlobalShortcutChanged</span><span class="p">(</span><span class="n">KAction</span><span class="o">*</span> <span class="n">action</span><span class="p">,</span> <span class="k">const</span> <span class="n">QKeySequence</span><span class="o">&</span> <span class="n">seq</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It should go in kaction_p.h.</pre>
</div>
<br />
<p>- Kevin</p>
<br />
<p>On February 26th, 2013, 7:28 p.m. UTC, Valentin Rusu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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, David Faure and Kevin Ottens.</div>
<div>By Valentin Rusu.</div>
<p style="color: grey;"><i>Updated Feb. 26, 2013, 7:28 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;">KDELibs cleanup task: move global shortcut facilities from KAction to KGlobalAccel. The second step, registering QActions instead of KActions, will be done after completing this review.</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;">Code compiles</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/shortcuts/kglobalaccel_p.h <span style="color: grey">(04feaba)</span></li>
<li>kdeui/shortcuts/kglobalaccel.cpp <span style="color: grey">(36dbab7)</span></li>
<li>kdeui/actions/kaction_p.h <span style="color: grey">(b50e678)</span></li>
<li>kdeui/shortcuts/kglobalaccel.h <span style="color: grey">(ed68bba)</span></li>
<li>kdeui/actions/kaction.cpp <span style="color: grey">(ec63a72)</span></li>
<li>KDE5PORTING.html <span style="color: grey">(e1b41d1)</span></li>
<li>kdeui/actions/kaction.h <span style="color: grey">(ddaa4de)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109019/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>