> 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. > > Shantanu Tushar Jha wrote: > 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. > > Fredrik Höglund wrote: > You indent the lines with 2 spaces, while the coding style specifies 4. > The coding style is documented in detail here: > http://techbase.kde.org/Policies/Kdelibs_Coding_Style
Oops, I didn't pay attention to that in the code, sorry. Will change it now. > 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. > > wrote: > 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? > > wrote: > The layout isn't done until items are actually inserted into the model, > so until that happens m_columns will be 0. So should I just return; the keyPressEvent function in the beginning if m_columns is zero - if (m_columns == 0) return; ? > 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. > > > > wrote: > 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 :) > > wrote: > The flow can only be configured in the config dialog when the applet is > used as a containment, but basically it controls how the icons are laid out > in the view, like this: > > LeftToRight: > [1][2][3] > [4][5][6] > [7][8][9] > > RightToLeft: > [3][2][1] > [6][5][4] > [9][8][7] > > TopToBottom: > [1][4][7] > [2][5][8] > [3][6][9] > Thanks, actually I figured this out today morning by manually using setFlow :) I'll make the code according to it. - 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
