<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/105427/">http://git.reviewboard.kde.org/r/105427/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 3rd, 2012, 10:59 p.m., <b>Christoph Feck</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;">Thanks for the patch :) Unfortunately, it is too late for 4.9, so you should target 4.10 (master branch).
Also, I am not so sure the dedicated configuration is really needed, but nice to have.
Some remarks in the review below.</pre>
</blockquote>
<p>On July 4th, 2012, 7:31 a.m., <b>Michael Skiba</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;">Thank you for the quick review :-)
This is my very first codepatch for a KDE bundled application (and my first usage of the reviewboard), your input is very much appreciated :)
I had the idea to make it a user adjustable setting when the feature requester suggested grouping binary numbers by 8, representing one byte, which is very reasonable, while I found it much easier to check/copy binary numbers grouped by 4. Also he suggested grouping hex numbers by 4, which is perfectly reasonable as well, but my favorite hex editor groups them by two. So I thought if in doubt (and there are no official rules for it) make it flexible, so everyone can choose their own personal favorite. Does this approach bloat the settings menu too much or is it ok? (as I said most of what I've done so far were textbook examples which print something in a commandline ;-) )</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;">As for the final decision about the new settings, the current maintainer of KCalc is Evan Teran. I would suggest to add him as a reviewer, because I don't know if he monitors the mailing list.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 3rd, 2012, 10:59 p.m., <b>Christoph Feck</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/105427/diff/1/?file=71181#file71181line456" style="color: black; font-weight: bold; text-decoration: underline;">kcalcdisplay.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool KCalcDisplay::setAmount(const KNumber &new_amount)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">456</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span><span class="k">else</span> <span class="k">if</span> <span class="p">((</span><span class="n">num_base_</span> <span class="o">==</span> <span class="n">NB_OCTAL</span><span class="p">)</span> <span class="o">&&</span> <span class="n">groupdigits_</span> <span class="o">&&</span> <span class="o">!</span><span class="n">special</span><span class="p">)</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;">I strongly suggest to create a new method for the digit grouping, and call it with a different argument, depending on number base.
Additionally, the comments seem to make the code more complicated than it is. Reduce the amounts of comments, and let the code describe itself. Only add comments where the code does not do what it seems to do.
</pre>
</blockquote>
<p>On July 4th, 2012, 7:31 a.m., <b>Michael Skiba</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;">It's rather difficult for me to judge which comments are needed and which are not, since I don't have really much real world programming experience, though I agree that it makes the actual code harder to read.
Regarding the function you thought about something like this?
void KCalcDisplay::setText(const QString &string)
{
text_ = string;
bool special = (string.contains(QLatin1String( "nan" )) || string.contains(QLatin1String( "inf" )));
// I'd leave this one as it is, since it relies on KGlobal
if ((num_base_ == NB_DECIMAL) && groupdigits_ && !special) {
if (string.endsWith(QLatin1Char( '.' ))) {
text_.chop(1);
text_ = KGlobal::locale()->formatNumber(text_, false, 0);
text_.append(KGlobal::locale()->decimalSymbol());
} else {
text_ = KGlobal::locale()->formatNumber(text_, false, 0);
}
}else if ((num_base_ == NB_BINARY) && groupdigits_ && !special) {
text_ = groupdigits(text_, binaryGrouping_);
}else if ((num_base_ == NB_OCTAL) && groupdigits_ && !special) {
text_ = groupdigits(text_, octalGrouping_);
}else if ((num_base_ == NB_HEX) && groupdigits_ && !special) {
text_ = groupdigits(text_, hexadecimalGrouping_);
}
update();
emit changedText(text_);
}
QString KCalcDisplay::groupdigits(QString displayString, int numDigits)
{
QString tmpDisplayString;
int stringLength = text_.length();
for (int i = stringLength; i > 0 ; i--){
if(i % numDigits == 0 && i != stringLength){
tmpDisplayString = tmpDisplayString + " ";
}
tmpDisplayString = tmpDisplayString + displayString[stringLength - i];
}
return tmpDisplayString;
}
Also I was wondering (and I hope I don't stress your patience to much here) whether it would be more efficient to change the if statement to something like this:
if(groupdigits_ && !special){
if (num_base_ == NB_DECIMAL) {
if (string.endsWith(QLatin1Char( '.' ))) {
text_.chop(1);
text_ = KGlobal::locale()->formatNumber(text_, false, 0);
text_.append(KGlobal::locale()->decimalSymbol());
} else {
text_ = KGlobal::locale()->formatNumber(text_, false, 0);
}
}else if (num_base_ == NB_BINARY) {
text_ = groupdigits(text_, binaryGrouping_);
}else if (num_base_ == NB_OCTAL) {
text_ = groupdigits(text_, octalGrouping_);
}else if (num_base_ == NB_HEX) {
text_ = groupdigits(text_, hexadecimalGrouping_);
}
}
I've not yet uploaded an updated patch because I wasn't sure if this section is as you've expected it, so I've not yet modified the last part in the code. All the other suggestions above are in the code already and seem to work. :-)</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;">You are right, the "if" statement can be pulled out this way. Inside the "if", I would use a switch/case chain, and maybe rename the method to "groupDigits".</pre>
<br />
<p>- Christoph</p>
<br />
<p>On July 4th, 2012, 7:31 a.m., Michael Skiba 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 KDE Utils.</div>
<div>By Michael Skiba.</div>
<p style="color: grey;"><i>Updated July 4, 2012, 7:31 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;">Completing a JJ from KBO: Enable grouping of non-decimal numbers. I've added new options in the settings menu to allow the user to set individual grouping width.
KCalc now displays "11 1101 0110" in binary mode instead of "1111010110", the user can change that behavior to any value he or she wants for example bytewise: "11 11010110".
The user can set individual widths, which are stored for binary, octal and decimal. I've set the following defaults:
binary = 4 ['1110 1010']
octal = 4 ['5323 1356']
hexadecimal = 2 ['A2 F4 C2 11'] </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;">Save/Restoring of saved individual values work.
Calculation and conversation is correct.</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=247151">247151</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>general.ui <span style="color: grey">(15bee34)</span></li>
<li>kcalc.h <span style="color: grey">(01cc37e)</span></li>
<li>kcalc.cpp <span style="color: grey">(76bb123)</span></li>
<li>kcalc.kcfg <span style="color: grey">(6b613bd)</span></li>
<li>kcalcdisplay.h <span style="color: grey">(84c1908)</span></li>
<li>kcalcdisplay.cpp <span style="color: grey">(c1c238f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105427/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/105427/s/614/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/07/03/kcalc_400x100.png" style="border: 1px black solid;" alt="Grouped binary number" /></a>
<a href="http://git.reviewboard.kde.org/r/105427/s/615/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/07/03/kcalc1_400x100.png" style="border: 1px black solid;" alt="Updated settings window" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>