Review Request 128580: If we pass a QIcon as an argument to IconItem::Source, use it

[David Edmundson]

> On Aug. 3, 2016, 7:38 a.m., David Rosca wrote:
> > Why does it break for SNIs? It first tries QIcon::fromTheme and if it fails (isNull()) fallbacks to original QIcon. This patch just switched it, why does it make a difference?
> > 
> > The problem I see here is that if a custom icon loader is used which uses static icon name (let's say "plasma" as we have it in breeze icons), but the actual plasma.png file representing completely different icon. In this case, however, it would fail in the codepath above - being loaded either from plasma theme or svg with kiconloader.
> 
> David Edmundson wrote:
>     The reason it fails, is that QIcon::fromTheme ends up returning the unknown icon. Which is not "null". 
>     and I can't just disable it having a fallback, as maybe it originally was a string which didn't have an icon.
> 
> David Rosca wrote:
>     How come? What is "unknown icon"? Maybe the actual bug is https://quickgit.kde.org/?p=kiconthemes.git&a=commit&h=0abf1b7a148cf6b27caea01a329631e0f1daa983 ?
> 
> Kai Uwe Broulik wrote:
>     The "unknown" icon is the generic sheet of paper thing you usually get when an icon wasn't found.
> 
> David Rosca wrote:
>     But that's not what should QIcon::fromTheme(iconname) return when icon name is invalid. It should just return null icon.

1) You're right. I was missing that patch. I just assumed I had it as I was fixing something in Neon....
It returns name, so it's not "null"... and so on. Sorry about that.

2) This commit still makes sense as it saves getting an icon when we already have one, so I'll leave it in.


- David


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


On Aug. 3, 2016, 11:05 a.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128580/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2016, 11:05 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> Currently the code gets the icon name from the QIcon and tries to do
> some Plasma theming with it.
> However if that fails it then loads the QIcon::fromTheme again.
> 
> This is pointless in most cases and will break any icons that have a
> custom loader (all SNIs)
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/core/iconitem.cpp 29c7f05b5df060df7b362b331f7edc412df12307 
> 
> Diff: https://git.reviewboard.kde.org/r/128580/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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