> On March 13, 2012, 1:12 p.m., Marco Martin wrote: > > looks good, a thing i would like to be tested is when the saved position is > > invalid, like either negative or an enormous value. > > > > this shouldn't break it (is even quite probable a value not being valid > > anymore because there are less files than the previous session) > > Fredrik Höglund wrote: > I think it would be a good idea to save the modification time for the > folder and use that to check if the saved scrollbar value is likely to be > invalid. If the user is able to scroll the view while the layout is in > progress, this should also abort restoring the position. > > Also, I'm wondering if we really need to save the position separately for > the iconview and the listview, or if we should use the same key. > > Otherwise the patch looks good to me. > > Ignat Semenov wrote: > Well I've been thinking about separate vs single key and I think separate > is easier to read and maintain, less checks and branches. > > The main problem with a single key would be when the applet is put into a > panel, first the icon view gets created and grabs the value, then the list > view gets created and gets a 0. Two keys are easier to work with I think. > > As for scrolling when the layout is in progress, this method is intended > to be used at startup only, so the user can not scroll the view. Or do you > mean that some dev could use restoreScrollbarPosition() manually after > startup? > > Folder mtime is a nice idea, one more corner case, will try to implement.
Actually, aborting the automatic scrolling works just fine, as smoothScroll(savedPosition) is used and that one can be interrupted easily. - Ignat ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 ----------------------------------------------------------- On March 13, 2012, 1:51 p.m., Ignat Semenov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104258/ > ----------------------------------------------------------- > > (Updated March 13, 2012, 1:51 p.m.) > > > Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. > > > Description > ------- > > This patch implements scrolbar position saving on plasma exit. The change is > fairly trivial, however, due to the fact that the view is not populated and > layouted immediately simply scrolling to the desired position on creating the > view does not work. Instead a signal is emitted on finishing the item layout, > when the view has a valid size and the scrollbar has a valid range. The > signal is connected to a slot which scrolls the view to the desired position > and then disconnects the signal. For the user, a public function in > AbstractItemView is introduced, which performs the connection. > > The only problem is that ListView turned out not to have any layout method. > It just paints the items one by one, calculating their position on the fly, > so I put the signal at the end of updateScrollbar to ensure the scrollbar > range is valid. Maybe it should go into the "if (max>0)" branch? > > > This addresses bug 261139. > http://bugs.kde.org/show_bug.cgi?id=261139 > > > Diffs > ----- > > plasma/applets/folderview/abstractitemview.h aa68b90 > plasma/applets/folderview/abstractitemview.cpp 3debb70 > plasma/applets/folderview/folderview.h 4e441eb > plasma/applets/folderview/folderview.cpp a94ce87 > plasma/applets/folderview/iconview.cpp 5c4e086 > plasma/applets/folderview/listview.cpp b17e7c4 > > Diff: http://git.reviewboard.kde.org/r/104258/diff/ > > > Testing > ------- > > Tested both the icon view and the list view, works fine. > > > Thanks, > > Ignat Semenov > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
