<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105308/">http://git.reviewboard.kde.org/r/105308/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 20th, 2012, 10:03 p.m., <b>Andreas Pakulat</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On June 21st, 2012, 10:23 a.m., <b>Ivan Shapovalov</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If kconfig can be dropped, then multiple-groups are the preferred way, of course.
But then - some special actions should be taken for migration...
I'll try to implement that in a few days (just now I have some other projects to do).</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I didn't say drop kconfig. Careful here, kconfig would still be used, but kconfig-compiler and hence the .kcfg/.kcfgc files would be dropped.
Yes this requires some effort at recognizing old files and auto-converting them to the new format. It might even be possible to re-use the kconfig-update mechanism that KDE supplies for the actual configuration files in $HOME/.kde/share/config, so it would be done by a small script upon opening the project.</pre>
<br />
<p>- Andreas</p>
<br />
<p>On June 20th, 2012, 8:24 p.m., Ivan Shapovalov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop and Aleix Pol Gonzalez.</div>
<div>By Ivan Shapovalov.</div>
<p style="color: grey;"><i>Updated June 20, 2012, 8:24 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>projectbuilders/cmakebuilder/cmakejob.cpp <span style="color: grey">(af05577)</span></li>
<li>projectmanagers/cmake/cmakeconfig.kcfg <span style="color: grey">(5ca623e)</span></li>
<li>projectmanagers/cmake/cmakeutils.cpp <span style="color: grey">(3c9b736)</span></li>
<li>projectmanagers/cmake/settings/cmakepreferences.cpp <span style="color: grey">(5be668b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105308/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>