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.

> 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!


- 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/20140404/c3978b0e/attachment.htm>