Use the much faster urls() method from QMimeData
ClosedPublic

Authored by jtamate on Jan 28 2018, 8:57 AM.

Details

Summary

When requesting a list of text/uri-list, use the much faster QMimeData
urls() method.
The unittests pass (the desktop: protocol) and
CCBUG: 342056
is half solved. The paste speed is as fast as drag&drop in local files
and with fish: files.
The lock in X11 plasma or kwin still needs another patch.

Test Plan

Select 2000 files in one folder, cut and paste them in another disk.

Diff Detail

Repository
R244 KCoreAddons
Lint
Lint Skipped
Unit
Unit Tests Skipped
jtamate created this revision.Jan 28 2018, 8:57 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 28 2018, 8:57 AM
jtamate requested review of this revision.Jan 28 2018, 8:57 AM
dfaure requested changes to this revision.Jan 28 2018, 10:15 AM

Ah I guess it's faster because QMimeData skips the encoding/decoding via QByteArray when both drag and drop are in the same process? That's not something we can do ourselves (for the "kde uri list" mimetype), unless we call the protected QMimeData::retrieveData, I guess. Hmm.

src/lib/io/kurlmimedata.cpp
76

But then why call this at all, if you don't need the bytearray?
Just make it

if (mimeData->hasUrls())
This revision now requires changes to proceed.Jan 28 2018, 10:15 AM
jtamate updated this revision to Diff 26110.Jan 28 2018, 10:30 AM
  • Use the much faster urls() method from QMimeData

Using hasUrls() and fixed a typo in the comment

dfaure accepted this revision.Jan 28 2018, 10:48 AM
This revision is now accepted and ready to land.Jan 28 2018, 10:48 AM
This revision was automatically updated to reflect the committed changes.
mwolff added a subscriber: mwolff.Jan 30 2018, 3:35 PM
mwolff added inline comments.
src/lib/io/kurlmimedata.cpp
74

if firstMimeType == "text/uri-list" (see above, PreferLocalUrls), it's still going to be slow, no? Should this be handled generally here by introducing a helper that checks the mime type and then uses either urls() directly or call data as before?

also, what happens if PreferLocalUrls is set, but no text/uri-list data is available, could the old way of getting the data for the s_kdeUriListMime have worked more reliably?

jtamate updated this revision to Diff 26238.Jan 31 2018, 8:36 AM

Something like this?

if firstMimeType == "text/uri-list" (see above, PreferLocalUrls), it's still going to be slow, no? Should this be handled >generally here by introducing a helper that checks the mime type and then uses either urls() directly or call data as >before?

also, what happens if PreferLocalUrls is set, but no text/uri-list data is available, could the old way of getting the data >for the s_kdeUriListMime have worked more reliably?

If PreferLocalUrls then firstMimeType = text/url-list
if firstMimeType == text/url-list apply urls() method

The test hash both desktop: and file: urls in the same MimeType and it passes.

dfaure added inline comments.Feb 3 2018, 12:43 AM
src/lib/io/kurlmimedata.cpp
75

This can't be right, it would mean ba is ignored when firstMimeType==text/uri-list (which would mean the call to data() above was for nothing).

Since the method to call (data() or urls()) depends on the mimetype, either the whole idea of swapping the mimetypes has to be dropped, or as Milian suggests, a helper with an if() should encapsulate this (so we can call it in both places) (but that's more string comparisons....).

To make it fast I'd do, well, OK the code wouldn't fit into this margin, let's make a separate RR: https://phabricator.kde.org/D10257