14:02 <  rfonseca> aday: regarding gphotos, issue #763156, what should happen after "Discard all edits" is clicked? Change the "Edited in Photos" string and hide the button? Remove the whole line?
16:25 <    jimmac> rfonseca: it should say something like "Untouched" or "Unmodified"
17:03 <  rfonseca> jimmac: what about the button?
17:05 <    jimmac> i was pondering an undo, but i'd probably just remove the button

Created attachment 325329 [details] [review]
Add revert to original API

Created attachment 325330 [details] [review]
properties-dialog: Add option to revert edits

For some reason, gtk_widget_hide is not working on the revert button.

Created attachment 326127 [details] [review]
add revert to original API

Accounting for possible changes introduced by #764801.

Created attachment 326128 [details] [review]
properties-dialog: add option to revert edits

In this new version we only disable the "Discard all edits" button instead of hiding it.

Created attachment 326129 [details] [review]
properties-dialog: merge width and height into dimensions.

Created attachment 326130 [details] [review]
properties-dialog: add option to revert all edits

Ops, this is the correct version.

Review of attachment 326127 [details] [review]:

Thanks, Rafael. Looks good, except:

::: src/photos-base-item.c
@@ +2485,3 @@
+photos_base_item_revert_to_original (PhotosBaseItem *self)
+{
+  photos_pipeline_revert_to_original (self->priv->pipeline);

We should implicitly call photos_base_item_process_async, as we did in bug 765105

Review of attachment 326127 [details] [review]:

::: src/photos-base-item.c
@@ +2005,3 @@
+photos_base_item_get_edited (PhotosBaseItem *self)
+{
+  return photos_pipeline_get_edited (self->priv->pipeline);

The pipeline can be NULL if the item hasn't been loaded, yet. Since we don't need the pixel data for this, we can try to only create the pipeline.

Review of attachment 326129 [details] [review]:

Looks fine, except one small detail:

::: src/photos-properties-dialog.c
@@ +502,3 @@
+      gchar *dims_str;
+
+      dims_str = g_strdup_printf ("%"G_GINT64_FORMAT" x %"G_GINT64_FORMAT" pixels", width, height);

Nitpick: would be nice to use the Unicode U+00D7 code point (ie. ×) instead of the 'X' character. See https://wiki.gnome.org/Design/OS/Typography

Created attachment 332729 [details] [review]
properties-dialog: Merge width and height into dimensions

Fixed and pushed.

Review of attachment 326130 [details] [review]:

::: src/photos-properties-dialog.c
@@ +222,3 @@
+
+  app = g_application_get_default ();
+  g_application_hold (app);

Even if we are holding the GApplication, running the async operations with 'self->cancellable' means that they will be cancelled if the dialog is closed before they are complete. Cancelling the process_async isn't so bad, but cancelling the pipeline_save_async means that the changes might not be saved to disk. That can be a bit surprising.

Therefore, the least we should do is not to pass a GCancellable.

However, not using a GCancellable means we can't rely on PropertiesDialog (ie. self) staying alive across the calls. It might be nicer to move the async code to photos-application.c in that case. The dialog can emit a signal to indicate when the button was clicked.

Created attachment 333004 [details] [review]
base-item, pipeline: Add is_edited and revert_to_original API

Created attachment 333006 [details] [review]
application, properties-dialog: Add UI to revert all edits

Fixed and pushed to master. Thanks for the patches, Rafael.