----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102450/#review6128 -----------------------------------------------------------
Ship it! Thanks for the update! Looks good and is exactly like the proposal you, Frank and I discussed per e-mail. As usual I've added a punch of my nitpicking stuff ;-) Please push it to master after fixing, you don't need to add another diff here. dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5407> 1. const is missing: const QSet<int>... 2. I'd suggest to use 'selection' or 'selectedItems' instead of 'itemsSet' for consistency with the other code. dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5408> Remove: It is only used in the else-path dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5409> Change to: if (itemsSet.isEmpty()) { return; } dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5410> Coding style - braces: if (itemsSet.count() == 1) { dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5411> Replace 'const int& i' by just 'int i'. The const-reference in foreach is fine for classes but unnecessary (and even more expensive) for scalar types like int. dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5412> As the KFileItem declaration has been remove above, use: const KFileItem fileItem = ...; dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5413> Coding style, please use: } else { dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5414> const KFileItem item = ... dolphin/src/views/dolphinview.cpp <http://git.reviewboard.kde.org/r/102450/#comment5415> 1. const KFileItem item = ...; 2. I'd suggest to move it down right before it is needed before 'emit requestContextMenu' - Peter On Aug. 29, 2011, 9:14 a.m., Tirtha Chatterjee wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102450/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2011, 9:14 a.m.) > > > Review request for KDE Base Apps and Peter Penz. > > > Summary > ------- > > Allow opening files and directories by pressing 'Enter' key. In case multiple > files are selected when enter is pressed, all of them are opened. In case of > multiple directories, the first directory gets opened in the current tab, > while the other directories open in new tabs. > > > Diffs > ----- > > dolphin/src/kitemviews/kitemlistcontroller.h 04d4985 > dolphin/src/kitemviews/kitemlistcontroller.cpp 207535c > dolphin/src/views/dolphinview.h 437f12f > dolphin/src/views/dolphinview.cpp 959e4da > > Diff: http://git.reviewboard.kde.org/r/102450/diff > > > Testing > ------- > > Yes, tested and working. > > > Thanks, > > Tirtha > >
