Created attachment 71194 [details] [review]
remove some code that looks fishy

I briefly scanned throug the code and noticed this bit:

>     new->thumburi = gnome_thumbnail_factory_lookup (thumbs,
>                                                    escaped_path,
>                                                    info->mtime);
>    if (new->thumburi == NULL) {
>      gchar * md5sum;
>
>      md5sum = gnome_thumbnail_md5 (escaped_path);
>
>      new->thumburi = g_strconcat (g_get_home_dir (),
>                                  "/.thumbnails/normal/",
>                                  md5sum,
>                                  ".png",
>                                  NULL);
>
>      g_free (md5sum);
>    }

It seems wrong because if the mtime doesn't match it will pull a stale thumbnail from the cache.

Created attachment 71371 [details] [review]
speed things up a bit

attachment 71194 [details] [review] makes things load quite a bit slower because it rethumbnails each image several times.  This one works better.

The patch looks good to me. Rodney, does it for you?

Not really. It just doesn't seem right to me. I'll have to actually go look at the full code I wrote for thumbnailing, again first.

Hi Rodney,

any progress on your review?

So one obvious problem with this patch looking back at it, is that I try to use the escaped_path variable immediately after freeing it.

Created attachment 76694 [details] [review]
a better patch

So this patch 
1) removes the code that tries to pull from ~/.thumbnails directly                 without checking mtime
2) will only generate a new thumbnail if a lookup shows there isn't one already
3) reloads the thumbnail after saving it to pick up any metadata it added to the image
4) never loads a full background just to find out its size
5) moves a g_object_unref (pixbuf) so that it won't ever get passed NULL

Looks ok now. Please commit this Ray. Thanks.

Patch committed