Review Request 128666: Change KFileItemModel's createMimeData handling so it supports the upcoming stash:/ ioslave.

[David Faure]

> On Aug. 13, 2016, 8:11 a.m., David Faure wrote:
> > I actually think url() or mostLocalUrl(), but not targetUrl(), should be used in all cases, in this code.
> > 
> > targetUrl() is supposed to be "the URL that opens when clicking on the file". All other KIO operations (copy, move, d-n-d) are supposed to use the item url() or mostLocalUrl().
> > git log points to bug 307336, review 111209, which was about nepomuksearch and tags. I remember those bending the rules a little bit about that stuff. Assuming baloo doesn't need this, I vote for porting this to mostLocalUrl().
> > 
> > But wait, the code already prefers mostLocalUrl(), a bit further down. Doesn't your slave set UDS_LOCAL_PATH ? Please debug what happens in this code.
> > canUseMostLocalUrls should be true, and data->setUrls(mostLocalUrls) should happen, in which case this patch shouldn't be needed.
> 
> arnav dhamija wrote:
>     No, I do not use UDS_LOCAL_PATH. As you mentioned in the email: 
>     
>     ```So, thinking about it again, it's exactly what you want for stash:,
>     rather than UDS_LOCAL_PATH which is more for kio_desktop (which is really just
>     a virtual view of a local folder, and deletion should delete the actual file,
>     etc.). Sorry for the false track.
>     ```
>     If mostLocalUrl() falls back to the normal item.url() if a local path is not found, it is the best fit for this purpose.

Ah, right. It would be useful to document the chosen solution in a text file in the slave sources and the reasoning about why we chose that solution, I was confused because of all the back and forth :-)

Well then this should use url().

The issue with d-n-d is that we don't know if the recipient is going to be a KIO-aware application or a non-KIO-aware application.
This is why we have support for exporting both local URLs and remote URLs in KUrlMimeData::setUrls().

For "search results" ioslaves (nepomuk, baloo) or kio_desktop, DnD can use the target/local path even for a file-manager "move".
For stash:/ you need to be notified of moves, so DnD cannot use the target/local path.

To make everything work, I think this should 1) never use targetUrl, 2) put both url() and mostLocalUrl() into the mimedata using KUrlMimeData::setUrls().

And unittests wouldn't hurt...


- David


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


On Aug. 13, 2016, 8:12 a.m., arnav dhamija wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128666/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2016, 8:12 a.m.)
> 
> 
> Review request for Dolphin, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> I am writing a new ioslave for Dolphin to make it possible to virtually stage directories without affecting the source. This requires the use of a targetUrl, but creating the mime data using the targetUrl is messing up my c/p operations through the Dolphin GUI as it does not respect the hierarchy of the stash:/ ioslave. Hence, I need createMimeData to use the item's url only for this ioslave. The special handling isn't very neat, but at least this way, it won't break any other existing ioslaves which need the targetUrl.
> 
> 
> Diffs
> -----
> 
>   src/kitemviews/kfileitemmodel.cpp 1f94972 
> 
> Diff: https://git.reviewboard.kde.org/r/128666/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> arnav dhamija
> 
>

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