D10155: Use the much faster urls() method from QMimeData
dfaure added inline comments.
INLINE COMMENTS
> kurlmimedata.cpp:75
> +QByteArray ba = mimeData->data(firstMimeType);
> +if (ba.isEmpty() || firstMimeType == QStringLiteral("text/uri-list")) {
> // Extracting uris from text/uri-list, use the much faster QMimeData
> method urls()
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
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D10155
To: jtamate, #frameworks, dfaure
Cc: mwolff, michaelh, ngraham
D10155: Use the much faster urls() method from QMimeData
jtamate updated this revision to Diff 26238. jtamate added a comment. 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. REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10155?vs=26113&id=26238 REVISION DETAIL https://phabricator.kde.org/D10155 AFFECTED FILES src/lib/io/kurlmimedata.cpp To: jtamate, #frameworks, dfaure Cc: mwolff, michaelh, ngraham
D10155: Use the much faster urls() method from QMimeData
mwolff added inline comments.
INLINE COMMENTS
> kurlmimedata.cpp:74
> }
> QByteArray ba = mimeData->data(QString::fromLatin1(firstMimeType));
> if (ba.isEmpty()) {
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?
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D10155
To: jtamate, #frameworks, dfaure
Cc: mwolff, michaelh
D10155: Use the much faster urls() method from QMimeData
This revision was automatically updated to reflect the committed changes. Closed by commit R244:18e4d245d3d5: Use the much faster urls() method from QMimeData (authored by jtamate). REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10155?vs=26110&id=26113 REVISION DETAIL https://phabricator.kde.org/D10155 AFFECTED FILES src/lib/io/kurlmimedata.cpp To: jtamate, #frameworks, dfaure Cc: michaelh
D10155: Use the much faster urls() method from QMimeData
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH paste (branched from master) REVISION DETAIL https://phabricator.kde.org/D10155 To: jtamate, #frameworks, dfaure Cc: michaelh
D10155: Use the much faster urls() method from QMimeData
jtamate updated this revision to Diff 26110. jtamate added a comment. - Use the much faster urls() method from QMimeData Using hasUrls() and fixed a typo in the comment REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10155?vs=26096&id=26110 BRANCH paste (branched from master) REVISION DETAIL https://phabricator.kde.org/D10155 AFFECTED FILES src/lib/io/kurlmimedata.cpp To: jtamate, #frameworks, dfaure Cc: michaelh
D10155: Use the much faster urls() method from QMimeData
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
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.
INLINE COMMENTS
> kurlmimedata.cpp:76
> if (ba.isEmpty()) {
> ba = mimeData->data(QString::fromLatin1(secondMimeType));
> +// If extracting uris from test/uri-list, use the much faster
> QMimeData method urls()
But then why call this at all, if you don't need the bytearray?
Just make it
if (mimeData->hasUrls())
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D10155
To: jtamate, #frameworks, dfaure
Cc: michaelh
D10155: Use the much faster urls() method from QMimeData
jtamate created this revision. jtamate added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION 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. REPOSITORY R244 KCoreAddons BRANCH paste (branched from master) REVISION DETAIL https://phabricator.kde.org/D10155 AFFECTED FILES src/lib/io/kurlmimedata.cpp To: jtamate, #frameworks, dfaure Cc: michaelh
