<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/110211/">http://git.reviewboard.kde.org/r/110211/</a>
</td>
</tr>
</table>
<br />
<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 KDevelop and Milian Wolff.</div>
<div>By Sven Brauch.</div>
<p style="color: grey;"><i>Updated April 27, 2013, 1:34 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">Hmm. More difficult than anticipated, because a whole herd of functions (in uicontroller, assistantpopup etc) wants actions() to be const.
So, we have two options: keep actions() const, with the argument that "data the object exposes publically is kept const by calling the function, so it's ok to have it const"; or remove the const to avoid using const_cast, and in turn remove a quite a few more consts in function arguments and other member functions. Attached is a patch for the first variant, but if you prefer the second, I can do that too.
Either way it's a huge improvement over the previous situation, because the const_casts are hidden in the base class implementation somewhere, and people implementing the interface (like me) are not required to use them but can instead implement the obvious createActions() method.</pre>
</td>
</tr>
</table>
<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;">When implementing a problem resolution assistant for kdev-python,
I noticed some very strange behaviour in the assistants API: actions
would only be executed if they were added to the assistant in the
actions() method (which is const, and should not create actions!),
while creating them in the constructor of the assistant made them
show up, but do nothing when triggered. The root of the problem turned
out to be that the assistants are instantiated in the parse job,
thus if you create the actions in the assistant's constructor, they
will live in the parse job thread, not in the application main thread;
the KAction's created from the assistant actions however would
live in the main thread, because the method to convert iAssistantAction
-> KAction is called from the main thread. Thus, connections wouldn't
work correctly, as the thread of the receiver is not running an event loop.
The existing code, as said, smartly™ works around this problem by adding
the actions to the assistant in the actions() method the first time
it is called, which happens from the main thread too. This sucks,
mainly because it cost me two hours to figure out why my actions wouldn't
be executed when I created them in the constructor, but also because
the API defines the function as const (which it should be), but the code
just ignores this (by using const_cast). The real fun thing is that you
*have* to do non-const things in a function defined by the API as const,
because there's no other way to make it actually work.
This patch moves an action to the main window's thread when it is created,
and also adds an assertion to make sure the connected objects reside
in the correct threads.
I'm not very well-informed about Qt's threading stuff (or any threading
stuff, really) yet, so this might well be the wrong approach to fix the
issue. If it is, please let me know and suggest a better alternative.
I see one possible issue: When creating the actions in the constructor, way
more actions than before will be created, increasing memory usage.
This might be relevant or not, I don't know, it's probably not; but
maybe someone can comment on that when reviewing.</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;">The natural way (adding actions in the constructor) now works. Before I push this, I will refactor the code in cppsupport to create its actions in the constructor, too, instead of this const_cast mess which is currently there.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>interfaces/iassistant.h <span style="color: grey">(2a65e45)</span></li>
<li>interfaces/iassistant.cpp <span style="color: grey">(a102f27)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110211/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>