----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/#review541 -----------------------------------------------------------
I have some comments in addition to the coding style issues mentioned by Aaron. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h <http://reviewboard.kde.org/r/368/#comment330> This function should be named scrollTo() to match the QAbstractItemView API. In my opinion it also belongs in the AbstractItemView class, since it's not view specific. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp <http://reviewboard.kde.org/r/368/#comment331> A problem with the way this function is implemented is that it assumes that the view is always sorted and that the icons always flow from left to right. When the user has rearranged the icons (m_layoutBroken is true), you have to assume that the icons are no longer arranged in a grid and that the visual order no longer matches the order in the model. When this is the case, and the user has pressed the up key for example, you have to iterate over all the icons and find the one that is closest to the current icon while still being above it. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp <http://reviewboard.kde.org/r/368/#comment332> The implementation of this function suffers from the same problem as the one above. It should also use the smoothScroll() function instead of changing the scrollbar value directly. I would implement it by doing something like: const QRect r = mapFromViewport(visualRect(index)); if (r.top() < 0) { smoothScroll(0, r.top()); } else if (r.bottom() > geometry().height()) { smoothScroll(0, r.bottom() - geometry().height()); } - Fredrik On 2009-03-20 10:21:32, Shantanu Tushar Jha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/368/ > ----------------------------------------------------------- > > (Updated 2009-03-20 10:21:32) > > > 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
