[Development] Some QIcon::fromTheme() enhancements

[Antti Kaijanmäki]

On 23.08.2013 11:32, Olivier Goffart wrote:
> Stable means it does not change much.

I was very careful not to introduce any big changes. I'm only a default 
path and providing a fallback in the case that the normal loading from 
theme directories fails. I'm not touching the way theme directories are 
travelled or how the engine handles the search or matching of the found 
icons or the icon cache.

As I stated in my original mail there is a bigger problem of QIconLoader 
not being able to handle themes expanding over multiple base dirs and 
fixing that is clearly something for dev.


> The bug was already there since the beginning of the feature in Qt 4.7.  They
> already had to wait so many years, they can wait a few moth more.
>
> However, a regression in Qt 5.1.2 would be much more damageable.  Maybe there
> is a broken small .xpm in one style that makes an otherwise good png in
> another theme not taken. Maybe this happens to be a huge performance
> bottleneck with some setup, maybe there is a bug in XPM parsing. Or any other
> problem we don't think about now.

If there is a broken XPM in some of the themes then it's a bug in the 
theme and not Qt. The worst thing that could happen is that you get a 
low resolution XPM icon instead of a PNG one (from the parent theme), 
but that would mean that the XPM file has been put to the theme on 
purpose to override the parent PNG.

And then again if there is a bug in XPM parsing then it's more severe 
bug that needs to be addressed. QIcon uses XPM files in it's examples so 
XPM support is a first class citizen in Qt.


> You would have to have good arguments that this bug is urgent enough.
> Following a specification is important of course, but it is only some details
> about it and it does not seem an urgent issue to me.

My only argument would be that QIcon::fromTheme() does not do what it's 
documented to do. That's partly because the documentation does not 
actually describe what is implemented and what is not. All it says is that

===
Returns the QIcon corresponding to name in the current icon theme. If no 
such icon is found in the current theme fallback is returned instead.

The latest version of the freedesktop icon specification and naming 
specification can be obtained here:

 
http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html
 
http://standards.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
===

These changes bring the functionality of the function closer to what 
it's documented to do with very little changes.

I don't have any strong feelings about getting this to stable. dev is 
also fine, but I don't see a reason why this could not be included in 
stable. The chances of introducing regression is IMO very low.


  -- Antti