Review Request 112584: Autocomplete feature for Calligra Sheets

[Inge Wallin]

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


Looks pretty good, except the disregard for the Calligra coding standards.  I'll check for deeper issues, if any when you fixed the trivial problems.


sheets/ui/CellEditor.h
<http://git.reviewboard.kde.org/r/112584/#comment29159>

    Qt and Calligra uses naming of getters and setters like this:
     foo() and setFoo()
    rather than
     getFoo() and setFoo()



sheets/ui/CellEditor.h
<http://git.reviewboard.kde.org/r/112584/#comment29160>

    Two mistakes here:
    1. Don't add private members when the class has a d pointer.  That defeats the purpose of it, namely to insure forward binary compatibility
    2. 'c' is a bad name because it tells you nothing.



sheets/ui/CellEditor.cpp
<http://git.reviewboard.kde.org/r/112584/#comment29161>

    Use camel casing: wordCollection



sheets/ui/CellEditor.cpp
<http://git.reviewboard.kde.org/r/112584/#comment29162>

    Some formatting here doesn't follow the calligra standards:
     - Braces on same line as for/while/do/etc
     - spaces around operators and after ';'
     - space between 'for' and '('
    



sheets/ui/CellEditor.cpp
<http://git.reviewboard.kde.org/r/112584/#comment29163>

    Same problem as above with formatting.  Check for the same problem in other places, I won't point it out everywhere.



sheets/ui/CellEditor.cpp
<http://git.reviewboard.kde.org/r/112584/#comment29164>

    space after ','



sheets/ui/CellEditor.cpp
<http://git.reviewboard.kde.org/r/112584/#comment29165>

    This is unreadable because 'c' is such an obscure name (see above). Also, every member variable should have the 'm_' prefix.


- Inge Wallin


On Sept. 8, 2013, 2:51 p.m., Jigar Raisinghani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112584/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2013, 2:51 p.m.)
> 
> 
> Review request for Calligra and Inge Wallin.
> 
> 
> Description
> -------
> 
> This feature enables you to save time by just clicking/selecting redundant entries to fill a cell in the sheet. 
> 
> How to use:
> 1) Open a file containing some data/ Open a blank file and enter some data
> 2) Try entering data in the columns which already contain some data which has atleast first 3 characters similar to some other entry in the same column
> 
> You can see that a popup appears near your cell which gives you suggestions based on the text you have entered in the current cell. You can chose any suggestion by clicking on it or using the arrow keys to navigate through options and using Tab/Enter to fill in the suggestion in the cell.
> 
> Optimization:
> 1) Currently, Autocomplete has been optimized to a large extent by Hash Tables
> 
> Further Optimization possible(but not yet done):
> 1) Navigate only through cells which are already populated using nextInRow() and nextInColumn() to populate the HashTable.
> 2) Use thread to populate Hash Table when sheet is loaded. This process if run in background, can optimize the feature further.
> 
> Further Improvements possible:
> 1) Currently, the choices filled in the suggestion list are based on Most Recently Used 3 entries. Autocomplete can be made smarter by populating the list with 3 Most Redundant entries but this would increase the complexity and optimization would again be needed.
> 
> 
> Diffs
> -----
> 
>   sheets/ui/CellEditor.h 92bdad6 
>   sheets/ui/CellEditor.cpp d539c06 
> 
> Diff: http://git.reviewboard.kde.org/r/112584/diff/
> 
> 
> Testing
> -------
> 
> The feature works successfully for fair amount of data. Testing yet to be done for Huge Files.
> 
> 
> Thanks,
> 
> Jigar Raisinghani
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130908/8e9776ec/attachment.htm>