Re: [PATCH] Exif support for image_properties

[Alexander Larsson <alexl redhat com>]

On Thu, 2003-10-09 at 14:13, Hugo wrote:
> Hey All,
> 
> At home i regularly use EXIF tag in JPEG images. I wanted to view these
> tags from nautilus so i decided to hack in support for EXIF in the
> properties dialog. The most likely place was the image_properties dialog
> imho. The attached patch is the result of my hacking. I've used it for a
> few days now and it works ok for me. For those who are not familiar with
> EXIF, its a method of including details about a digital photo with the
> jpeg file. Stuff like flash level, exposure level en ISO rating.
> 
> Is there a chance that this patch can be included in the nautilus
> distribution? If yes, is there someone who wants to help me get this
> patch ready for inclusion?
> 
> To use the patch libexif 0.5.12 must be installed on the system.

Really cool. I've always wanted to add this (in fact, its the reason I
added the image properties page at all).

Some comments about the patch:

+#ifdef HAVE_EXIF
+  /* Copy from exif-loader.c */
+  typedef enum {
+          EL_READ = 0,
+          EL_READ_SIZE_HIGH_BYTE,
+	  EL_READ_SIZE_LOW_BYTE,
+	  EL_SKIP_BYTES,
+	  EL_EXIF_FOUND,
+	  EL_FAILED
+  } ExifLoaderState;
+
+struct _ExifLoader {
+        ExifLoaderState state;
+ 
+        int      size;
+        int      last_marker;
+        unsigned char *buf;
+        int      bytes_read;
+        unsigned int ref_count;
+};
+#endif

Why is this needed at all? Is there no other way to get the LoaderState
from the ExifLoader object? This seems very fragile and could break when
libexif gets upgraded. There must be a non-invasive way to see if the
ExifLoader found something.

+        char *utf_attribute = NULL;
+        char *utf_value = NULL;

The Nautilus coding standards (docs/style-guide.html) doesn't allow
assignment in declarations.

+        if (!attribute) return;

Always use indentation and brackets.

+        }
+        else {

else goes on the same line as the }

+                utf_attribute = g_locale_to_utf8 (attribute, -1, NULL,
NULL, NULL);

This can fail, you should probably use eel_make_valid_utf8 as a last
fallback. Maybe put the whole three-stage to-utf8 conversion part into a
function.

+		/* create and show the widget */
+		gtk_label_set_text(GTK_LABEL (view->details->exifwidget),_("EXIF data"));
+		view->details->exiflistwidget = gtk_scrolled_window_new(NULL, NULL);
+		gtk_scrolled_window_set_policy(GTK_SCROLLED_WINDOW(view->details->exiflistwidget), 

(and other places) 
Always put spaces before paranthesis in function and macro calls.

+#ifdef HAVE_EXIF
+	if (!view->details->got_exif) {
+		gtk_label_set_text (GTK_LABEL (view->details->exifwidget), _("No EXIF data found"));
+	}
+#endif /*HAVE_EXIF*/

I'm not sure about this. First of all we should probably not expose the
name EXIF in the ui. Secondly it'll show up for all images, including
non-jpegs. Its' not like we're saying "No IFF text hunk found" for
jpegs. If there is nothing there, we obviously didn't find any more
data.

+#ifndef HAVE_EXIF
 		} else if (!view->details->got_size) {
 			gnome_vfs_async_read (view->details->vfs_handle,
 					      view->details->buffer,
@@ -119,9 +279,22 @@ file_read_callback (GnomeVFSAsyncHandle 
 					      file_read_callback,
 					      view);
 			return;
+#else /*HAVE_EXIF*/
+		} else if (!view->details->got_size && 
+			   ((view->details->exifldr->state != EL_FAILED) &&
+			    (view->details->exifldr->state != EL_EXIF_FOUND))){
+			gnome_vfs_async_read (view->details->vfs_handle,
+					      view->details->buffer,
+					      sizeof (view->details->buffer),
+					      file_read_callback,
+					      view);
+			return;

You can make the #ifdef area smaller. Something like this would be
easier to maintain:
} else if (!view->details->got_size
#ifdef HAVE_EXIF
	   && (view->details->exifldr->state != EL_FAILED &&
	   view->details->exifldr->state != EL_EXIF_FOUND)
#endif
	   ) {

+	view->details->exifwidget = gtk_label_new(_("loading exif data..."));

Isn't the "loading..." string enough? I guess it could be changed to the
resolution string earlier than the exif data is finished, but we should
just change that, so we change the dialog only once.

Also, I don't actually have libexif installed on this box. Care to post
a small screenshot of how it looks? From the code I think we should
probably make a few changes. I bet the exif attribute names are all
scary, we should use good real translated strings for them instead.
Also, we should perhaps not show every exif attribute in the file?
Aren't there a few that are sort of unnecessary? (need to look at that
screenshot. :)
And I think the presentation should probably be like the current
resolution string instead of a list.





=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a time-tossed Republican vampire hunter from a doomed world. She's a 
wealthy cigar-chomping mechanic from beyond the grave. They fight crime!