Review Request: Find list items by typing their initial letters.

[Peter Penz]

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


Thanks for this patch. As discussed per e-mail already I think from a design point of view this is fine and the patch looks good! I've added quite a lot of nitpicking comments, would be great if you could fix those things and do one update here. Afterwards I'm confident that this can be pushed.


dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/102465/#comment5351>

    Coding style - braces:
    if (startFromIndex < 0) {
        startFromIndex = 0;
    }
    
    also
       startFromIndex = qMax(0, startFromIndex)
    would be an option.



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/102465/#comment5352>

    Coding style - braces:
    for (int i = startFromIndex; i < count(); i++) {
    ...



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/102465/#comment5353>

    Coding style - braces



dolphin/src/kitemviews/kitemlistcontroller.h
<http://git.reviewboard.kde.org/r/102465/#comment5354>

    Please remove this signal, the searching is done internally and should not be exposed to the public.



dolphin/src/kitemviews/kitemlistcontroller.h
<http://git.reviewboard.kde.org/r/102465/#comment5355>

    - Change 'QString text' to 'const QString& text'
    
    - I'd suggest to rename it to: void requestItemActivationByKeyboard(const QString& text)



dolphin/src/kitemviews/kitemlistcontroller.cpp
<http://git.reviewboard.kde.org/r/102465/#comment5356>

    Coding style - braces



dolphin/src/kitemviews/kitemlistcontroller.cpp
<http://git.reviewboard.kde.org/r/102465/#comment5357>

    const int startFromIndex = ...
    const int index = ...;



dolphin/src/kitemviews/kitemlistkeyboardmanager.h
<http://git.reviewboard.kde.org/r/102465/#comment5358>

    I'm fine with your suggestion above to use the (quite long) name KItemListKeyboardSearchManager. I think it makes clearer what is done here...
    
    But please add a _p.h postfix to the name of the headerfile (-> kitemlistkeyboardsearchmanager_p.h) as it is meant as private class from a module point of view.



dolphin/src/kitemviews/kitemlistkeyboardmanager.h
<http://git.reviewboard.kde.org/r/102465/#comment5361>

    I think the name is quite confusing, I'd suggest to simply use:
       void addKey(const QString& key)
    and add a documentation to the method what it does (also here: 'const QString&' instead of 'QString')



dolphin/src/kitemviews/kitemlistkeyboardmanager.h
<http://git.reviewboard.kde.org/r/102465/#comment5359>

    Replace 'QString string' by 'const QString& string'



dolphin/src/kitemviews/kitemlistkeyboardmanager.h
<http://git.reviewboard.kde.org/r/102465/#comment5360>

    Personally I'd prefer using a QTimer* m_timer here, but this is fine too.



dolphin/src/kitemviews/kitemlistkeyboardmanager.cpp
<http://git.reviewboard.kde.org/r/102465/#comment5362>

    KFileItemModel (or any other concrete implementation of the model) may not be included in this class. Is not used anyhow here :-)



dolphin/src/kitemviews/kitemmodelbase.h
<http://git.reviewboard.kde.org/r/102465/#comment5363>

    Please start each sentence in the documentation with a capital letter and end it with a dot.


- Peter


On Aug. 27, 2011, 8:36 p.m., Tirtha Chatterjee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102465/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2011, 8:36 p.m.)
> 
> 
> Review request for KDE Base Apps and Peter Penz.
> 
> 
> Summary
> -------
> 
> This patch allows finding items by typing on the keyboard while the KItemListController is in focus. Timer and key queuing is handled by KItemListKeyboardManager, and search itself is done by reimplementing indexForKeyboardSearch(QString) from KItemModelBase. This implementation does not, at the moment, take care of the repetitive typing as suggested by Frank. I think we can implement that once this is in.
> 
> p.s. Not sure if line 213 in kitemlistcontroller.cpp is the best way to do it. Returning true does not work.
> p.s. I feel the name KItemListKeyboardManager can be changed to KItemListKeyboardSearchManager, although a little too big.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 31d3f89 
>   dolphin/src/kitemviews/kfileitemmodel.h 654c7db 
>   dolphin/src/kitemviews/kfileitemmodel.cpp f36ab83 
>   dolphin/src/kitemviews/kitemlistcontroller.h 134e116 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp 2e32880 
>   dolphin/src/kitemviews/kitemlistkeyboardmanager.h PRE-CREATION 
>   dolphin/src/kitemviews/kitemlistkeyboardmanager.cpp PRE-CREATION 
>   dolphin/src/kitemviews/kitemmodelbase.h 4670469 
>   dolphin/src/kitemviews/kitemmodelbase.cpp fc604e7 
> 
> Diff: http://git.reviewboard.kde.org/r/102465/diff
> 
> 
> Testing
> -------
> 
> yes. tested and works.
> 
> 
> Thanks,
> 
> Tirtha
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110827/1cf34d74/attachment.htm>