Review Request: Add keyboard navigation to plasma applet Folder View

Shantanu Tushar Jha jhahoneyk at gmail.com
Sat Apr 4 20:19:43 CEST 2009



> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > I think in general the code looks good, but there are still numerous coding style issues, especially with the way the code is indented.

Oh, I apologise for that, but I'm unable to figure out where I've messed up with the indentation. Please point few examples where there's a problem, it'll be lots of help.


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h, line 119
> > <http://reviewboard.kde.org/r/368/diff/4/?file=4670#file4670line119>
> >
> >     I'd prefer it if this function was in AbstractItemView instead, since the code will work for the ListView class as well.

Ok, done. Will reflect it in next Diff after I resolve the flow issue.


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 110
> > <http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line110>
> >
> >     IconView already sets the focus policy to StrongFocus at the top of the constructor (as of today).
> >

Oh, I think it has been added to the constructor recently. I've removed my line.


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1344
> > <http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1344>
> >
> >     This could result in a division by zero.

The only place where m_columns is set to 0 is the constructor, but after the layout is done, there should be at least one column and one row, then how m_column can be zero? m_columns is used to hold the number of columns visible, isn't it?


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1372
> > <http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1372>
> >
> >     This code still doesn't take the flow into account.
> >     The icons can flow from left to right, right to left, top to bottom and so on, as indicated by m_flow.
> >

I can't figure out much what does flow mean here, only that it might mean that the model might be displayed in reverse order for some value of m_flow. What exactly is it doing, is unclear to me as I couldn't find some way to set the flow from the applet config and see the effect. Please hint on this.
Thanks a lot for the help :)


- Shantanu


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


On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/368/
> -----------------------------------------------------------
> 
> (Updated 2009-04-02 13:21:02)
> 
> 
> 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 948236 
>   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 
> 
> 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
> 
>



More information about the Plasma-devel mailing list