<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/119208/">https://git.reviewboard.kde.org/r/119208/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 10th, 2014, 10:18 a.m. UTC, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This will mean you'll have to special-case these imports in the navigation windows though, no? There, you do want to say that it is a namespace (and not a class/type), or?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Furthermore, unrelated to this review request, could you explain me again why you create alias declarations instead of importing the context of the imported file into the namespace/class context?</p></pre>
</blockquote>
<p>On July 10th, 2014, 10:51 a.m. UTC, <b>Denis Steckelmacher</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Even if the type of the context is set to Class, the kind of the declaration remains Namespace. The code-completion items and the navigation widget therefore still show the proper icon and "namespace ...". Is there a place that may use the type of the context and that I should check?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding aliased declarations, I first tried to import the context of the file in the namespace, but then I discovered that I needed some sort of filtering. For instance, when importing a file, the namespace alias declarations of this file should not be imported (typing "MyFile." should offer completion items for every local declaration of MyFile, but not for "MyFile.QtQuick", "MyFile.MyOtherFile", etc).</p></pre>
</blockquote>
<p>On July 10th, 2014, 11:15 a.m. UTC, <b>Milian Wolff</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding navigation: ok, cool. if it works that sounds OK.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding alias declarations: we should rethink that and probably filter it in the code completion then, instead of spamming the duchain with alias declarations.</p></pre>
</blockquote>
<p>On July 10th, 2014, 11:32 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does "MyFile.QtQuick.Foo" work in theory? If yes, then I certainly agree with Milian that the filtering should take place in the completion list.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I remember having played with addImportedParentContext, namespaces and automatic imports, and I remember that using alias declarations was a last resort solution. Until just now, I did not remember what was the problem, though :-).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now I have remembered what it was: http://steckdenis.be/post-2014-06-17-qml-module-versions-and-automatic-imports.html (first title, fourth paragraph) :</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is a small technical difficulty here, though: if every file imports all the others, we get circular dependencies (A imports B and C, but B imports A and C, thus A can see itself by the way of B). The solution that I used is to “copy” the declarations of every file into the others instead of importing them. This way, A doesn’t import anything but contains the declarations of B, C, etc. B does the same, but uses the version of A without the declarations of B, C, etc copied. This way, A cannot see itself by the way of B, and B cannot see itself by the way of A.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Using clearer and more technical words, I have a problem when unnamed imports are used (the automatic import of the files in the same directory, and imports without an "as" clause). Let's take three files: A, B and C. Let's say that B and C are in the same directory, and that A contains an "import B as B" statement.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If addImportedParentContext is used, then the top-context of B will import the top-context of C (because they are in the same directory). The top-context of C will import the top-context of B, which will create a cyclic import (very difficult to solve, because both B and C should be able to see the declarations of the other file, but that could be worked around using namespaces and {...import...} namespace aliases). This is the first problem.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now, A imports B, because A is interested in the declarations of B. If A creates a namespace named B (because there was an "as B" clause in the import), and imports the top-context of B in this namespace, then every declaration visible from the top-context of B will be available in the namespace B. The problem is that B sees the declaration of C, so the declarations of C become visible in the namespace "B", that should only contain the declarations of B. This is the second problem.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Using alias declarations, I can copy only the local declarations of the context I import. This avoids circular imports (A imports the declarations of B, no matter which declarations B can have imported, because aliased declarations are filtered out), and ensures that importing a file only imports the file and not everything else it may have imported itself.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe a better solution exists for these two problems. I'll think about it, but the solution used depends on the importance you give to concise code (alias declarations require less than 10 lines of code and work well) and clean DUChain (alias declarations fill the DUChain, and using nested contexts or namespace aliases could simplify the DUChain at the expense of code complexity).</p></pre>
<br />
<p>- Denis</p>
<br />
<p>On July 10th, 2014, 10:04 a.m. UTC, Denis Steckelmacher 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 KDevelop.</div>
<div>By Denis Steckelmacher.</div>
<p style="color: grey;"><i>Updated July 10, 2014, 10:04 a.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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KDevPlatform sometimes special-cases contexts having the Namespace type. This was not a problem until I discovered that this QML snippet does not work:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">import</span> <span style="color: #BA2121">"myfile.js"</span> as MyFile
Item {
onTest<span style="color: #666666">:</span> console.log(MyFile.myVariable)
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In order to better see which contexts are used, here is the same snippet in a C++-like language:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">namespace</span> MyFile {
alias myVariable <span style="color: #666666">=</span> <span style="color: #BA2121">"myVariable in myfile.js"</span>;
}
<span style="color: #008000; font-weight: bold">class</span> <span style="color: #0000FF; font-weight: bold">__</span> <span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">public</span> Item {
<span style="color: #B00040">void</span> onTest() {
console.log(MyFile<span style="color: #666666">::</span>myVariable);
}
};
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The line starting with "onTest" calls findDeclarations(myVariable) on MyFile, that is a namespace (and has a namespace context). myVariable is a local declaration of MyFile, but is not returned by findDeclarations because an if statement skips findLocalDeclarations for Namespace contexts. A comment in language/duchain/ducontext.cpp says that declarations in namespace contexts are resolved in the top-context, but this cannot happen in my case because I set the DontSearchInParent flag when looking for object members.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch work-arounds this issue by giving a Class type to the internal contexts of namespace declarations. This way, KDevPlatform does not special-case the contexts and works as expected.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The whole testsuite passes. The feature shown above (importing Javascript files) is not added by this patch, but is made to work with this patch applied.</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>duchain/declarationbuilder.cpp <span style="color: grey">(8927633)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119208/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>