Review Request 115696: Break Custom Buildsystem Plugin apart into 2 pieces.

Sergey Kalinichev kalinichev.so.0 at gmail.com
Sat Feb 22 17:03:20 UTC 2014 


> On Feb. 18, 2014, 1:40 a.m., Milian Wolff wrote:
> > Woha, big patch! Thanks for the effort you put into this feature, much appreciated. There are a few things I'd like to see changed, but all in all this looks very good already.
> > 
> > If we can then in the future move the make-resolve stuff into a second plugin which implements the interface I want to have added, then it can easily be disabled if wanted.

Thanks for the review, btw!!


> On Feb. 18, 2014, 1:40 a.m., Milian Wolff wrote:
> > projectmanagers/custom-definesandincludes/customdefinesandincludesmanager.cpp, line 93
> > <https://git.reviewboard.kde.org/r/115696/diff/1/?file=243514#file243514line93>
> >
> >     why this one? should/is this code threadsafe? it should not have to.

Well, ParseJobs use this code. AFAIK many parse jobs can be running at the same time, but there is only one instance of CustomDefinesAndIncludesManager, so...


> On Feb. 18, 2014, 1:40 a.m., Milian Wolff wrote:
> > projectmanagers/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp, line 176
> > <https://git.reviewboard.kde.org/r/115696/diff/1/?file=243530#file243530line176>
> >
> >     this should trigger new deprecation warnings for which I am to blame in some part.
> >     
> >     anyhow - please don't use the deprecated API. Prefer the KDevelop::Path API by doing project->path().toUrl().isParentOf(url)

Yes, I wanted to do it that way too, but that API return a slightly different result e.g. project->folder() returns /some/path/ while project->path().toUrl() /some/path (without /), and projectpathsmodel.cpp relies on trailing /. But you're right anyway, so I've workarounded it.
BTW is it desired behavior or just a project's API bug?


- Sergey


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


On Feb. 22, 2014, 9:02 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115696/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2014, 9:02 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Bugs: 254662 and 329788
>     http://bugs.kde.org/show_bug.cgi?id=254662
>     http://bugs.kde.org/show_bug.cgi?id=329788
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> The features are:
> 1. It's possible to add includes/defines for files/directories from withing a project.
> 2. Data stored in Custom Build Manager's format.
> 3. Language plugins use IDefinesAndIncludes interface to retrieve custom includes/defines.
> 
> 
> There are questions though:
> 1. What to do with .kdev_include_paths files? Should we convert it's data to the new format upon project loading or we could just ignore it for projects and use it only for "out of project" files?
> 2. For some reasong this plugin shows up above others in the list of plugins (see screenshots), is it ok, if not how to change that behaviour?
> 3. Does other languages have defines/includes notion, if not how to make it available only for C/C++ then? (X-KDevelop-Language=C++ doesn't seem to work...)
> 
> Also there are some features missing:
> 1. So far there is no notification mechanism about changed defines/includes. (that is you have to press F5 to see the changes).
> 2. There is no caching (I wonder if it needed at all?)
> ...
> 
> 
> Diffs
> -----
> 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_kdevcustomdefinesandincludes.desktop PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathsmodel.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfgc PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/definesmodel.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/definesmodel.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/defineswidget.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/defineswidget.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/includesmodel.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/includesmodel.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/includeswidget.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/includeswidget.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/customdefinesandincludesmanager.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/debugarea.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/debugarea.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/CMakeLists.txt PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg PRE-CREATION 
>   languages/CMakeLists.txt 847426b 
>   languages/cpp/includepathcomputer.cpp 325ae55 
>   languages/plugins/CMakeLists.txt PRE-CREATION 
>   languages/plugins/custom-definesandincludes/CMakeLists.txt PRE-CREATION 
>   languages/plugins/custom-definesandincludes/customdefinesandincludesmanager.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kdevcustomdefinesandincludesmanager.desktop.cmake PRE-CREATION 
>   languages/plugins/custom-definesandincludes/settingsconverter.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/settingsconverter.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/settingsmanager.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/settingsmanager.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/CMakeLists.txt PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/plugintest.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/plugintest.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/multipathproject/.kdev4/multipathproject.kdev4 PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/multipathproject/config.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/multipathproject/multipathproject.kdev4 PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/multipathproject/src/main.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/simpleproject/.kdev4/simpleproject.kdev4 PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/simpleproject/config.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/simpleproject/simpleproject.kdev4 PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/projects/simpleproject/src/main.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/testconfig.h.in PRE-CREATION 
>   projectmanagers/custom-buildsystem/CMakeLists.txt e742fe4 
>   projectmanagers/custom-buildsystem/configconstants.h 1f25752 
>   projectmanagers/custom-buildsystem/configwidget.h 85dcb09 
>   projectmanagers/custom-buildsystem/configwidget.cpp 6a665a6 
>   projectmanagers/custom-buildsystem/configwidget.ui d2157d4 
>   projectmanagers/custom-buildsystem/custombuildsystemconfig.h d9844b2 
>   projectmanagers/custom-buildsystem/custombuildsystemconfigwidget.h 9f2efcf 
>   projectmanagers/custom-buildsystem/custombuildsystemconfigwidget.cpp 1c3f770 
>   projectmanagers/custom-buildsystem/custombuildsystemplugin.cpp 074a338 
>   projectmanagers/custom-buildsystem/tests/CMakeLists.txt 6332faf 
>   projectmanagers/custom-buildsystem/tests/custombuildsystemplugintest.cpp ea9e2f5 
>   projectmanagers/custom-buildsystem/tests/projects/builddirproject/.kdev4/builddirproject.kdev4 d3d1e52 
>   projectmanagers/custom-buildsystem/tests/projects/multipathproject/.kdev4/multipathproject.kdev4 f988fe6 
>   projectmanagers/custom-buildsystem/tests/projects/simpleproject/.kdev4/simpleproject.kdev4 c5a856a 
> 
> Diff: https://git.reviewboard.kde.org/r/115696/diff/
> 
> 
> Testing
> -------
> 
> Yes, modified/enhanced tests a little bit.
> 
> 
> File Attachments
> ----------------
> 
> custom_build_system_before.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/12/c7683483-a74f-4f84-a91d-127af710dab4__custom_build_system_before.png
> custom_build_system_after.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/12/f4473409-be37-435a-a600-4a0159e6dd96__custom_build_system_after.png
> custom_defines_and_includes.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/12/e49fb372-dd4e-4715-bfa4-5dac59caf0be__custom_defines_and_includes.png
> interface_in_kdevplatform
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/22/eabc7944-30c4-44f4-a0e9-99435c5dc2f9__interface_in_kdevplatform
> full patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/22/bf556239-493b-47c5-9610-11be03cb747d__diff_full
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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

More information about the KDevelop-devel mailing list