Review Request 112904: Pivot Tables Improved

[Inge Wallin]

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


I have taken a look at the code and have some comments.  In general it looks quite good except for the fact that most of the coding standards of calligra have been violated.  So fix the code to adhere to the standards and there will not be so much left to do.

For testing of the actual feature I need a crash course in pivot tables and how they work from the user perspective.


CMakeLists.txt
<http://git.reviewboard.kde.org/r/112904/#comment30077>

    This is not so good because staging means "in test stage".  In other words, it should not be built for production (release) builds per definition.
    
    Instead you should fix the things that are in staging that you need for this feature so that they are no longer in staging.



CMakeLists.txt
<http://git.reviewboard.kde.org/r/112904/#comment30078>

    What does the extra "NEEDS" do? I bet there is something missing here.  But even if there is, you only need one "NEEDS" anyway.



plugins/staging/pivottables/pivotmain.cpp
<http://git.reviewboard.kde.org/r/112904/#comment30080>

    In general, function and method names should be camelcased (first word lower case), not separated with underscores.



plugins/staging/pivottables/pivotmain.cpp
<http://git.reviewboard.kde.org/r/112904/#comment30079>

    Calligra coding standards: 
    
     - space after for, if, while, do
     - space around operators like = + - and so on
     - space after ; but not before.
    
    I'll not repeat this so look through the changes for violations of these rules.
    



plugins/staging/pivottables/pivotmain.cpp
<http://git.reviewboard.kde.org/r/112904/#comment30082>

    coding standards:
     - one variable per line
    
    Also, I'm not fond of the names 'count' and 'count2'. can they be made more descriptive?  prevcount should be camel cased: prevCount



plugins/staging/pivottables/pivotmain.cpp
<http://git.reviewboard.kde.org/r/112904/#comment30081>

    coding standards:
     - braces on same line as the end paren



plugins/staging/pivottables/pivotmain.cpp
<http://git.reviewboard.kde.org/r/112904/#comment30083>

    coding standards:
     - use braces also around just one line



plugins/staging/pivottables/pivotmain.cpp
<http://git.reviewboard.kde.org/r/112904/#comment30084>

    This is an excellent example for why you should use braces around even one line.
    
    Here you have commented out the one line inside the 'if' statement.  This means that the *next* line will be affected instead which is totally not what you want.  So not only is this wrong in form, it's actually a newly introduced bug.


- Inge Wallin


On Sept. 23, 2013, 5:55 p.m., Jigar Raisinghani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112904/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2013, 5:55 p.m.)
> 
> 
> Review request for Calligra and Inge Wallin.
> 
> 
> Description
> -------
> 
> The following features have been added/changed in Pivot Tables:
> 1) The Drag and Drop has significantly improved. Now you can drag n drop a label from any ListWidget to another which was not so easy to do in previous version where you had to reset everything and then again repeat the process.
> 2) The unwanted Rows/Columns are now automatically filtered out when generating the Pivot Table. Earlier, all the permuations where generated even if they contained no data. This reduces the size of Pivot Table significantly and makes it much easier to read/understand.
> 3) The UI has been changed. Now It is much more compact and comparitively more intuitive as compared to the previous version.
> 
> P.S: I will add a demo of the working Pivot Tables soon but you can check out the captures here( http://www.flickr.com/photos/99682874@N07/ )P.
> 
> 
> Diffs
> -----
> 
>   plugins/staging/pivottables/pivotmain.ui 3471394 
>   plugins/staging/pivottables/pivotmain.cpp 7178666 
>   CMakeLists.txt 9babbe8 
>   plugins/staging/pivottables/pivotmain.h c9fff50 
> 
> Diff: http://git.reviewboard.kde.org/r/112904/diff/
> 
> 
> Testing
> -------
> 
> The Pivot tables is generated in absolutely no time for fair amount of data.
> 
> 
> Thanks,
> 
> Jigar Raisinghani
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130928/95f1f7e0/attachment.htm>