Review Request: Add keyboard navigation to plasma applet Folder View

[Aaron Seigo]

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


this is a very important feature to add, so thanks for working on it. there are a number of small things that need to be improved a bit in the patch before it can go in, but you've done most of the hard work already! :)


/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment321>

    code style: no spaces around or inside the ()s, and you don't need to use "this->". just: setFocusPolicy(Qt::StrongFocus);



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment322>

    code style: one line per variable:
    
    int hdirection = 0;
    int vdirection = 0;



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment323>

    switch (event->key()) {



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment324>

    coding style: an if should be written this way:
    
    if (..conditional..) {
        ... block ...
    }
    
    braces are required even on single line if/while/for statements



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment325>

    this could be written as:
    
    case Qt::Key_Return:
    case Qt::Key_Enter:
         emit activated(m_selectionModel->currentIndex();
         break;
    
    C++ supports "fall throughs" in switch statements that make this nice.
    
    also, if the key pressed was Return or Enter, there is no need to do any repainting or movement of the selection index. so the method should simply return at this point.
    
    finally, there should be a default case at the end to avoid compiler warnings:
    
         default:
             break;



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment326>

    this seems like more painting than necessary? unless the scrollbar moves, only the two icons that changed (the one that was selected and the one that is now selected) need to be repainted.
    



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
<http://reviewboard.kde.org/r/368/#comment327>

    this should be an if/else if/else statement:
    
      if (currentRow == numberOfRows) {
            m_scrollBar->setValue(m_scrollBar->maximum());
      }
      else if( currentRow == 1 ) {
            m_scrollBar->setValue(m_scrollBar->minimum());
      } else { 
            m_scrollBar->setValue(division * currentRow);
      }
    
    otherwise it will never set the scrollbar to the minimum when currentRow == 1, but to division. this should solve the problem you were having.


- Aaron


On 2009-03-20 09:14:10, Shantanu Tushar Jha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/368/
> -----------------------------------------------------------
> 
> (Updated 2009-03-20 09:14:10)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This partly addresses the above bug, adding keyboard navigation and launch using Enter key.
> Please report if the code is too complex, I've tried my best to keep it to the point.
> 
> 
> This addresses bug 187241.
>     https://bugs.kde.org/show_bug.cgi?id=187241
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 941694 
>   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 941694 
> 
> Diff: http://reviewboard.kde.org/r/368/diff
> 
> 
> Testing
> -------
> 
> Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar->setValue( m_scrollBar->minimum() ); is used. What am I doing wrong?
> 
> 
> Thanks,
> 
> Shantanu
> 
>