Allow opening files and directories by pressing 'Enter' or 'Return'

Review Request #102450 - Created Aug. 27, 2011 and submitted

Information
Tirtha Chatterjee
kde-baseapps
master
Reviewers
kde-baseapps
ppenz
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.
Yes, tested and working.
Tirtha Chatterjee
Tirtha Chatterjee
Peter Penz
Tirtha Chatterjee
Review request changed
Peter Penz
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.
  1. I have used itemActivated signal here since itemTriggered is already used by DolphinView. Then I noticed that in my previous keyboard-searching patch, we use the name 'activation' to mean selection. So is it okay to use activation here?
  2. QAbstractItemView also uses the signal 'activated()' for the same meaning as we have, so I'd say using itemActivated() instead of itemTriggered() is fine. Would like to hear Frank's opinion about this, but I'd say please push the patch, we can change this afterwards if we change our mind (currently I'd also tend to rename the DolphinView signal to 'itemActivated' for consistency).
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
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 (Diff revision 5)
 
 
Remove: It is only used in the else-path
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
Change to:
if (itemsSet.isEmpty()) {
   return;
}
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
Coding style - braces:
if (itemsSet.count() == 1) {
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
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 (Diff revision 5)
 
 
As the KFileItem declaration has been remove above, use:
const KFileItem fileItem = ...;
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
Coding style, please use:
} else {
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
const KFileItem item = ...
dolphin/src/views/dolphinview.cpp (Diff revision 5)
 
 
1. const KFileItem item = ...;
2. I'd suggest to move it down right before it is needed before 'emit requestContextMenu'
Commit Hook
This review has been submitted with commit ebc84058d15b250417d16a2d14ca0b62aa729792 by Tirtha Chatterjee to branch master.
Loading...