Re: [Nautilus-list] thumbnailing-files-from-the-future patch

[Alex Larsson <alexl redhat com>]

On 18 Mar 2002, Havoc Pennington wrote:

> 
> Hi,
> 
> Here's a revised patch.
> 
> Havoc
> 
> Index: nautilus-thumbnails.c
> ===================================================================
> RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-thumbnails.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 nautilus-thumbnails.c
> --- nautilus-thumbnails.c	6 Mar 2002 15:35:58 -0000	1.37
> +++ nautilus-thumbnails.c	18 Mar 2002 20:57:01 -0000
> @@ -52,6 +52,10 @@
>  					   GNOME_VFS_PERM_GROUP_READ | \
>  					   GNOME_VFS_PERM_GROUP_WRITE | \
>  					   GNOME_VFS_PERM_OTHER_READ)
> +
> +/* Should never be a reasonable actual mtime */
> +#define INVALID_MTIME 0
> +
>  /* thumbnail task state */
>  static GList *thumbnails;
>  static char *new_thumbnail_uri;
> @@ -219,33 +223,54 @@ make_thumbnail_uri (const char *image_ur
>  	return thumbnail_uri;
>  }
>  
> -/* utility routine that takes two uris and returns true if the first file has been modified later than the second */
> -/* FIXME bugzilla.gnome.org 42565: it makes synchronous file info calls, so for now, it returns FALSE if either of the uri's are non-local */
>  static gboolean
> -first_file_more_recent (const char* file_uri, const char* other_file_uri)
> +get_file_mtime (const char *file_uri, time_t* mtime)
>  {
> -	gboolean more_recent;
> -	
> -	GnomeVFSFileInfo *file_info, *other_file_info;
> +	GnomeVFSFileInfo *file_info;
>  
> -	/* if either file is remote, return FALSE.  Eventually we'll make this async to fix this */
> -	if (!uri_is_local (file_uri) || !uri_is_local (other_file_uri)) {
> +	if (!uri_is_local (file_uri)) {
> +		*mtime = INVALID_MTIME;
>  		return FALSE;
>  	}
>  	
>  	/* gather the info and then compare modification times */
>  	file_info = gnome_vfs_file_info_new ();
>  	gnome_vfs_get_file_info (file_uri, file_info, GNOME_VFS_FILE_INFO_FOLLOW_LINKS);
> -	
> -	other_file_info = gnome_vfs_file_info_new ();
> -	gnome_vfs_get_file_info (other_file_uri, other_file_info, GNOME_VFS_FILE_INFO_FOLLOW_LINKS);
>  
> -	more_recent = file_info->mtime > other_file_info->mtime;
> +	*mtime = file_info->mtime;

In theory you should look at file_info->valid_fields & GNOME_VFS_FILE_INFO_FIELDS_MTIME 
here. I guess local files will always have mtime though. (FAT does, 
doesn't it?)

  
>  	gnome_vfs_file_info_unref (file_info);
> -	gnome_vfs_file_info_unref (other_file_info);
> +	
> +	return TRUE;
> +}
> +
> +/* utility routine that checks mtime identity on file one and file two, and returns
> + * mtime of the first file. If anything goes wrong, it claims the files have the
> + * same mtime, which is a very thumbnailing-specific kind of fallback.
> + */
> +/* FIXME bugzilla.gnome.org 42565: it makes synchronous file info calls, so for now, it returns FALSE if either of the uri's are non-local */
> +static gboolean
> +files_have_different_mtime (const char* file_uri, const char* other_file_uri, time_t* first_file_mtime)
> +{
> +	gboolean different_mtime;
> +	time_t file_mtime, other_file_mtime;
> +
> +	/* if either file is remote, return FALSE.  Eventually we'll make this async to fix this */
> +
> +	*first_file_mtime = INVALID_MTIME;

You're assigning first_file_mtime unconditionally here, but conditionally 
below.

> +	
> +	/* gather the info and then compare modification times */
> +	if (!get_file_mtime (file_uri, &file_mtime))
> +		return FALSE;
> +	
> +	if (!get_file_mtime (other_file_uri, &other_file_mtime))
> +		return FALSE;
> +
> +	different_mtime = file_mtime != other_file_mtime;
> +	if (first_file_mtime)
> +		*first_file_mtime = file_mtime;

Lacking lots of {'s and }'s here :)
  
> -	return more_recent;
> +	return different_mtime;
>  }
>  
>  /* structure used for making thumbnails, associating a uri with where the thumbnail is to be stored */
> @@ -254,6 +279,7 @@ typedef struct {
>  	char *thumbnail_uri;
>  	gboolean is_local;
>  	pid_t thumbnail_task;
> +	time_t original_file_mtime;
>  } NautilusThumbnailInfo;
>  
>  /* GCompareFunc-style function for comparing NautilusThumbnailInfos.
> @@ -324,11 +350,14 @@ nautilus_get_thumbnail_uri (NautilusFile
>  	NautilusFile *destination_file;
>  	gboolean local_flag = TRUE;
>  	gboolean  remake_thumbnail = FALSE;
> +	time_t file_mtime;
>  	
>  	file_uri = nautilus_file_get_uri (file);
>  		
>  	thumbnail_uri = make_thumbnail_uri (file_uri, FALSE, TRUE, TRUE);
> -		
> +	
> +	file_mtime = INVALID_MTIME;
> +	
>  	/* if the thumbnail file already exists locally, simply return the uri */
>  	
>  	/* note: thumbnail_uri is always local here, so it's not a disaster that we make a synchronous call below.
> @@ -336,7 +365,7 @@ nautilus_get_thumbnail_uri (NautilusFile
>  	if (vfs_file_exists (thumbnail_uri)) {
>  		
>  		/* see if the file changed since it was thumbnailed by comparing the modification time */
> -		remake_thumbnail = first_file_more_recent (file_uri, thumbnail_uri);
> +		remake_thumbnail = files_have_different_mtime (file_uri, thumbnail_uri, &file_mtime);
>  		
>  		/* if the file hasn't changed, return the thumbnail uri */
>  		if (!remake_thumbnail) {
> @@ -360,7 +389,7 @@ nautilus_get_thumbnail_uri (NautilusFile
>  		if (vfs_file_exists (thumbnail_uri)) {
>  		
>  			/* see if the file changed since it was thumbnailed by comparing the modification time */
> -			remake_thumbnail = first_file_more_recent(file_uri, thumbnail_uri);
> +			remake_thumbnail = files_have_different_mtime (file_uri, thumbnail_uri, &file_mtime);
>  		
>  			/* if the file hasn't changed, return the thumbnail uri */
>  			if (!remake_thumbnail) {
> @@ -403,13 +432,23 @@ nautilus_get_thumbnail_uri (NautilusFile
>  	}
>  	
>  	/* the thumbnail needs to be created (or recreated), so add an entry to the thumbnail list */
> - 
> +
> +	if (file_mtime == INVALID_MTIME) {
> +		/* This may fail, but the thumbnailing code handles
> +		 * INVALID_MTIME in the NautilusThumbnailInfo
> +		 */
> +		get_file_mtime (file_uri, &file_mtime);
> +	}
> +	
>  	if (result != GNOME_VFS_OK && result != GNOME_VFS_ERROR_FILE_EXISTS) {
>  		g_warning ("error when making thumbnail directory %d, for %s", result, thumbnail_uri);	
>  	} else {
> -		NautilusThumbnailInfo *info = g_new0 (NautilusThumbnailInfo, 1);
> +		NautilusThumbnailInfo *info;
> +		
> +		info = g_new0 (NautilusThumbnailInfo, 1);
>  		info->thumbnail_uri = file_uri;
>  		info->is_local = local_flag;
> +		info->original_file_mtime = file_mtime;
>  		
>  		if (g_list_find_custom (thumbnails, info, compare_thumbnail_info) == NULL) {
>  			thumbnails = g_list_append (thumbnails, info);
> @@ -562,6 +601,7 @@ make_thumbnails (gpointer data)
>  			NautilusFile *file;
>  			GnomeVFSFileSize file_size;
>  			char *thumbnail_path;
> +			GnomeVFSFileInfo *file_info;
>  			
>  			file = nautilus_file_get (info->thumbnail_uri);
>  			file_size = nautilus_file_get_size (file);
> @@ -590,19 +630,21 @@ make_thumbnails (gpointer data)
>  				/* scale the content image as necessary */	
>  				scaled_image = eel_gdk_pixbuf_scale_down_to_fit (full_size_image, 96, 96);	
>  				g_object_unref (full_size_image);
> -				
> +
>  				thumbnail_path = gnome_vfs_get_local_path_from_uri (new_thumbnail_uri);
> +				
>  				if (thumbnail_path == NULL
>  				    || !eel_gdk_pixbuf_save_to_file (scaled_image, thumbnail_path)) {
>  					g_warning ("error saving thumbnail %s", thumbnail_path);
>  				}
> -				g_free (thumbnail_path);
>  				g_object_unref (scaled_image);
> +				g_free (thumbnail_path);

Is this just a random change?

>  			} else {
>  				/* gdk-pixbuf couldn't load the image, so trying using ImageMagick */
>  				char *temp_str;
> -
> +				
>  				thumbnail_path = gnome_vfs_get_local_path_from_uri (new_thumbnail_uri);
> +				
>  				if (thumbnail_path != NULL) {
>  					temp_str = g_strdup_printf ("png:%s", thumbnail_path);
>  					g_free (thumbnail_path);
> @@ -616,6 +658,21 @@ make_thumbnails (gpointer data)
>  				}
>  				
>  				/* we don't come back from this call, so no point in freeing anything up */
> +			}
> +			
> +			if (info->original_file_mtime != INVALID_MTIME) {
> +				file_info = gnome_vfs_file_info_new ();
> +				file_info->mtime = info->original_file_mtime;
> +				/* we don't care about atime, but gnome-vfs
> +				 * makes us set it along with mtime.
> +				 */

That's bad. gnome-vfs should look at file_info->valid_fields I think. 

Maybe you want to read the old atime and putting it here instead of 
overwriting it with the mtime. If you have a script that deletes old 
thumbnails (or if nautilus later wants to do so) we want the atime to 
reflect when it was last accessed. If you use atime == original images 
mtime then new thumbnails would be deleted immediately. :)

Except i guess nautilus reads the file immediately, so it won't matter 
much...

> +				file_info->atime = info->original_file_mtime;
> +				
> +				gnome_vfs_set_file_info (new_thumbnail_uri,
> +							 file_info,
> +							 GNOME_VFS_SET_FILE_INFO_TIME);
> +							 
> +				gnome_vfs_file_info_unref (file_info);
>  			}
>  			
>  			_exit(0);
> 

Otherwise looks good. Please apply to HEAD.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's an ungodly neurotic matador for the 21st century. She's a tortured 
paranoid opera singer from Mars. They fight crime!