----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128465/#review97486 -----------------------------------------------------------
Just a small nitpick. I like your solution, nicely done! src/kiconloader.cpp (line 341) <https://git.reviewboard.kde.org/r/128465/#comment65759> New line for the opening brace. src/kiconloader.cpp (lines 342 - 345) <https://git.reviewboard.kde.org/r/128465/#comment65760> I don't know if this is as you intent it. The call mLastUnknownIconCheck.start(); starts the timer, but does it "restart" the timer if it was already started after the 5 seconds check? The QElapsedTimer::start and QElapsedTimer::restart functions are implemented differently (see http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qelapsedtimer_unix.cpp for reference). I havent't tested if start and restart behave the same. If the do then your code does what you intent. But then i wonder why QElapsedTimer has two different implementations for the same logic. If start doesn't restart when called again, then your code - after the 5 seconds define - always ends up outside the if. In that case replacing mLastUnknownIconCheck.start(); with mLastUnknownIconCheck.restart(); would make it work as intended. Ahh, vague.. If you look at the qelapsedtimer_generic.cpp then you see that start just calls restart (is that being used for Linux or is qelapsedtimer_unix.cpp being used? This confuses me..). Anyhow, if the generic implementation is used then you're fine with the code as is :) - Mark Gaiser On jul 16, 2016, 9:52 a.m., David Faure wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128465/ > ----------------------------------------------------------- > > (Updated jul 16, 2016, 9:52 a.m.) > > > Review request for KDE Frameworks, Christoph Feck, David Rosca, Michael Pyne, > and Olivier Goffart. > > > Repository: kiconthemes > > > Description > ------- > > Summary: > The code said "unknown icons should be searched for anew each time > [so that installing new icons works]". However this leads to massive > performance issues when opening the file dialog in a dir with many > files for which there is no mimetype icon. > In my case, 293 callgrind.out.* files in one dir (it's ironic that > they would create a huge performance issue...). > > To make both sides happy (installing new icons should still work, but > still unknown icons shouldn't lead to a full disk search every time > icon.isNull() or icon.pixmap() is called), let's re-resolve unknown > icons again only after 5 seconds. > > Benchmark results for loading an unavailable icon : > before: 43 msecs per iteration (1897 disk locations checked) > after: 0.013 msecs per iteration (pixmap found in the on-disk cache) > And the file dialog no longer crawls to a halt in the dir with 293 callgrind > files. > > Test Plan: > > Reviewers: > > Subscribers: > > > Diffs > ----- > > autotests/kiconengine_unittest.cpp 105e86c1e7bc6170c626aa80d34cdb6422acca9c > src/kiconloader.cpp d885318c166f2a996b038218366317615886a14e > > Diff: https://git.reviewboard.kde.org/r/128465/diff/ > > > Testing > ------- > > (described in commit log) > > > Thanks, > > David Faure > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
