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

[Aleix Pol Gonzalez]

> On July 25, 2016, 1:58 p.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.

Can you propose a test case to add then?


- Aleix


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


On July 24, 2016, 2: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, 2: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/20160726/067bd989/attachment-0001.html>