Review Request 117021: Fix Bug 332159 - dolphin does not remember scroll position when going back

[Emmanuel Pescosta]

> On March 24, 2014, 11:45 p.m., Frank Reininghaus wrote:
> > Thanks for your analysis and your patch! You are right, the double emission of KFileItemModel's directoryLoadingCompleted() signal is the problem.
> > 
> > Modifying DolphinView::updateViewState() such that it still keeps the position to restore if the restoration was not successful looks like a good approach to fix the problem! The second call of that function will restore the correct position then. However, I'm not really sure why
> > 
> > KFileItemModel::setShowHiddenFiles(bool show)
> > 
> > calls slotCompleted() at all (this is what triggers the emission of directoryLoadingCompleted() eventually). Removing that call also fixes the problem for me, but it will probably cause some regressions? There is probably a reason why the call is there, but it seems that the first commit that "git blame" points to simply changed the indentation of that line. Some more code archeology might be needed. But maybe you had already tried this approach and found that the change in DolphinView is better?
> > 
> > The reason why I'm asking is that I see a certain risk that the change in DolphinView will cause subtle bugs in the future. Imagine the following scenario:
> > 
> > 1. In folderA/, scroll down and click the very last folder (in Details View).
> > 
> > 2. While in folderA/subfolder/, remove any other subfolder in folderA from a terminal.
> > 
> > 3. Go "back". Restoring the position will fail because the view's total height is smaller than it used to be. However, with your patch, the desired position will still be stored in m_restoredContentsPosition, right?
> > 
> > 4. Go to any other folder, which has more items than folderA/. If I'm not mistaken, the view might now scroll down to the offset which should originally have been restored in step 3.
> > 
> > This reminds me a bit of https://bugs.kde.org/show_bug.cgi?id=329377, which was not exactly easy to debug. But I'm not completely sure if my analysis is right, and the same problem could indeed happen in the "restore scroll position" context.
> 
> Emmanuel Pescosta wrote:
>     > Removing that call also fixes the problem for me, but it will probably cause some regressions?
>     I don't know if it will cause any regressions, this is why I have fixed it in DolphinView::updateViewState() so 
>     that all slotCompleted() calls try to scroll to the right position.
>     
>     > subtle bugs in the future
>     No problems found so far. 
>     
>     I think we should fix it by removing the slotCompleted() call in setShowHiddenFiles, because it is a much nicer
>     solution and if we get some regression bug reports (I doubt that ;) we can revert it and apply this patch. 
>     What do you think?
>     
>     Thanks!
> 
> Frank Reininghaus wrote:
>     Just tried it - it does cause a regression: pressing F8 (to enable "Show hidden files") will not actually show the hidden files then. They will be shown, e.g., after expanding a folder in details view though, because this will trigger the dir lister's completed() signal.
>     
>     It seems that the point of the slotCompleted() call (which was introduced in http://quickgit.kde.org/?p=kde-baseapps.git&a=commit&h=bcf5697fbd09171a6b63599c09923a17b1524d14 btw, I just went through the history) is to make sure that the hidden files are actually inserted into the model.
>     
>     Maybe a better solution is to replace the slotCompleted() call in KFileItemModel::setShowHiddenFiles(bool show) by dispatchPendingItemsToInsert(), which will insert the items into the model without emitting that "completed" signal. Requires some careful testing though to make sure that it doesn't break anything else (and that it still fixes the "scroll position" bug).
> 
> Emmanuel Pescosta wrote:
>     > dispatchPendingItemsToInsert()
>     Tested it in the last few days under normal usage, no problems noticed so far.
>     
>     I'll test it under some special circumstances this weekend.
> 
> Frank Reininghaus wrote:
>     If you haven't found any problems with that approach yet (I haven't seen any either), I think that you should push it to KDE/4.13, such that the bug will be fixed in 4.13.1. Thanks for your investigations!
>

I'll push it to KDE/4.13 soon.

Thanks for your help! :)


- Emmanuel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117021/#review54044
-----------------------------------------------------------


On March 24, 2014, 4:19 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117021/
> -----------------------------------------------------------
> 
> (Updated March 24, 2014, 4:19 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 332159
>     http://bugs.kde.org/show_bug.cgi?id=332159
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Fix Bug 332159 - dolphin does not remember scroll position when going back
> 
> DolphinView::updateViewState will be called multiple times in some special cases,
> so we only reset the restored contents position if we have successfully reached it.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinview.cpp 2769d67 
> 
> Diff: https://git.reviewboard.kde.org/r/117021/diff/
> 
> 
> Testing
> -------
> 
> Works. 
> 
> See Ashish's comment #5 in the bug report:
> 
>  Ashish 2014-03-23 19:02:46 CET
> I can reproduce this only when the 1st folder has "Show hidden files" enabled. 
> Step 1: Enable Show Hideen Files in say, home folder. 
> 2: Scroll down, open any folder. 
> 3: click back. We reach the top position in home folder. When Hidden files is not enabled, clicking on back remembers the scroll position. 
> Version tested: 4.12.3 on Ubuntu 13.10
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140417/f55e2fc2/attachment.htm>