[Nautilus-list] long-standing icon-factory bug

[Alex Larsson <alexl redhat com>]

While working on resolving the SVG scaling issues i came upon a really 
subtle bug that made nautilus a lot larger than it needed.

In nautilus-icon-factory.c::load_icon_for_scaling() we have this code to 
calculate the max size of the icon:
 size_request.maximum_width = MAXIMUM_ICON_SIZE * requested_size / NAUTILUS_ZOOM_LEVEL_STANDARD;

My take on this code is that it sets one max size for each zoom level 
nominal size, in order to not store the same pixmap in the cache with only 
the max size different.

Do you see the bug though? It's very subtle. It should be:
size_request.maximum_width = MAXIMUM_ICON_SIZE * requested_size / NAUTILUS_ICON_SIZE_STANDARD;

So instead of multiplying the requested size by 2 (basically scaling the 
normal max size with the zoom level) it uses the enum value for the zoom 
level! So it ends up multiplying by 96/3 = 32. Eventually this item will 
be returned to the top level get_icon_from_cache() that will store a copy 
in the cache with the requested max size (which typically is nominal*2), 
so instead of using the old cached value we store a duplicate!!!

Before fix:
20742 alex      11   0 18928  18M  9112 S     4.9  3.6   0:04 nautilus

After fix:
17986 alex      11   0 14788  14M  9104 S     0.0  2.8   0:04 nautilus

Woops! Apparently this saved 4 megs of unused pixmap cache. And this is 
measured directly after startup having rendered my home dir.

The fix was a bit more complicated due to the fact that the old code never 
handled the fact that the inner get_icon_from_cache() call could return 
the "same" icon as the outer call. In fact, there was even a g_assert() 
that this was not the case.

I'm attaching the patch I checked in. I'm reasonably certain that it is 
correct, and I did test it some. Nevertheless i would like for someone 
else to verify the correctness of the ref-counting, as we don't want to 
leak stuff in the cache.

I now have a plan of attack for the remaining SVG issues, namely that the 
svg icons (especially emblems) are rendered to large and then scaled down, 
and that we need scalable embedded text and emblem attach point 
definitions. I'm to tired to do that tonight though.

/ Alex

Index: nautilus-icon-factory.c
===================================================================
RCS file: 
/cvs/gnome/nautilus/libnautilus-private/nautilus-icon-factory.c,v
retrieving revision 1.257
diff -u -p -r1.257 nautilus-icon-factory.c
--- nautilus-icon-factory.c	2002/02/25 21:05:55	1.257
+++ nautilus-icon-factory.c	2002/02/27 04:20:42
@@ -1603,7 +1603,7 @@ load_icon_for_scaling (NautilusScalableI
 		REQUEST_PICKY_BY_NAME_SECOND_CHOICE
 	};
 
-	size_request.maximum_width = MAXIMUM_ICON_SIZE * requested_size / NAUTILUS_ZOOM_LEVEL_STANDARD;
+	size_request.maximum_width = MAXIMUM_ICON_SIZE * requested_size / NAUTILUS_ICON_SIZE_STANDARD;
 	size_request.maximum_height = size_request.maximum_width;
 
 	for (i = 0; i < G_N_ELEMENTS (requests); i++) {
@@ -1945,13 +1945,20 @@ get_icon_from_cache (NautilusScalableIco
 
 		/* Create the key and icon for the hash table. */
 		key = g_new (CacheKey, 1);
-		nautilus_scalable_icon_ref (scalable_icon);
 		key->scalable_icon = scalable_icon;
 		key->size = *size;
 
+		/* recursive get_icon_from_cache might already have placed icon
+		 * in hash table if there was an exact match */
+		if (g_hash_table_lookup (hash_table, key) != NULL) {
+			g_free (key);
+			return icon;
+		}
+		
 		/* Add the item to the hash table. */
-		g_assert (g_hash_table_lookup (hash_table, key) == NULL);
+		nautilus_scalable_icon_ref (scalable_icon);
 		g_hash_table_insert (hash_table, key, icon);
+			
 	}
 
 	/* Hand back a ref to the caller. */