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

Milian Wolff mail at milianw.de
Mon Feb 17 21:40:55 UTC 2014 

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


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.


projectmanagers/custom-buildsystem/CMakeLists.txt
<https://git.reviewboard.kde.org/r/115696/#comment35217>

    I do not like that this is a library which must be linked. Rather, could you take the approach of writing an interface and then have plugin(s) implementing it thus providing the feature(s)?
    
    You could add that interface along with iprojectfilterprovider e.g. There you can also provide code which goes through the plugins and finds those that implement the new interface.



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

    remove this



projectmanagers/custom-definesandincludes/customdefinesandincludesmanager.h
<https://git.reviewboard.kde.org/r/115696/#comment35215>

    put this into the kdevelop namespace



projectmanagers/custom-definesandincludes/customdefinesandincludesmanager.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35216>

    why this one? should/is this code threadsafe? it should not have to.



projectmanagers/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg
<https://git.reviewboard.kde.org/r/115696/#comment35204>

    outdated comment :)



projectmanagers/custom-definesandincludes/kcm_widget/defineswidget.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35206>

    a custom debug area for this new plugin would be helpful and could be added with just a few lines of code. See e.g. plugins/projectfilter/projectfilterdebug.*



projectmanagers/custom-definesandincludes/kcm_widget/includeswidget.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35207>

    again, this could/should not be removed probably



projectmanagers/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35209>

    stray newline



projectmanagers/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35210>

    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)



projectmanagers/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35211>

    undo



projectmanagers/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35212>

    undo



projectmanagers/custom-definesandincludes/kcm_widget/projectpathswidget.cpp
<https://git.reviewboard.kde.org/r/115696/#comment35213>

    undo, see above


- Milian Wolff


On Feb. 12, 2014, 1:34 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115696/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2014, 1:34 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
> -------
> 
> So, here is the first approach.
> 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. Includes/defines come from build managers. I don't know maybe language plugins should retrieve this information directly, as e.g. current CPP plugin relies on it (if there is no includes from build manager it invokes MakeFiles fall back, but if there are it won't use that fall back mechanism then).
> 
> 
> 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. Where to place the new plugin? Currently I've placed it in the projectmanagers folder, but I'm thinking about moving it to e.g. languages/plugins dir as imo this plugin is not exactly a "project manager".
> 3. 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?
> 4. Does other languages have defines/includes notion, if not how to make it available only for C/C++ then?
> 
> 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?)
> 3. All IncludePath* stuff from current cpp plugin is not yet integrated into this one.
> ...
> 
> But I think that these features can/should be implemented in separate commits.
> 
> 
> Diffs
> -----
> 
>   projectmanagers/custom-definesandincludes/kcm_widget/defineswidget.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/defineswidget.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/includesmodel.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/includesmodel.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/includeswidget.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/includeswidget.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/kcm_kdevcustomdefinesandincludes.desktop PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/projectpathsmodel.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/projectpathsmodel.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/projectpathswidget.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/projectpathswidget.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/settingsconverter.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/settingsconverter.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/settingsmanager.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/settingsmanager.cpp PRE-CREATION 
>   projectmanagers/custommake/CMakeLists.txt 1731d00 
>   projectmanagers/custommake/custommakemanager.cpp 346c8cd 
>   projectmanagers/custom-definesandincludes/CMakeLists.txt PRE-CREATION 
>   projectmanagers/custom-definesandincludes/configentry.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/customdefinesandincludesmanager.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/customdefinesandincludesmanager.cpp PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/CMakeLists.txt PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfgc PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/definesmodel.h PRE-CREATION 
>   projectmanagers/custom-definesandincludes/kcm_widget/definesmodel.cpp PRE-CREATION 
>   projectmanagers/custom-buildsystem/configwidget.ui d2157d4 
>   projectmanagers/custom-buildsystem/custombuildsystemconfig.h d9844b2 
>   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 
>   projectmanagers/cmake/cmakemanager.cpp 7a34b37 
>   projectmanagers/cmake/CMakeLists.txt 63e23f4 
>   projectmanagers/CMakeLists.txt 8b78e1e 
>   projectmanagers/custom-buildsystem/configwidget.cpp 6a665a6 
>   projectmanagers/custom-buildsystem/configwidget.h 85dcb09 
>   projectmanagers/custom-buildsystem/configconstants.h 1f25752 
>   projectmanagers/custom-buildsystem/CMakeLists.txt e742fe4 
> 
> 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
> full_patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/12/80543139-50d8-45df-bac4-a3d1865d671c__full_patch
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

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

More information about the KDevelop-devel mailing list