Review Request 118718: Add settings dialog to add additional compilers.

[Sergey Kalinichev]

> On June 19, 2014, 1:47 p.m., Milian Wolff wrote:
> > languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp, line 253
> > <https://git.reviewboard.kde.org/r/118718/diff/2/?file=280795#file280795line253>
> >
> >     that looks odd - is this to generate some default compiler? then please add a new function for that with a proper name. This pattern is used in a few places in this patch and its always strange and I wonder what it should do :)
> >     
> >     generally, some more comments would help so that one directly sees what a given loop will do. for this one e.g. something like "reset project compiler to default"
> >     
> >     why is the last factory being used, btw? shouldn't the factory of the compiler that is unregistered be used?

>that looks odd - is this to generate some default compiler?

You mean createCompiler("", "")? It creates a compiler with an empty name and path. This is used in 2 situations: when user adds a new compiler (he'll set the name and the path afterward) and when we create the DummyCompiler that knows it's name.

> then please add a new function for that with a proper name
Why? The code imo is already very straightforward... Anyhow, what exactly this function should look like?

>why is the last factory being used, btw? 
Because in that implementation the last factory was the the DummyFactory. But now I use a proper function to retrieve it.


> On June 19, 2014, 1:47 p.m., Milian Wolff wrote:
> > languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.cpp, line 68
> > <https://git.reviewboard.kde.org/r/118718/diff/2/?file=280807#file280807line68>
> >
> >     this should never happen, or?

This happens with the DummyFactory


> On June 19, 2014, 1:47 p.m., Milian Wolff wrote:
> > languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp, line 278
> > <https://git.reviewboard.kde.org/r/118718/diff/2/?file=280812#file280812line278>
> >
> >     this and the loop below essentially does a semantic compilerProvider->setCompilers(cw.compilers())
> >     
> >     and that's how I would expect it to be called from here.
> >

Sadly, but the ICompilerProvider interface doesn't have the setCompilers method, and I'm reluctand to add it, as I don't want to pollute the interface with rarely used methods that can be implemented with register/unregister calls instead


- Sergey


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


On June 13, 2014, 11:37 a.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118718/
> -----------------------------------------------------------
> 
> (Updated June 13, 2014, 11:37 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> Now there are compilers that auto-detected and those that added manually. The latter exist globally and can be used for any project.
> 
> 
> Diffs
> -----
> 
>   languages/plugins/custom-definesandincludes/compilerprovider/CMakeLists.txt 45cccbc 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.h 8ab52ab 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp 6483995 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.h b96df3b 
>   languages/plugins/custom-definesandincludes/compilerprovider/gcclikecompiler.cpp 97262ec 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompiler.h 77ceb75 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompilerfactory.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/compilerprovider/icompilerprovider.h 21bfc05 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.h e0ee668 
>   languages/plugins/custom-definesandincludes/compilerprovider/msvccompiler.cpp 1c6b6c5 
>   languages/plugins/custom-definesandincludes/kcm_widget/CMakeLists.txt ea0fe01 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilersmodel.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/compilerswidget.ui PRE-CREATION 
>   languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg 9acb2e0 
>   languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp c09b00e 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.h 3618170 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp c3a0823 
>   languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui 9831b94 
>   languages/plugins/custom-definesandincludes/settingsmanager.h f215268 
>   languages/plugins/custom-definesandincludes/settingsmanager.cpp fb7fd0f 
> 
> Diff: https://git.reviewboard.kde.org/r/118718/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> addCompiler.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/13/613c5d8c-6425-42ad-b0e4-1ccc2384a838__addCompiler.png
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140621/8dd7b32b/attachment.html>