Review Request 119703: Always clear DolphinView's m_currentItemUrl member in updateViewState()

[Frank Reininghaus]

> On Aug. 11, 2014, 7:14 p.m., Emmanuel Pescosta wrote:
> > Thanks for fixing this bug!
> > 
> > I tested your patch and I haven't found any problems yet. 
> > 
> > Maybe we should move the "m_currentItemUrl = KUrl();" into the "if (m_currentItemUrl != KUrl())" block?

Thanks for testing it! The statement already is in this block, isn't it?


- Frank


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


On Aug. 12, 2014, 7:22 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119703/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2014, 7:22 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 329377
>     http://bugs.kde.org/show_bug.cgi?id=329377
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> m_currentItemUrl is used to keep track of an item which should be made the current item as soon as loading the current folder is finished (e.g., when going back, or when pasting items). DolphinView::updateViewState() is the function that checks if m_currentItemUrl is not empty and then tries to make it the current item.
> 
> However, if m_currentItemUrl is not found in the model, then the function makes 0 the current index (I'm not entirely sure why), and, what's worse, still keeps the URL in m_currentItemUrl. The next time updateViewState() is called, which could be, e.g., after an expansion toggle next to a folder in Details View is clicked, and the children of that folder are loaded, the following happens: 0 is again made the current index, which causes strange issues, like the unexpected selection of all items between the first item in the view and the selected folder.
> 
> I think that we should always clear m_currentItemUrl in updateViewState(). Even if that function is called multiple times (e.g., when many large files are being pasted), I think that there will not be a problem because we will always try to make the first pasted URL the current one, and this one should already be there the first time updateViewState() is called.
> 
> But I hope that I do not overlook anything - Emmanuel, do you agree that this change is safe? I do not want to break what you did in https://git.reviewboard.kde.org/r/109950/ :-)
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinview.cpp c1f585d 
> 
> Diff: https://git.reviewboard.kde.org/r/119703/diff/
> 
> 
> Testing
> -------
> 
> I cannot reproduce the bug with the steps from https://bugs.kde.org/show_bug.cgi?id=329377#c10 any more (but I still can with the unpatched KDE/4.14 branch).
> 
> I think I tried most of the things that m_currentItemUrl is used for (restoring the view state when going 'back', pasting files, using the --select option on the command line), and I have not found any new problems so far.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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