<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/112590/">http://git.reviewboard.kde.org/r/112590/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 8th, 2013, 9:04 p.m. UTC, <b>Andreas Pakulat</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/112590/diff/2/?file=188213#file188213line49" style="color: black; font-weight: bold; text-decoration: underline;">shell/completionsettings.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">CompletionLevel</span> <span class="n">level</span><span class="p">(</span><span class="n">m_level</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">47</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">static_cast</span><span class="o"><</span><span class="n">CompletionLevel</span><span class="o">></span><span class="p">(</span><span class="n">readIntConfig</span><span class="p">(</span><span class="s">"completionLevel"</span><span class="p">,</span> <span class="n">static_cast</span><span class="o"><</span><span class="kt">int</span><span class="o">></span><span class="p">(</span><span class="n">MinimalWhenAutomatic</span><span class="p">)));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You should not cast integers read from the config file like this. What happens if the config file contains an int that does not fall into the range used for the enum? IMO having the individual flags as separate boolean entries in the config is also more readable inside the config file. So that should be kept, which means there needs to be code added to actually change those boolean members in the ccpreferences class when another entry is selected.</pre>
</blockquote>
<p>On September 9th, 2013, 11:11 a.m. UTC, <b>Vlas Puhov</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 don't understand how can config file contain an int that does not fall into the range? It could only happen if you edit config file manually. But then you should know exactly what you're doing, otherwise you can break anything. And if something goes wrong YOU are the one who responsible for it. And the same goes for any setting, not just completionLevel. Anyway I don't think it's a good idea to change settings manually.
Moreover seems like this technique (int read from config casted to enum) used throughout kdevelop's code base, see e.g. ExternalScriptPlugin. But if you REALLY edit config files manually all the time and put it's readability above all, ok then I can do it. But again I find it very odd to have three separate entries for one setting (because it's not obvious that those values actually change the same setting).</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It can also happen when switching back and forth across versions for example when sharing some config files across different systems. Anyway, you do have a point in that these are probably rare cases. So how about compromising between 'readable' (reading config files does happen more often than changing manually IMO) and avoiding 3 extra fields: Store a text value instead of the integer. I'm not sure how KConfigSkeleton handles comboboxes, it may be possible to set Qt::DataRole for the entries and get that stored, which would be good as its an untranslated string that can be stored and retrieved without running into localization issues. So you could try wether adding a value for that and changing the field type from int to QString works already, maybe the docs have something on that or otherwise the KDE devel channels (irc, mailinglist) should be able to help.
I guess a configuration-update script is not needed here since these settings haven't been stored properly so far. So there's no need to upgrade existing configs to the new format.</pre>
<br />
<p>- Andreas</p>
<br />
<p>On September 9th, 2013, 11:07 a.m. UTC, Vlas Puhov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Vlas Puhov.</div>
<p style="color: grey;"><i>Updated Sept. 9, 2013, 11:07 a.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;">CCPreferences class is written with use of KCModule, so all setting can be saved/loaded automatically. For that widget's names should start with kcfg_ prefix, but neither todoMarkerWords nor completionDetail had one, so this feature didn't work.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=324510">324510</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>shell/settings/ccsettings.ui <span style="color: grey">(64d24e2)</span></li>
<li>shell/settings/ccpreferences.h <span style="color: grey">(4ecf623)</span></li>
<li>shell/settings/ccpreferences.cpp <span style="color: grey">(0347fc6)</span></li>
<li>shell/settings/ccconfig.kcfg <span style="color: grey">(1bc89b1)</span></li>
<li>shell/completionsettings.h <span style="color: grey">(b3f28ce)</span></li>
<li>shell/completionsettings.cpp <span style="color: grey">(6b6adf6)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112590/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>