Review Request 107866: Pivot Tables for Calligra Sheets

[Marijn Kruisselbrink]

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



sheets/dialogs/pivot.h
<http://git.reviewboard.kde.org/r/107866/#comment19539>

    All the existing code has this as:
    namespace Calligra
    {
    namespace Sheets
    {
    
    (so no indentation, and every { on a separate line... which seems a bit verbose, but at least get rid of the indentation)



sheets/dialogs/pivot.cpp
<http://git.reviewboard.kde.org/r/107866/#comment19540>

    inconsistent indentation, please use 4 spaces everywhere, also the prevailing code-style places { on the same line as if (and for/while/...) statements. Only for methods, classes (and namespaces it seems) { gets a line of its own



sheets/dialogs/pivotfilters.h
<http://git.reviewboard.kde.org/r/107866/#comment19541>

    oh, and please add comments behind these to make it clear what block they close; so change these two lines to:
    } // namespace Sheets
    } // namespace Calligra



sheets/dialogs/pivotfilters.cpp
<http://git.reviewboard.kde.org/r/107866/#comment19542>

    Some consistent ordering of #include's would be nice too, in general put the .h that matches the .cpp first, and then the rest in groups either most local to most global or the other way around, not sure which is most common in sheets code, so:
    // Pivot table includes
    // Sheets includes
    // Calligra includes
    // KDE includes
    // Qt includes
    // C++ includes
    or the exact reverse order.



sheets/dialogs/pivotfilters.cpp
<http://git.reviewboard.kde.org/r/107866/#comment19543>

    can you come up with more descriptive names than flag, flag1 and flag2?


- Marijn Kruisselbrink


On Jan. 15, 2013, 7:29 p.m., Jigar Raisinghani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107866/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2013, 7:29 p.m.)
> 
> 
> Review request for Calligra and Marijn Kruisselbrink.
> 
> 
> Description
> -------
> 
> I had built Pivot Tables as part of my GSoC 2012 project. Some minor features were remaining which i finished lately. I have committed my latest code to the branch. 
> 
> How to use:   Blog link : http://jigarraisinghani.blogspot.in/2012/07/pivot-tablesupdate-here-is-update-about.html
>               Video link: http://www.youtube.com/watch?v=uz2PGVNyseA
> 
>  
> 
> Note:
> 1) Please drop only the fields containing non alphabets in "Values" and support is built for only 1 field in "Values". You can drop various in "Rows" & "Columns".
> 
> Features Still to be built:
> 1) Page Fields: Only GUI is there, but the functionality is yet to be added.
> 2) Pivot Options: The functionality only contains simple functions yet. The Base Field, Base Item functionalities are yet to be added.
> 
> Also note that pivot tables only makes sense if used in correct manner.
> 
> 
> Diffs
> -----
> 
>   sheets/CMakeLists.txt f617322 
>   sheets/sheets.rc 7eae858 
> 
> Diff: http://git.reviewboard.kde.org/r/107866/diff/
> 
> 
> Testing
> -------
> 
> I have tested the code for different test cases(data sets) and it works fine.
> 
> 
> Thanks,
> 
> Jigar Raisinghani
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130116/0b6da05e/attachment.htm>