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

[David Rosca]

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



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 Rosca


On Aug. 2, 2016, 9:11 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128580/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2016, 9:11 p.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/93121e9a/attachment-0001.html>