<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104258/">http://git.reviewboard.kde.org/r/104258/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 13th, 2012, 1:12 p.m., <b>Marco Martin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Fredrik</p>
<br />
<p>On March 13th, 2012, 1:51 p.m., Ignat Semenov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund.</div>
<div>By Ignat Semenov.</div>
<p style="color: grey;"><i>Updated March 13, 2012, 1:51 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested both the icon view and the list view, works fine.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=261139">261139</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plasma/applets/folderview/abstractitemview.h <span style="color: grey">(aa68b90)</span></li>
<li>plasma/applets/folderview/abstractitemview.cpp <span style="color: grey">(3debb70)</span></li>
<li>plasma/applets/folderview/folderview.h <span style="color: grey">(4e441eb)</span></li>
<li>plasma/applets/folderview/folderview.cpp <span style="color: grey">(a94ce87)</span></li>
<li>plasma/applets/folderview/iconview.cpp <span style="color: grey">(5c4e086)</span></li>
<li>plasma/applets/folderview/listview.cpp <span style="color: grey">(b17e7c4)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104258/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>