Review Request: Store CMake build system settings per build directory rather than globally (TODO in cmakepreferences.cpp)

[Andreas Pakulat]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105308/#review14925
-----------------------------------------------------------


I didn't fully look at the code, but I think the path taken is wrong. You're now storing half a dozen lists and require that entries in those lists are synchronized. This can be easily broken when someone fiddles with his config files or even by buggy code.

IMHO you should use config groups, one for each builddir and within each group you'd be able to again have simply key/value mappings. The group-names could be generated, something like "BuildDirConfig_<number>" and you can easily get all groups from the base KConfigGroup and then fetch those having the correct prefix.

I also don't think using kconfig-compiler has any benefit here, so the .kcfg-stuff can be dropped too. The main point for using that tool is to easily connect widgets in the UI and their values to certain entries in the config file. This only works for simple direct mapping of widget -> config key, which you don't have in this case.

- Andreas Pakulat


On June 20, 2012, 8:24 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105308/
> -----------------------------------------------------------
> 
> (Updated June 20, 2012, 8:24 p.m.)
> 
> 
> Review request for KDevelop and Aleix Pol Gonzalez.
> 
> 
> Description
> -------
> 
> This addresses the TODO at the beginning of cmakepreferences.cpp, making
> CMake build system settings saved in the project configuration file
> separately for each configured build directory.
> Specifically, 3 places have been changed:
> * cmakeconfig.kcfg: single URLs replaced with StringLists (and names of
> the parameters updated to reflect their semantics)
> * cmakeutils.cpp: functions changed to pick required fields from the lists
> * cmakepreferences.cpp: functions changed to work with lists + commented out
> the save() call on build directory creation since it is somewhat annoying.
> 
> URLs are saved to the StringLists via KUrl::url() except build directories'
> pathss themselves, which are stored as local file pathes (it was the old
> behavior).
> 
> Also, cmakeArguments() had been changed in cmakejob.cpp to make it use
> temporary variables instead of calling each fetch function twice.
> 
> ----
> 
> There is a possible regression - if no build directory is configured for a project, an error message is displayed instead of presenting a build directory creation dialog.
> Though a user can always go into the project settings and create the directory from there.
> 
> 
> Diffs
> -----
> 
>   projectbuilders/cmakebuilder/cmakejob.cpp af05577 
>   projectmanagers/cmake/cmakeconfig.kcfg 5ca623e 
>   projectmanagers/cmake/cmakeutils.cpp 3c9b736 
>   projectmanagers/cmake/settings/cmakepreferences.cpp 5be668b 
> 
> Diff: http://git.reviewboard.kde.org/r/105308/diff/
> 
> 
> Testing
> -------
> 
> Used two build directories on a project, one configured to build with MinGW via -DCMAKE_TOOLCHAIN_FILE.
> Ensured the settings are saved and applied correctly across build directory switch and KDevelop multiple restart.
> Existing projects' configurations are picked up correctly.
> 
> All unit-tests are also passed.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

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