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

Milian Wolff mail at milianw.de
Sun Feb 23 13:38:18 UTC 2014 

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



languages/cpp/includepathcomputer.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35577>

    just a note: see above, "computeForeground" -> this is only called from the GUI thread. No need for the mutex.



languages/cpp/includepathcomputer.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35578>

    can't you do something like m_defines = buildManager->defines(file) + manager->defines(file) or similar? Is the manual loop really required?



languages/plugins/custom-definesandincludes/customdefinesandincludesmanager.h
<https://git.reviewboard.kde.org/r/115696/#comment35579>

    this should use the current year and your name + email



languages/plugins/custom-definesandincludes/customdefinesandincludesmanager.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35580>

    same



languages/plugins/custom-definesandincludes/debugarea.h
<https://git.reviewboard.kde.org/r/115696/#comment35581>

    same



languages/plugins/custom-definesandincludes/debugarea.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35582>

    same



languages/plugins/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35584>

    please add a comment, noting that the model should be ported to use the Path api directly, which hopefully gets rid of this 



languages/plugins/custom-definesandincludes/settingsmanager.h
<https://git.reviewboard.kde.org/r/115696/#comment35583>

    why is this exported? remove this - there should be no need for this, or?



languages/plugins/custom-definesandincludes/tests/plugintest.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35586>

    comparing with the test case below, I'd extract most of the project loading into a separate function and use that.
    
    Also, you should probably add a cleanup() function. That is called automatically after each test and should close all projects.



languages/plugins/custom-definesandincludes/tests/plugintest.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35585>

    even 5s should suffice here I think



projectmanagers/custom-buildsystem/custombuildsystemconfigwidget.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35587>

    if the project argument is not used anymore, please remove it from the argument list


- Milian Wolff


On Feb. 22, 2014, 5: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, 5: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/20140223/9ab8158b/attachment-0001.html>

More information about the KDevelop-devel mailing list