Review Request 128563: Preserve selected items when changing folders

[Martin Tobias Holmedahl Sandsmark]

> On Jan. 29, 2017, 2:49 p.m., Emmanuel Pescosta wrote:
> > I hope that this is more understandable than my previous attempt.

It's a long time since I wrote this patch now so I don't remember all the details. But I went through several iterations before I found something that worked, including most of what you have suggested. I'm fairly sure this is the best solution short of a more major refactoring.


> On Jan. 29, 2017, 2:49 p.m., Emmanuel Pescosta wrote:
> > src/dolphinviewcontainer.cpp, lines 457-462
> > <https://git.reviewboard.kde.org/r/128563/diff/2/?file=473178#file473178line457>
> >
> >     Move this code to `DolphinViewContainer::slotUrlNavigatorLocationChanged` after `m_view->setUrl(url)` 
> >     or swap `QTimer::singleShot(0, this, &DolphinView::updateViewState)` and `emit directoryLoadingCompleted()` in `DolphinView::slotDirectoryLoadingCompleted`.
> >     I don't know what of boths approaches is the nicer one (maybe 1. because it doesn't rely on the event queue ordering), but it's up to you ;)
> >     
> >     This makes sure that the member variables (m_selectedUrls, ...) are restored before DolphinView::updateViewState() gets invoked and we can avoid the work-around in `DolphinView::restoreState`.
> 
> Emmanuel Pescosta wrote:
>     Or another and better approach:
>     * Remove `QTimer::singleShot(0, this, &DolphinView::updateViewState)` from `DolphinView::slotDirectoryLoadingCompleted`
>     * And instead call it at the end of `DolphinView::restoreState`
>     
>     Because after thinking about it again, your approach (restore state when directory loading completed) seems to be the cleanest one.

The last suggestion here breaks the patch completely, the first ones just breaks the second testcase.


> On Jan. 29, 2017, 2:49 p.m., Emmanuel Pescosta wrote:
> > src/views/dolphinview.cpp, lines 1197-1204
> > <https://git.reviewboard.kde.org/r/128563/diff/2/?file=473179#file473179line1197>
> >
> >     `DolphinView::updateViewState` does this already. Can be removed.

This breaks the second testcase.


- Martin Tobias Holmedahl


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


On Dec. 11, 2016, 12:38 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128563/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2016, 12:38 p.m.)
> 
> 
> Review request for Dolphin, David Faure, Elvis Angelaccio, Emmanuel Pescosta, and Sune Vuorela.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Instead of just clearing the selection when going back and forth in history, it now preserves the selected items.
> 
> Another bug it fixes is if you enter a folder, go back, and then hold down shift and press an arrow key to try to select something.
> 
> 
> Diffs
> -----
> 
>   src/dolphinviewcontainer.h 62f9110 
>   src/dolphinviewcontainer.cpp 1c43fc9 
>   src/views/dolphinview.cpp a737dd0 
> 
> Diff: https://git.reviewboard.kde.org/r/128563/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

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