----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102465/#review6096 -----------------------------------------------------------
Ship it! Looks fine! Please just push it to master after fixing the minor const-comments. dolphin/src/kitemviews/kitemlistcontroller.cpp <http://git.reviewboard.kde.org/r/102465/#comment5381> Please remove the 'const' from searchFromNextItem. dolphin/src/kitemviews/kitemlistkeyboardsearchmanager.cpp <http://git.reviewboard.kde.org/r/102465/#comment5382> const bool dolphin/src/kitemviews/kitemlistkeyboardsearchmanager.cpp <http://git.reviewboard.kde.org/r/102465/#comment5383> const qint64 dolphin/src/kitemviews/kitemlistkeyboardsearchmanager_p.h <http://git.reviewboard.kde.org/r/102465/#comment5384> Add a . at the end of the sentence dolphin/src/kitemviews/kitemlistkeyboardsearchmanager_p.h <http://git.reviewboard.kde.org/r/102465/#comment5385> I agree with Frank regarding the bool-parameter. However I'd suggest to get the code pushed to master like this, as it is an internal interface. Probably it might be more readable to have 2 signals here but let's postpone this discussion... - Peter On Aug. 28, 2011, 12:21 p.m., Tirtha Chatterjee wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102465/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2011, 12:21 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/kitemlistkeyboardsearchmanager.cpp PRE-CREATION > dolphin/src/kitemviews/kitemlistkeyboardsearchmanager_p.h 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 > >
