Differential D10085 Diff 26245 src/views/draganddrophelper.cpp

Changeset View

Standalone View

View Options

src/views/draganddrophelper.cpp

Context not available.
29#include <KIO/DropJob> 29​#include <KIO/DropJob>
30#include <KJobWidgets> 30​#include <KJobWidgets>
31 31​
32 ​
33bool DragAndDropHelper::urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl) 32​bool DragAndDropHelper::urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl)
34{ 33​{
35 return std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) { 34​ // This method is quite expensive and can be called several times. Use a cache.
35​ static bool lastResult = false;
36​ static const QList<QUrl>* lastUrls = nullptr;
elvisangelaccioUnsubmitted
Done

Please use constFind()

elvisangelaccio: Please use `constFind()`
37​ static const QUrl* lastDestUrl = nullptr;
38​
39​ // If checking the same data as last time, return the same resutl.
40​ if (&urls == lastUrls && &destUrl == lastDestUrl) {
41​ return lastResult;
42​ }
43​ // otherwise do the expensive check and store the results
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…
44​ lastResult = std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) {
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…
36 return url.matches(destUrl, QUrl::StripTrailingSlash); 45​ return url.matches(destUrl, QUrl::StripTrailingSlash);
37 }) != urls.constEnd(); 46​ }) != urls.constEnd();
47​ lastUrls = &urls;
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…
48​ lastDestUrl = &destUrl;
49​
50​ return lastResult;
38} 51​}
39 52​
40KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event, QWidget* window) 53​KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event, QWidget* window)
Context not available.
elvisangelaccioUnsubmitted
Done

Unrelated whitespace change

elvisangelaccio: Unrelated whitespace change