Review Request: KCalc: JJ: Group digits in n-base mode (bin, oct, hex)

Michael Skiba opensource at michael-skiba.de
Wed Jul 4 07:31:14 UTC 2012 


> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > 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.

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 ;-) )


> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > general.ui, line 13
> > <http://git.reviewboard.kde.org/r/105427/diff/1/?file=71176#file71176line13>
> >
> >     Is there a reason for the added sizePolicy property? Otherwise, remove.

I probably clicked somewhere and it got auto-generated by QT Creator O:-)


> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > kcalc.kcfg, line 146
> > <http://git.reviewboard.kde.org/r/105427/diff/1/?file=71179#file71179line146>
> >
> >     Are these strings user-visible? If yes, they need some polish.

Honestly I don't know. I just saw that every other entry had a <whatsthis>-Tag and added them here too. However I was not able to display this text with Shift+F1 ("What's this?") ...


> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > kcalcdisplay.h, line 88
> > <http://git.reviewboard.kde.org/r/105427/diff/1/?file=71180#file71180line88>
> >
> >     The argument is not a flag, but the number of digits. Maybe rename to "int digits".

Done


> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > kcalcdisplay.cpp, line 456
> > <http://git.reviewboard.kde.org/r/105427/diff/1/?file=71181#file71181line456>
> >
> >     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.
> >

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. :-)


- Michael


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


On July 3, 2012, 9:11 p.m., Michael Skiba wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105427/
> -----------------------------------------------------------
> 
> (Updated July 3, 2012, 9:11 p.m.)
> 
> 
> Review request for KDE Utils.
> 
> 
> Description
> -------
> 
> 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'] 
> 
> 
> This addresses bug 247151.
>     http://bugs.kde.org/show_bug.cgi?id=247151
> 
> 
> Diffs
> -----
> 
>   general.ui 15bee34 
>   kcalc.h 01cc37e 
>   kcalc.cpp 76bb123 
>   kcalc.kcfg 6b613bd 
>   kcalcdisplay.h 84c1908 
>   kcalcdisplay.cpp c1c238f 
> 
> Diff: http://git.reviewboard.kde.org/r/105427/diff/
> 
> 
> Testing
> -------
> 
> Save/Restoring of saved individual values work.
> Calculation and conversation is correct.
> 
> 
> Screenshots
> -----------
> 
> Grouped binary number
>   http://git.reviewboard.kde.org/r/105427/s/614/
> Updated settings window
>   http://git.reviewboard.kde.org/r/105427/s/615/
> 
> 
> Thanks,
> 
> Michael Skiba
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20120704/cee2885c/attachment-0001.html>

More information about the Kde-utils-devel mailing list