Re: [Nautilus-list] Icon directory contents caching

[Alex Larsson <alexl redhat com>]

On Sun, 7 Oct 2001, Darin Adler wrote:

> on 10/5/01 7:49 PM, Alex Larsson at alexl redhat com wrote:
> 
> > Here is a first rough cut at removing all the stats that the icon-factory
> > does.
> 
> I'll have to look closely to be sure, but I think these changes may have
> messed up some of the subtle points of themed vs. non-themed icon search.
> It's important, when looking for variations of an icon, to find variations
> only in the same directory that the master icon is in. I worry that with
> these changes we might again mix things up and get a non-themed large
> version of a themed icon, so display a mix of different style icons for the
> same file. It's important for us to choose what kind of icon is to be used
> in a way that's independent of the size of variation of the icon being
> loaded. This is made (unnecessarily) complex by the "-aa" and other icon
> variation features of the icon factory. The code ought to be structured in a
> way that makes this much more clear. In fact, having the cache might make
> that possible. The existing code can't afford to do the check for which is
> the right icon to use separately from actually loading the icon, but if it
> was simply a hash table lookup it might be OK to do it that way.

It is entierly possible that i messed up the semantics a bit, but i did 
try to make non-intrusive changes.

It is true that the exact semantics of the icon factory code is less than 
crystal clear. I would personally appreciate a separation of the 
icon-choosing parts of it (mapping icon name or mimetype + size + 
current theme -> icon pixmap filename) from the in-core pixbuf caching. 
The mapping part could even be put in a library such as libgnome so that 
other apps could get the same icons without having the dependency on 
NautilusFile etc. 

Even if this were not done i'd love it if the code was made clearer, and 
even more so if the method to look up an icon was clearly documented.
 
> It does seem to me that a hash table is more of a natural than a ptrarray
> for holding a "does this file exist?" data structure. But the real issue is
> speed, and an O(n^2) can be faster than O(n lg n) if the n values are small
> enough.
I think a hash table is the right solution here, it will be faster and 
make the code clearer.
 
> > First of all, where it used to use gnome_vfs_icon_path_from_filename() i
> > now load it manually. This function looks at the files installed in the
> > gnome-prefix. It now uses the hardcoded /usr, but it really should use the
> > gnome-libs prefix, or otherwise the nautilus prefix.
> 
> It's definitely bad to hardcode "/usr". On the other hand, it would be
> slightly bizarre to actually compile in the gnome-libs path at nautilus
> configure time. I just want to do this the "normal" way. Too bad there is no
> normal way.
> 
> What package are these global images coming from? Is it really gnome-libs?
> Or gnome-core? Or multiple packages?

Multiple packages. Each app installs it's own icons there for instance. 
Many icons are from gnome-core though.
 
> > There is a small change in behaviour. When it can't find an icon in the
> > current or default theme, it used to look for icons in the global dir
> > /usr/share/pixmap and /usr/share/pixmap/$theme_name. Now it only looks in
> > /usr/share/pixmap. Is this a problem? I thought the original behaviour was
> > sort of strange.
> 
> If we are looking for a themed icon and we don't find it, we shouldn't look
> anywhere else during the themed icon search pass. If there's no themed icon,
> we'll fall back to the non-theme icon search. The non-theme icon search
> should include the default theme and the global icon places. I agree that
> the global icon places don't really need to allow theme name subdirectories.

Hmm, but this is not what the current code does. Although i guess that is 
the normal effect, since there seldom is a /usr/shar/pixmap/$(theme_name) 
directory.

I agree that that would be the correct method though. I will change this 
tomorrow.
 
> > I didn't change the code for the theme_is_in_user_directory case. Partly
> > because of laziness, but partly because that code seems kind of broken.
> > It always seems to read the default icons from the user directory if
> > theme_is_in_user_directory, ignoring default_theme_is_in_user_directory.
> > It could be made to work with the cache, simplifying the code, if
> > fill_theme_icon_dir_cache() took an theme_is_in_user_directory argument
> > and fill_default_icon_dir_cache() took a
> > default_theme_is_in_user_directory argument.
> 
> While I'm not a fan of the theme_is_in_user_directory feature, I dislike
> making it more broken (not cached) without removing it. I'd like to fix
> these minor problems with theme_is_in_user_directory or remove the feature.
> Ideally we'd do that before putting in this cache. Sorry that I won't have
> time to tackle that today.

I'll fix it tomorrow, taking a closer look at supporting 
default_theme_is_in_user_directory too.

> It would be nice to use functions here to avoid making this gigantic
> function even longer.

Oki. Just converting it to use hash tables goes some of the way.

/ Alex