Review Request 128514: KIconLoader: reduce number of lookups when doing fallbacks

[David Faure]

> On July 25, 2016, 11:58 a.m., Anthony Fieroni wrote:
> > src/kiconloader.cpp, line 1041
> > <https://git.reviewboard.kde.org/r/128514/diff/1/?file=472255#file472255line1041>
> >
> >     This is not correct. When you have genericFallback && !currentName.empty() you never reach line 1071 to get an icon. So your code returns an empty path for non empty currentName in genericFallback. If this can be expected, i'm not familliar with path expectations, you can make exclusive check before 2 loops, i.e. before foreach.
> 
> Christoph Feck wrote:
>     I guess the while condition is a left-over from truncating the fallback hierarchy. Your comment makes sense, but I am not sure if empty names actually make it to this function in the first place (it is not public API).
>     
>     *Update:* KIconLoader::iconPath already makes sure the name is not empty, but when you call QIcon::fromTheme("-x"), it would end up looking an empty file name.
> 
> Aleix Pol Gonzalez wrote:
>     Can you propose a test case to add then?

Anthony: if genericFallback is true, then in fact we have no more fallback to do, so it is expected that we return empty then. I can add an early return to make this clearer, but the bool genericFallback is still needed so we don't keep looping when going from e.g. video-ogg to video-x-generic, then stripping the -x-generic again, just to re-add it, and so on.

Christoph: I don't really think it's worth handling QIcon::fromTheme("-x"), it'll never happen :-) But even so, I don't see where it would end up looking an empty filename. After chop(2) we go to a "if isEmpty() break" test.

Aleix: without adding some kind of introspection hook, it's rather difficult to test that the code isn't looking up stupid things that don't exist, like an empty filename (from the outside the result is the same, not found).


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128514/#review97829
-----------------------------------------------------------


On July 24, 2016, 12:27 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128514/
> -----------------------------------------------------------
> 
> (Updated July 24, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> -------
> 
> In the first pass, we already checked whether the icon exists in all icon themes.
> So in the second pass, there's no point in starting with that again, we can go
> ahead and start doing fallbacks right away.
> 
> (The comment about the two passes was removed in b84858c, but some of it still
> applies.)
> 
> Tested with: strace -e file ./kiconengine_unittest testUnknownIconNotCached |& wc -l
> Before: 6341
> After: 4589
> 
> 
> Diffs
> -----
> 
>   autotests/kiconloader_benchmark.cpp ded5e0221f8126139a019a8310e8f120672ac3b4 
>   src/kiconloader.cpp 951775d77b7ec0e2d8099ee192ca25321dab4544 
> 
> Diff: https://git.reviewboard.kde.org/r/128514/diff/
> 
> 
> Testing
> -------
> 
> strace as mentionned above.
> 
> All unittests still pass (incl fallback lookups, didn't check how complete those were, though)
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160730/cc06e97c/attachment.html>