----------------------------------------------------------- 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 > >
