Created attachment 318819 [details] [review]
Preserve metadata after exporting https://bugzilla.gnome.org/show_bug.cgi?id=759156

Review of attachment 318819 [details] [review]:

Thanks for the patch, Umang. I see that you have managed to wrap your head around Autotools and gexiv2 on your own. Good.

Some comments:

::: configure.ac
@@ +36,3 @@
 GLIB_MIN_VERSION=2.39.3
 GOA_MIN_VERSION=3.8.0
+GEXIV2_MIN_VERSION=0.6.1

Out of curiosity, is there any special about 0.6.1? How did you decide that 0.6.1 should be the minimum?

::: src/Makefile.am
@@ +311,3 @@
 	$(GFBGRAPH_LIBS) \
 	$(GEGL_LIBS) \
+	$(GEXIV2_LIBS) \

And GEXIV2_CFLAGS should be added to AM_CPPFLAGS.

::: src/photos-application.c
@@ +938,3 @@
+  GFile *export;
+
+  metadata = photos_export_metadata_new (item);

The steps needed to preserve the metadata should be internal details of photos_base_item_save_async because it doesn't make sense to save an item without keeping the metadata.

It should be the last step in the series of operations performed by photos_base_item_save_async . Therefore, it should start from photos_base_item_save_save_to_stream unless there was an error.

::: src/photos-export-metadata.c
@@ +44,3 @@
+typedef struct _PhotosExportMetadataClass	PhotosExportMetadataClass;
+
+G_DEFINE_TYPE (PhotosExportMetadata, photos_export_metadata, GEXIV2_TYPE_METADATA);

Congratulations on creating your first GObject!

In this case, though, I wonder if we really need it. These steps could just go into photos-base-item.c.

@@ +69,3 @@
+	item_path = g_file_get_path (active_item);
+
+	if (! gexiv2_metadata_open_path (GEXIV2_METADATA (self), item_path, &error))

gexiv2_initialize should be called before using gexiv2. I think you didn't notice because you didn't have any XMP metadata.

gexiv2_initialize is different from gegl_init and such. It can fail. Since gexiv2 is only used when a main window is present, I would do something similar to photos_application_sanity_check_gegl in photos_application_create_window.

One big problem with the gexiv2 API today is that it only does synchronous IO. This is a problem because the user's home directory can be network mounted, and even if it isn't, IO operations involving big image files can be noticeably slow. This means that we will have to it asynchronously, and in this case, in a thread.

Secondly, you should use photos_base_item_download[_async] to get the full path of a PhotosBaseItem. Remember that we deal with stuff that can be on Facebook. While we only support editing local items at the moment, let's try to keep the internals as generic as possible. See the rest of the code for examples.

@@ +129,3 @@
+
+	filename = photos_base_item_get_filename (metadata->item);
+	export_path = g_strjoin (NULL, g_file_get_path (export),"/",filename, NULL);

Use g_build_filename instead of g_strjoin.

Created attachment 319330 [details] [review]
Preserve metadata after exporting

Review of attachment 319330 [details] [review]:

::: src/Makefile.am
@@ +282,3 @@
 	$(GFBGRAPH_CFLAGS) \
 	$(GEGL_CFLAGS) \
+	$(GEXIV2_LIBS) \

This is wrong.

I will let you figure out why because I mentioned this once before on this same bug, and at least thrice on IRC - twice just this morning while discussing this chunk of the patch.

Created attachment 319331 [details] [review]
Preserve metadata after exporting

Created attachment 319333 [details] [review]
Preserve metadata after exporting

Apologies if this is redundant information, but for details on what other metadata should be preserved, the MWG guidelines are very useful.

http://www.metadataworkinggroup.org/pdf/mwg_guidance.pdf

Images can also contain camera manufacturer Makernotes and several thumbnail images of various sizes.

http://www.exiv2.org/makernote.html

Review of attachment 319333 [details] [review]:

Thanks for the new patch, Umang. This looks much better now, and, as far as I can tell, works as expected.

Some comments below:

::: src/photos-application.c
@@ +534,3 @@
 
+  gexiv2_initialized = gexiv2_initialize ();
+  g_return_val_if_fail (gexiv2_initialized, FALSE);

The gexiv2 header files should be included. Otherwise the compiler will throw a warning.

::: src/photos-base-item.c
@@ +122,3 @@
 static void photos_base_item_filterable_iface_init (PhotosFilterableInterface *iface);
 
+static gboolean photos_base_item_save_metadata_finish (PhotosBaseItem *self, GAsyncResult *res, GError **error);

This is not needed. See below.

@@ +1066,3 @@
+                                              gpointer source_object,
+                                              gpointer task_data,
+                                              GCancellable *cancellable)

Nitpick. The indentation is off.

@@ +1079,3 @@
+  source_path = photos_base_item_download (self, cancellable, &error);
+  if (error != NULL)
+  {

Ditto.

@@ +1084,3 @@
+  }
+
+  export_path = g_file_get_path (G_FILE (g_task_get_task_data (task)));

You can just use the task_data parameter instead of calling the function.

@@ +1089,3 @@
+  if (! gexiv2_metadata_open_path (metadata, source_path, &error))
+  {
+    g_warning ("Unable to read file for metadata from %s", error->message);

We usually call g_warning when we handle the error/exception. In this case, it is done in src/photos-application.c where photos_base_item_save_finish is called. One alternative to g_warning is to somehow show the error in the UI.

Here we are only bubbling the error upwards, so the g_warning shouldn't be here. If the error message from gexiv2 is not sufficiently clear, we could use g_prefix_error to make it clearer.

@@ +1097,3 @@
+  if (error != NULL)
+  {
+    g_warning ("Unable to save metadata in exported file : %s", error->message);

Ditto.

@@ +1121,3 @@
+  {
+      g_warning ("Error in saving metadata : %s", error->message);
+      g_error_free (error);

We should fail photos_base_item_save_async if there was an error here by using g_task_return_error. Otherwise we should use g_task_return_boolean to indicate success.

@@ +1128,3 @@
+static void
+photos_base_item_save_metadata_async (PhotosBaseItem *self,
+                                      GFile *dir,

s/dir/file/

The rest of the code won't work if a directory gets passed. It needs a regular file.

@@ +1169,3 @@
+  data = (PhotosBaseItemSaveData *) g_task_get_task_data (task);
+
+  dir = g_file_get_child (data->dir, self->priv->filename);

s/dir/file/

g_file_get_child will give you a GFile instance representing the newly created exported file. It's not a directory.

@@ +1177,3 @@
     }
 
   g_task_return_boolean (task, TRUE);

This isn't quite right.

Implementations of asynchronous operations are usually held together by a GTask instance. When one of the g_task_return_* methods are called, it immediately finishes the operation and calls the callback function (if any) given to it by the user/caller/client.

In this case, calling g_task_return_boolean here means that photos_base_item_save_async is finished before the metadata was saved. This is wrong because a failure to save the metadata should fail photos_base_item_save_async. Otherwise the caller will be left in the dark.

We should call g_task_return_boolean (task, TRUE) only after photos_base_item_save_metadata_finish has indicated success. See above.

@@ +1179,2 @@
   g_task_return_boolean (task, TRUE);
+  photos_base_item_save_metadata_async (self, dir, NULL, photos_base_item_metadata_save, user_data);

We should pass task as the user_data of photos_base_item_save_metadata_async and hold a reference to it so that it stays alive across the async call.

Otherwise, the code can crash due to memory errors if you make all the above changes.

Created attachment 319366 [details] [review]
application, base-item: Preserve metadata when saving

I took the liberty to address the above issues and pushed it.

Hey Allan,

(In reply to Alan Pater from comment #7)
> Apologies if this is redundant information, but for details on what other
> metadata should be preserved, the MWG guidelines are very useful.
> 
> http://www.metadataworkinggroup.org/pdf/mwg_guidance.pdf
> 
> Images can also contain camera manufacturer Makernotes and several thumbnail
> images of various sizes.
> 
> http://www.exiv2.org/makernote.html

Thank you. These references are useful.

As far as I can tell, this patch is preserving (as much as supported by exiv2) metadata as expected. I am definitely not an expert in this field, so it will be very helpful if you can try it out and report back. Feel free to file a bug if something is wrong.

Thanks once again!