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

Sat Mar 1 17:32:36 UTC 2014

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

Cool, much better but there are still some issues. I'd appreciate if you could attend to them as well.

Thanks again for working on this! Awesome work!

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

    do you need all these includes here? they look as if they are included already by the interfaces you implement here and forward-definitions are fine here as well

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

    unrequired

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

    why is this exported? this seems wrong to me

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

    this and below: many things are already included in header, please clean this up and minimize the include statements

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

    I'd like to see this change to better follow our existing code patterns. why do you want to make this a singleton?

    just create the ManagerPrivate in the ctor and store it in the CDAIM class and call it from the functions below.

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

    please break overly long lines

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

    see above, refactor this please

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

    just move the code from the private class here

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

    same

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

    why are these functions virtual if you just delegate to them anyways?

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

    so is this the "auto include"? i.e. a file is chosen and then its included everywhere?

    ok, that does sound like a good idea after all. but please add a comment here

languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.h
<https://git.reviewboard.kde.org/r/115696/#comment36245>

    this looks broken

languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.h
<https://git.reviewboard.kde.org/r/115696/#comment36246>

    indent please

languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.h
<https://git.reviewboard.kde.org/r/115696/#comment36247>

    here and below: override

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

    remove?

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

    put into anon namespace please for internal linkage

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

    here and below:

    const QString definesKey = QLatin1String("Defines");

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

    you modify the path here but never do anything with it then - whats this about? do you wanted to modify cPath?

    generally, can you explain this loop with a comment so I can understand what you are trying to do? the repeated loop looks pretty wrong I'd say, at the least its probably more code than required

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

    this looks like it could share code with SettingsConverter::readSettings no?

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

    broken

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

    indent please

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

    broken

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

    ProjectType

    also uppercase the leading chars of the enumerators please

languages/plugins/custom-definesandincludes/tests/projects/simpleproject/.kdev4/simpleproject.kdev4
<https://git.reviewboard.kde.org/r/115696/#comment36254>

    this and the other files: do we actually need to have them in the source directory? could we  maybe generate them in the tests (in a temporary folder)?

    the contents of the cpp files are also meaningless, no? or do you actually want them to be parsed?

- Milian Wolff

On Feb. 25, 2014, 6:35 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115696/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 6:35 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/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/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/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/includeswidget.ui PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.h PRE-CREATION 
>   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/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/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/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/kcm_custombuildsystem.cpp e05874b 
>   projectmanagers/custom-buildsystem/tests/CMakeLists.txt 6332faf 
>   projectmanagers/custom-buildsystem/tests/custombuildsystemplugintest.cpp ea9e2f5 
>   projectmanagers/custom-buildsystem/tests/kcmuitestmain.cpp b90d84f 
>   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/20140301/6b9ec76c/attachment-0001.html>