<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="https://git.reviewboard.kde.org/r/118575/">https://git.reviewboard.kde.org/r/118575/</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;">One important thing to check is to let it parse something and then restart KDevelop a few times and watch out for crashes or weird behaviour. You won't notice itemrepository-related bugs so easily in a test or a single run. If that works, I'd say what you have looks ok ;)
I'm not fully convinced whether overriding the behaviour of the well-known functions is the nicest way to do this. Couldn't you provide a new function, say, visibleDeclarations(...) and call that instead? Or are there other disadvantages to doing that?</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="https://git.reviewboard.kde.org/r/118575/diff/2/?file=279235#file279235line38" style="color: black; font-weight: bold; text-decoration: underline;">duchain/qmlducontext.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">38</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * QML has a strange concept of visbility (nested components cannot see their</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;">maybe that docstring fits better with the method than with the class</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="https://git.reviewboard.kde.org/r/118575/diff/2/?file=279235#file279235line56" style="color: black; font-weight: bold; text-decoration: underline;">duchain/qmlducontext.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">56</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">virtual</span> <span class="kt">void</span> <span class="n">mergeDeclarationsInternal</span><span class="p">(</span><span class="n">QList</span><span class="o"><</span><span class="n">QPair</span><span class="o"><</span><span class="n">KDevelop</span><span class="o">::</span><span class="n">Declaration</span><span class="o">*</span><span class="p">,</span> <span class="kt">int</span><span class="o">>>&</span> <span class="n">definitions</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 function needs a docstring, mentioning it's overriden and for what reason ;)</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="https://git.reviewboard.kde.org/r/118575/diff/2/?file=279236#file279236line40" style="color: black; font-weight: bold; text-decoration: underline;">duchain/qmlducontext.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d_func_dynamic</span><span class="p">()</span><span class="o">-></span><span class="n">setClassId</span><span class="p">(</span><span class="k">this</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;">what's this for? maybe you can put a comment?</pre>
</div>
<br />
<p>- Sven Brauch</p>
<br />
<p>On June 5th, 2014, 9:15 p.m. UTC, Denis Steckelmacher wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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.</div>
<div>By Denis Steckelmacher.</div>
<p style="color: grey;"><i>Updated June 5, 2014, 9:15 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-qmljs
</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;">QML has very strange visibility rules. Here is an example:
import QtQuick.Controls 1.1
Label {
id: label
text: "Hello"
Button {
id: button
onClick: { label.hello = "World"; }
}
}
"button" is an object that is declared inside label, and thus has an inner context that is a child of the context of "label". However, "button" cannot see the "text" property of "label", even if this property is declared in a parent context. "button" can see "label", though, as "label" is declared in the top-level context (using Declaration::setContext, this was already the case without this patch).
The inner context of a QML object therefore must be able to see the top-level context, its imported parent contexts (class inheritance), but not its parent context. This patch handles that by introducing QmlDUContext, a subclass of DUContext. QmlDUContext provides a method that allows DeclarationBuilder to specify that a context should be blind to its parent (but not the top-level context, hence the addImportedParentContext(topContext()) call).</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;">Two new unit tests have been added. The first one ensures that the code-completion doesn't list symbols that should not be visible, and the second one checks that UseBuilder does not see uses where there shouldn't be any.
All the other unit tests still pass, but I'm not very confident of the implementation of QmlDUContext. There were many things to take into consideration, many constructors to implement, a macro that mustn't be forgotten, special setClassId calls, etc. I hope that I haven't missed anything, but I'm not sure (valgrind doesn't complain about anything, though, but I'm afraid of subtle cache-related bugs). No other language support plugin that I know (kdev-clang, kdev-oldcpp, kdev-python, kdev-ruby) subclasses DUContextData (they subclass DUContext, though), so I had no example.
</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>codecompletion/tests/qmlcompletiontest.cpp <span style="color: grey">(c029ea9)</span></li>
<li>duchain/CMakeLists.txt <span style="color: grey">(b02a257)</span></li>
<li>duchain/declarationbuilder.h <span style="color: grey">(1e6c895)</span></li>
<li>duchain/declarationbuilder.cpp <span style="color: grey">(ede6407)</span></li>
<li>duchain/qmlducontext.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>duchain/qmlducontext.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/files/test.qml <span style="color: grey">(cbe2fa6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118575/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>