Differential D10085 Diff 26498 src/views/draganddrophelper.cpp

Changeset View

Standalone View

View Options

src/views/draganddrophelper.cpp

Show All 23 Lines
24#include <QDBusMessage> 24​#include <QDBusMessage>
25#include <QDBusConnection> 25​#include <QDBusConnection>
26#include <QDropEvent> 26​#include <QDropEvent>
27#include <QMimeData> 27​#include <QMimeData>
28 28​
29#include <KIO/DropJob> 29​#include <KIO/DropJob>
30#include <KJobWidgets> 30​#include <KJobWidgets>
31 31​
32​QHash<QUrl, bool> DragAndDropHelper::m_cacheUrlListMatchesUrl;
32 33​
33bool DragAndDropHelper::urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl) 34​bool DragAndDropHelper::urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl)
34{ 35​{
35 return std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) { 36​ auto iteratorResult = m_cacheUrlListMatchesUrl.find(destUrl);
elvisangelaccioUnsubmitted
Done

Please use constFind()

elvisangelaccio: Please use `constFind()`
37​ // if m_cacheUrlListMatchesUrl does not contains destUrl
38​ if (iteratorResult == m_cacheUrlListMatchesUrl.end()) {
39​ // return the value inserted, that is calculated
40​ return *m_cacheUrlListMatchesUrl.insert(destUrl,
41​ // Iterating the list until a node matches destUrl
42​ std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) {
36 return url.matches(destUrl, QUrl::StripTrailingSlash); 43​ return url.matches(destUrl, QUrl::StripTrailingSlash);
37 }) != urls.constEnd(); 44​ // and comparing it with the imaginary end of the list
anthonyfieroniUnsubmitted
Done

Don't do that. It's ridiculous, it cannot guarantied that function will be called with same objects. It's enough speed-up with concurrent calls.

anthonyfieroni: Don't do that. It's ridiculous, it cannot guarantied that function will be called with same…
45​ }) != urls.constEnd() );
46​ }
47​ return *iteratorResult;
elvisangelaccioUnsubmitted
Done

This is really hard to read, and the comments only add more noise. How about doing it like this:

bool DragAndDropHelper::urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl)
{
    auto iteratorResult = m_cacheUrlListMatchesUrl.constFind(destUrl);
    if (iteratorResult != m_cacheUrlListMatchesUrl.constEnd()) {
        return *iteratorResult;
    }

    const bool destUrlMatches = (std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) {
        return url.matches(destUrl, QUrl::StripTrailingSlash);
    }) != urls.constEnd());

    return *m_cacheUrlListMatchesUrl.insert(destUrl, destUrlMatches);
}
elvisangelaccio: This is really hard to read, and the comments only add more noise. How about doing it like this…
38} 48​}
markgUnsubmitted
Done

You now have a double lookup in the m_cacheUrlListMatchesUrl hash. It's fast, so not that big of a deal.
But since we're in the optimizing mood here.. :)

auto result = m_cacheUrlListMatchesUrl.find(destUrl);
if (result != m_cacheUrlListMatchesUrl.end()) {
  return *m_cacheUrlListMatchesUrl.insert(destUrl,
    std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) {
      return url.matches(destUrl, QUrl::StripTrailingSlash);
    }) != urls.constEnd() );
} else {
  return *result;
}

The if statement is _really_ ugly now :P
Something along those lines should work.
Note that you can directly return from the insert, it gives an iterator back and that is - if i'm right - the iterator of the just inserted data.

markg: You now have a double lookup in the m_cacheUrlListMatchesUrl hash. It's fast, so not that big…
39 49​
40KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event, QWidget* window) 50​KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event, QWidget* window)
41{ 51​{
42 const QMimeData* mimeData = event->mimeData(); 52​ const QMimeData* mimeData = event->mimeData();
43 if (mimeData->hasFormat(QStringLiteral("application/x-kde-ark-dndextract-service")) && 53​ if (mimeData->hasFormat(QStringLiteral("application/x-kde-ark-dndextract-service")) &&
44 mimeData->hasFormat(QStringLiteral("application/x-kde-ark-dndextract-path"))) { 54​ mimeData->hasFormat(QStringLiteral("application/x-kde-ark-dndextract-path"))) {
45 const QString remoteDBusClient = mimeData->data(QStringLiteral("application/x-kde-ark-dndextract-service")); 55​ const QString remoteDBusClient = mimeData->data(QStringLiteral("application/x-kde-ark-dndextract-service"));
46 const QString remoteDBusPath = mimeData->data(QStringLiteral("application/x-kde-ark-dndextract-path")); 56​ const QString remoteDBusPath = mimeData->data(QStringLiteral("application/x-kde-ark-dndextract-path"));
47 57​
48 QDBusMessage message = QDBusMessage::createMethodCall(remoteDBusClient, remoteDBusPath, 58​ QDBusMessage message = QDBusMessage::createMethodCall(remoteDBusClient, remoteDBusPath,
49 QStringLiteral("org.kde.ark.DndExtract"), QStringLiteral("extractSelectedFilesTo")); 59​ QStringLiteral("org.kde.ark.DndExtract"), QStringLiteral("extractSelectedFilesTo"));
50 message.setArguments({destUrl.toDisplayString(QUrl::PreferLocalFile)}); 60​ message.setArguments({destUrl.toDisplayString(QUrl::PreferLocalFile)});
51 QDBusConnection::sessionBus().call(message); 61​ QDBusConnection::sessionBus().call(message);
52 } else { 62​ } else {
53 if (urlListMatchesUrl(event->mimeData()->urls(), destUrl)) { 63​ if (urlListMatchesUrl(event->mimeData()->urls(), destUrl)) {
54 return nullptr; 64​ return nullptr;
55 } 65​ }
56 ​
elvisangelaccioUnsubmitted
Done

Unrelated whitespace change

elvisangelaccio: Unrelated whitespace change
57 // Drop into a directory or a desktop-file 66​ // Drop into a directory or a desktop-file
58 KIO::DropJob *job = KIO::drop(event, destUrl); 67​ KIO::DropJob *job = KIO::drop(event, destUrl);
59 KJobWidgets::setWindow(job, window); 68​ KJobWidgets::setWindow(job, window);
60 return job; 69​ return job;
61 } 70​ }
62 71​
63 return nullptr; 72​ return nullptr;
64} 73​}
65 74​
75​void DragAndDropHelper::clearCacheUrlListMatchesUrl()
76​{
77​ DragAndDropHelper::m_cacheUrlListMatchesUrl.clear();
78​}
79​