Review Request 118743: Make every QML/JS file see the other QML/JS files in its directory

[Sven Brauch]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118743/#review60093
-----------------------------------------------------------


Good feature, but I'm not entirely happy with your approach yet -- see below ;)


duchain/declarationbuilder.cpp
<https://git.reviewboard.kde.org/r/118743/#comment41828>

    re QDirIterator: I wouldn't know what's wrong with using that in KDE code. I think the only guideline is that for user visible elements, e.g. line edits, you should use the K class over the Q class because of the KDE integration (clear button + different default text style in this case). Otherwise you're free to use everything from Qt.



duchain/declarationbuilder.cpp
<https://git.reviewboard.kde.org/r/118743/#comment41824>

    put that outside the loop, I think it will be faster that way because you spend less time passing the lock around



duchain/declarationbuilder.cpp
<https://git.reviewboard.kde.org/r/118743/#comment41825>

    Hm, this seems backward to me. I would prefer if you did it like "if a.qml is parsed, it looks for other files in the directory and imports them."
    
    For example, if you open all the files initially, will highlighting be correct (i.e. include highlighting for stuff from imported contexts) if they are parsed in the wrong order? I don't tihnk so, because you don't tell it to update the highlighting anywhere.



duchain/parsesession.cpp
<https://git.reviewboard.kde.org/r/118743/#comment41826>

    I dislike this method's name, it doesn't sound like it does something ...



duchain/parsesession.cpp
<https://git.reviewboard.kde.org/r/118743/#comment41827>

    keep the lock while you access the context


- Sven Brauch


On June 14, 2014, 1 p.m., Denis Steckelmacher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118743/
> -----------------------------------------------------------
> 
> (Updated June 14, 2014, 1 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-qmljs
> 
> 
> Description
> -------
> 
> If a directory contains a.js, b.js and c.js, a.js imports b.js and c.js, b.js imports a.js and c.js, and c.js imports a.js and b.js. This way, every file can see the declarations of all the other files. This brings two important features:
> 
> * The same code is used for QML files. Because this patch also declares the top-level component of a MyComponent.qml QML file as MyComponent, QML files can now instantiate the top-level component of other files. This is the "custom components" feature.
> * Javascript files generally don't contain import statements, and it is difficult to know which declarations should be visible in a file. With this patch, every file can see the others in the same directory. This is already quite useful because the majority of the Javascript developers put all their files in the same directory (mysite.js uses the jquery.js file stored along it).
> * QML files can see JS files and the other way around. This may be useful if an user writes a JS files designed to be used by a QML file (I think it is possible to share code between the QML and the Javascript worlds) because the QML components will be visible in the JS files (that can instantiate them), and the JS functions/objects can be used in QML slots.
> 
> This patch is a bit simpler than what I've imagined, but here are some remarks that I have about the code:
> 
> * I use QFileInfo because I've not found a static method that takes a file path and returns the path of its parent directory (a simple section('/', 0, -2) would have been enough if every operating system used / separators). QDirIterator is easy to use, but is it okay to use these classes in KDE code?
> * Every file includes every other file in the same directory. When there are many files in a directory, this can become quite huge, but KDevelop seems to handle imported parent contexts in a quite efficient manner.
> * I filter out the files in /kdevqmljssupport/, because they are big and numerous. I don't want all 24 of them to include all the others and to be parsed every time KDevelop starts.
> * The "class" of the top-level QML component of a file is not anonymous anymore but has a name, hence the small changes in the unit tests.
> 
> 
> Diffs
> -----
> 
>   codecompletion/tests/qmlcompletiontest.cpp 7e32a16 
>   duchain/declarationbuilder.h 2744ffe 
>   duchain/declarationbuilder.cpp 5a9b830 
>   duchain/parsesession.h 8afc145 
>   duchain/parsesession.cpp 3133c80 
>   duchain/tests/testdeclarations.cpp 51b90a5 
>   tests/files/plugins.qml 9b1c0d4 
>   tests/files/test.qml 4b08b47 
>   tests/files/types.js 2428b89 
>   tests/files/uses.js bfbd6fc 
> 
> Diff: https://git.reviewboard.kde.org/r/118743/diff/
> 
> 
> Testing
> -------
> 
> Two tests are added: one declares a JS variable in a file and uses it in another file (and checks that the use count is 1), the other instantiates the top-level QML component of "test.qml" in "plugins.qml" and checks that the uses are okay.
> 
> Some other unit tests needed to be changed because top-level QML components now have a class name. With these changes, the whole testsuite continues to pass and has not been slowed down by this patch.
> 
> 
> Thanks,
> 
> Denis Steckelmacher
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140614/4ee6a489/attachment-0001.html>