<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 27th, 2013, 10:29 a.m. UTC, <b>David Nolden</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;">Since you create an assistant action for every single problem you encounter in the parser, I rather would recommend you _not_ to create a KAction unconditionally for each assistant action. As I know KDE, a KAction is probably very memory heavy.
I would rather recommend to remove the 'const' from the actions method, explicitly state that the actions should only be created on-demand in that method, and additionally add some assertion somewhere to make sure that the returned actions actually belong to the main thread.</pre>
</blockquote>
<p>On April 27th, 2013, 10:45 a.m. UTC, <b>Sven Brauch</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;">That sounds reasonable. However I would then move the logic of that "create actions the first time actions() is called" to the base class, i.e.
* add a (pure?) virtual createActions() method which should create the actions
* make actions() non-virtual and non-const, and make it call createActions() the first time it is called
Then it's obvious how to implement the interface. What do you think?</pre>
</blockquote>
<p>On April 27th, 2013, 11:45 a.m. UTC, <b>David Nolden</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;">Yes that sounds like a good way to do it.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah no -- probably not a problem anyways. Even if you create the actions right away, you just create instances of IAssistantAction, which are, don't know, 8 bytes or so. The KActions are only created when the widget is actually being shown in either case.</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>