> 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
