QIcon::fromTheme(xxx, someFallback)

[Olivier Goffart]

On Sunday 23. August 2015 12:01:38 David Faure wrote:
> On Sunday 23 August 2015 10:30:13 Olivier Goffart wrote:
> > On Friday 21. August 2015 22:30:37 David Faure wrote:
> > > > I would also approve a change to Qt to add an overload of
> > > > QIcon::fromTheme
> > > > with only one argument without a fallback. (this would be binary
> > > > compatible, and source compatible unless someone took the address of
> > > > QIcon::fromTheme)
> > > 
> > > I thought about that, but that would change behavior of existing code,
> > > no?
> > > 
> > > Currently: QIcon::fromTheme("foo").isNull() is true if "foo" isn't
> > > found.
> > > If we add a one-arg overload: QIcon::fromTheme("foo").isNull() would be
> > > false, and QIcon::fromTheme("foo", QIcon()).isNull() would be true. The
> > > old
> > > behavior (for these rare apps that care) would require adding an
> > > explicit
> > > null-icon second arg. I wish this would have been done this way from the
> > > start, but we can't make that change now.
> > 
> > Ah ok, i first tought that the problem was the call to availableSizes.
> > But other problem is also the fact that we want to return a null icon if
> > not found.
> 
> It's the same problem, the QIcon code calls availableSizes in order to
> determine if it should return the (null by default) fallback ;)
> 
> > Fo this mean we could change QIcon::isNull fo be something like
> > 
> > if (!d) return true;
> > if (d->notNull) return false;
> > bool null;
> > d->virtualHook(CheckNull, &null);
> > if (null) {
> > 
> > 	const_cast(this)->operator=(QIcon())
> > 
> > } else {
> > 
> >    d->notNull = true;
> > 
> > }
> > return null;
> > 
> > 
> > So than isNull is also lazy, and the problem is solved... is it not?
> 
> Ah!! Yes, that would work. Can you make the change in Qt, since you know
> that code best?

Yes, 

Here are the patches:
https://codereview.qt-project.org/125244/
https://codereview.qt-project.org/125245/

So these patch will ensure that the icons are loaded lazily and that only 
icons that are meant to be shown will query the file system.

But the problem is that QIcon::isNull is likely to be called anyway.
And this will again do all the look ups in the file system.

The problem is that the current persistent cache is at the wrong level as it 
caches the image data. It would be better i think if we had a cache that 
cached   icon name -> list of files for each sizes.
That's what the gtk icon cache does (well, it does both I think).

We could try to use the gtk cache, But i don't think it's good to depend on 
their format. Or should augment the current cache with a mapping to the 
availables icons.

And should we do that in Qt or in the KIconLoader?



> > > Well, there is one solution: QIcon("foo"), the existing fileName ctor,
> > > if we add the fact that "if not found in the current dir then the icon
> > > will be looked up in the theme". That means one stat() per QIcon,
> > > but that's nothing compare to the current QIcon::fromTheme().
> > 
> > That could be a solution.
> > 
> > > And from the documentation, it looks like QIcon(QString) already
> > > behaves the optimal way, i.e. you don't get isNull() if the file doesn't
> > > exist, because "The file will be loaded on demand." right?
> > > That's exactly what I want for icons loaded from a theme, and
> > > which QIcon::fromTheme() doesn't allow implementing, due
> > > to its requirement to immediately find out if a null icon
> > > (or a specific fallback) should be returned instead.
> > 
> > And I think this behaviour is also confusing.
> 
> Not sure what you meant here. QIcon("file") should be null if
> ./file doesn't exist?

Yes.  Don't you think?

-- 
Olivier