Re: Review Request: Save scrollbar position on plasma exit
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. Ignat Semenov wrote: Actually, aborting the automatic scrolling works just fine, as smoothScroll(savedPosition) is used and that one can be interrupted easily. Ignat Semenov wrote: OK, as discussed with fredrikh on IRC yesterday, the patch now accounts for multiple layout passes. As far as the dir mtime issue goes, I think that actually falls into the range check case, that is, if the dir content changes, but the number of rows does not, it's probably ok to restore the scroll position. If the number of rows changes, then the scrollbar range changes and that is caught by the check in scrollToSavedPosition(). Same for the list view. Now the only relevant issue is actually aborting the restore if scrolling the view between layout passes in a slow dir. OK, the only thing left now is to correctly handle this: applet created on the desktop - scrolled - put into the panel -url changed - dragged back to the desktop - scrolled again, but the dir is different and the scroll is invalid on the other hand, we can't simply discard the value after restoring the position, since if the above is done without changing the url, the scrollbar restore on panel - desktop is desired and correct. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 --- On March 16, 2012, 4:31 p.m., Ignat Semenov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 4:31 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 (max0) 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.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine.
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 10:02 a.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Discard the scrollbar position restore if the user has manually scrolled the view before the listing is finished. The patch is for IconView only. There is a big problem with doing the same in ListView, actuallly, it is unnecessary there at all. This is why: There is no multi-pass layout in ListView; moreover, when the user clicks the icon in the panel, and the listing starts, the popup won't open before the listing is finished. The user has no chance of scrolling the view before the listing is finished, so the problem doens't even exist there. What botheres me is the Plasma stall on loading a big dir. I think we should port the multiple-pass layout code to the ListView class to ensure responsiveness, but that will be a different review request. Fredrik, what do you think? This RR is finished now, I think it's ready to go. Please, do nitpick :) 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 (max0) branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - 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.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 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
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 1:47 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Call discardScrollbarRestore() directly from AbstractItemView::scrollBarActionTriggered(). 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 (max0) branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - 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.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 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
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 4:31 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Avoid race condition between layoutFinished() and restoreScrollBarPosition(). Connect restoreScrollBarPosition() and scrollToSavedPosition() in the AbstractItemView ctor. Introduce AbstractItemView::setSavedScrollbarPosition(int) and call that before setModel in teh view setup methods in FolderView. 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 (max0) branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - 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.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 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
Re: Review Request: Save scrollbar position on plasma exit
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. Ignat Semenov wrote: Actually, aborting the automatic scrolling works just fine, as smoothScroll(savedPosition) is used and that one can be interrupted easily. OK, as discussed with fredrikh on IRC yesterday, the patch now accounts for multiple layout passes. As far as the dir mtime issue goes, I think that actually falls into the range check case, that is, if the dir content changes, but the number of rows does not, it's probably ok to restore the scroll position. If the number of rows changes, then the scrollbar range changes and that is caught by the check in scrollToSavedPosition(). Same for the list view. Now the only relevant issue is actually aborting the restore if scrolling the view between layout passes in a slow dir. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 --- On March 13, 2012, 6:44 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, 6:44 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 (max0) 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.h 12e93b3 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
Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- 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 (max0) branch? 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
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 13, 2012, 1:06 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Added bug number. 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 (max0) 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
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 --- 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) - Marco Martin On March 13, 2012, 1:06 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:06 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 (max0) 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
Re: Review Request: Save scrollbar position on plasma exit
--- 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. Changes --- Check the saved position for validity. Actually, I used to have that check, but removed it later because it was in restoreScrollbarPosition() and the range was of course invalid in that method. Silly me did not think about putting it where it belongs. :) 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 (max0) branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - 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
Re: Review Request: Save scrollbar position on plasma exit
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) 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. - Fredrik --- 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 (max0) 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
Re: Review Request: Save scrollbar position on plasma exit
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. 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. - 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 (max0) 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
Re: Review Request: Save scrollbar position on plasma exit
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 (max0) 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
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 13, 2012, 6:44 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Ensure the layout is really finished before emitting layoutFinished(). 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 (max0) branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - 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.h 12e93b3 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
