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

[Christoph Feck]

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


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.


general.ui
<http://git.reviewboard.kde.org/r/105427/#comment11958>

    Is there a reason for the added sizePolicy property? Otherwise, remove.



kcalc.kcfg
<http://git.reviewboard.kde.org/r/105427/#comment11959>

    Are these strings user-visible? If yes, they need some polish.



kcalcdisplay.h
<http://git.reviewboard.kde.org/r/105427/#comment11960>

    The argument is not a flag, but the number of digits. Maybe rename to "int digits".



kcalcdisplay.cpp
<http://git.reviewboard.kde.org/r/105427/#comment11961>

    see above



kcalcdisplay.cpp
<http://git.reviewboard.kde.org/r/105427/#comment11962>

    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.
    


- Christoph Feck


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/20120703/dafa873e/attachment.html>