Review Request: Clear the previous selection when pasting or dropping items (fixing a recent 4.9 branch regression)

[Frank Reininghaus]

> On Nov. 19, 2012, 10:34 p.m., Mark Gaiser wrote:
> > Hi Frank,
> > 
> > Why don't you just adjust the DolphinView::markUrlsAsSelected function?
> > void DolphinView::markUrlsAsSelected(const QList<KUrl>& urls)
> > {
> >     KItemListSelectionManager* selectionManager = m_container->controller()->selectionManager();
> >     selectionManager->clearSelection();
> >     m_selectedUrls = urls;
> > }
> > 
> > Above is just a copy/paste of a few things. It probably works :p
> > 
> > Actually, that function - as it is right now in git - isn't doing what it's name tells me. It should "select" the given url's. It just sets the url's that "should" be selected and doesn't do selection. The DolphinView::updateViewState does the actual selection...
> > 
> > 
> > Cheers,
> > Mark

> Why don't you just adjust the DolphinView::markUrlsAsSelected function?

In the drag&drop use case, we do not want to clear the previous selection if the user aborts the drop when the "Move/Copy/Link" menu is shown. Therefore, we cannot clear the selection before we know that the drop has actually happened.

> Actually, that function - as it is right now in git - isn't doing what it's name tells me. It should "select" the given url's. 
> It just sets the url's that "should" be selected and doesn't do selection. The DolphinView::updateViewState does the actual selection...

That's because this function is called *before* the pasted/dropped items have been added to the model.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107389/#review22225
-----------------------------------------------------------


On Nov. 19, 2012, 9:02 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107389/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2012, 9:02 p.m.)
> 
> 
> Review request for Dolphin and Emmanuel Pescosta.
> 
> 
> Description
> -------
> 
> To reduce the risk of regressions, I thought I'd better let other people have a look at non-trivial patches of mine before pushing. In particular, selecting items is an area where a lot can go wrong. Emmanuel, can you have a look at the patch? You're doing some work in the area at the moment, so maybe you see if that breaks something or if there's an easier way to do it (I doubt that we can avoid the new member though if we don't want to bring bug 217575 back though). This fix is mostly independent of your patch https://git.reviewboard.kde.org/r/107351/. That one should also go in, of course - I'm uploading the fix for the regression now because I want to fix it before KDE 4.9.4 is tagged.
> 
> Others are of course also welcome to comment on the patch!
> 
> 
> This addresses bug 310365.
>     http://bugs.kde.org/show_bug.cgi?id=310365
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinview.h 7d8e8b7 
>   dolphin/src/views/dolphinview.cpp 13db989 
> 
> Diff: http://git.reviewboard.kde.org/r/107389/diff/
> 
> 
> Testing
> -------
> 
> Fixes the bug for me, and I haven't found any new regressions so far.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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