<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/108212/">http://git.reviewboard.kde.org/r/108212/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 6th, 2013, 11:10 a.m., <b>Milian Wolff</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 missed the "why" for this patch set - can you elaborate please why this is required?</pre>
</blockquote>
<p>On January 6th, 2013, 12:08 p.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;">Reasons were following:
1. Every user of this class had to write the same code:
EnvironmentGroupList env( KGlobal::config() );
environment.addItems( env.groups() );
2. Nobody seemed to use EnvironmentGroupList::defaultGroup(), relying on "default" group being default - so, effectively, "Set group as default" in configuration KCM did nothing.
3. If KConfig value associated with the widget gets empty somehow, widget displays an empty line - which is not user-friendly.
4. Summarizing, EnvironmentSelectionWidget is a wrapper class that does nothing (well, apart from saving item text instead of item index to the KConfig).
Are these reasons ok?
</pre>
</blockquote>
<p>On January 6th, 2013, 2:06 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 don't have a strong opinion on any of this, except the storage. That should stay as it is and store the name of the environment group and not the index in the combobox. For the simple reason that the list is dynamic and there's no guarantee of order for the groups. Otherwise you'd have to store the index of each group along with the group and then store the current index which means adding unecessary bloat to the config files.</pre>
</blockquote>
<p>On January 6th, 2013, 2:14 p.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;">Of course, I did not change the storage. It stores the group name.</pre>
</blockquote>
<p>On January 6th, 2013, 2:50 p.m., <b>Milian Wolff</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;">The reasons sound good enough to me (please make sure to include them in your final commit).
Still, I do think you should refactor the selection widget to not inherit from KComboBox any longer.</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;">Sure. I just wanted to get greenlight on the idea itself before doing any additional work on it. :)</pre>
<br />
<p>- Ivan</p>
<br />
<p>On January 5th, 2013, 9:22 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.</div>
<div>By Ivan Shapovalov.</div>
<p style="color: grey;"><i>Updated Jan. 5, 2013, 9:22 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;">Feature-wise this gets rid of dependency on default KConfigXT value being "default" and allows explicitly specifying to use a default environment group whatever it will be.
Now an empty value shall be used by default, which has meaning of "use whatever is set by default at runtime".
Also, a special "Use default" line is now always added to the combo-box which maps to the empty config value, so user won't see empty entries in the combo-box.
</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;">Manual testing.</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>plugins/execute/nativeappconfig.cpp <span style="color: grey">(481d56c)</span></li>
<li>plugins/executescript/scriptappconfig.cpp <span style="color: grey">(b8f0729)</span></li>
<li>util/CMakeLists.txt <span style="color: grey">(29de126)</span></li>
<li>util/environmentconfigurebutton.cpp <span style="color: grey">(8303c70)</span></li>
<li>util/environmentselectionmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>util/environmentselectionmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>util/environmentselectionwidget.h <span style="color: grey">(0a69afc)</span></li>
<li>util/environmentselectionwidget.cpp <span style="color: grey">(63fdebc)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108212/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>