<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/120025/">https://git.reviewboard.kde.org/r/120025/</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You wrote: "I have not yet completely grasped the concept of the default shortcuts and why they are set only explicitely via KActionCollection::setDefaultShortcuts."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think the concept is simple. QAction only knows "the current shortcuts, those that will trigger the action". Now imagine that the user configures the shortcut for the quit action (default Ctrl+Q) to use something else instead (Ctrl+E for exit, whatever). QAction will then only know about Ctrl+E, the actual active shortcut. But the shortcut configuration dialog needs to know that the default shortcut was Ctrl+Q, so that we can offer "revert to default", for instance (and to show that the shortcut was manually modified).<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This information API is in KActionCollection because, well, there's no KAction anymore, and it stores the default shortcut into the QAction as a dynamic property.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, this means another solution could be to just set the dynamic property in KStandardAction, without going through KActionCollection via invokeMethod. That would probably more robust,<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
including the case of creating an action without a collection as parent, and then putting it into a collection later on.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Good thing I wrote the explanation, it made me realize that there is a better solution :-)</p></pre>
<br />
<p>- David Faure</p>
<br />
<p>On August 31st, 2014, 4:40 p.m. UTC, Friedrich W. H. Kossebau 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.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated Aug. 31, 2014, 4:40 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=338222">338222</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfigwidgets
</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;">As e.g. reported in https://bugs.kde.org/show_bug.cgi?id=338222 ("False positive critical warnings for KStandardActions") currently KXMLGUIFactoryPrivate::saveDefaultActionProperties complains about lots of actions that have been created properly via KStandardActions with a KActionCollection as parent. Just grep the log of your favourite XMLGUI-based KF5-ported program to see yourself.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have not yet completely grasped the concept of the default shortcuts and why they are set only explicitely via KActionCollection::setDefaultShortcuts. But to me it makes some sense to have this automatically called for all standardactions which are created directly with a KActionCollection as parent.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I decided not to change KActionCollection::addAction because I had even less idea what all might be affected by that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So this is what this patch does:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> add a call to KActionCollection::setDefaultShortcuts if there is a standard shortcut (via QMetaObject::invokeMethod, like done for KActionCollection::addAction)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
</em> also move code which only should be done in case of a created action into one, same branch</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Needs https://git.reviewboard.kde.org/r/120024/ to make KActionCollection::setDefaultShortcuts() invokable.</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>src/kstandardaction.cpp <span style="color: grey">(a18527b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120025/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>