<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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If there's no more complaints, then I'd submit this, I think it's okay. I'll adjust C++ before, of course.</pre>
<br />
<p>- Sven</p>
<br />
<p>On April 27th, 2013, 1:34 p.m. UTC, Sven Brauch 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 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;">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> </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>