Created attachment 323095 [details] [review]
Add notification for edited item

Review of attachment 323095 [details] [review]:

Thanks for the nice patch!

::: po/POTFILES.in
@@ +8,3 @@
 src/photos-delete-notification.c
 [type: gettext/glade]src/photos-dlna-renderers-dialog.ui
+src/photos-done-notification.c

Nice!

::: src/photos-done-notification.c
@@ +107,3 @@
+
+  photos_done_notification_destroy (self);
+  g_application_release (app);

We can't release the app, yet. Since edit_done saves the pipeline, any kind of revert needs to call photos_base_item_pipeline_save_async too.

@@ +119,3 @@
+  g_application_hold (app);
+
+  photos_base_item_operations_revert (self->item);

If you had already clicked "done" a few times before, then photos_base_item_operation_revert will revert it to the last snapshot, not the original. That is the same as clicking the "cancel" button.

Here we want to revert to the original. Thinking of it, it is probably better to have a "Discard all edits" button for reverting to the original, and an "Undo" to go back to the previous snapshot, as you have done here. That means one more new string (thinking of the String Freeze), but it can be shared with the properties dialog. I don't know. Let's see how it goes. :)

Anyway, for reverting to the original, we need a revert_to_original method that will clean self->hash, call photos_utils_remove_children_from_node and connect the input and output proxies.

Related to this, jimmac mocked up this "discard all edits" UI for the properties dialog: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/photos/wires-revert-to-original.png

(In reply to Debarshi Ray from comment #2)
> Review of attachment 323095 [details] [review] [review]:
> @@ +119,3 @@
> +  g_application_hold (app);
> +
> +  photos_base_item_operations_revert (self->item);
> 
> If you had already clicked "done" a few times before, then
> photos_base_item_operation_revert will revert it to the last snapshot, not
> the original. That is the same as clicking the "cancel" button.
> 
> Here we want to revert to the original. Thinking of it, it is probably
> better to have a "Discard all edits" button for reverting to the original,
> and an "Undo" to go back to the previous snapshot, as you have done here.
> That means one more new string (thinking of the String Freeze), but it can
> be shared with the properties dialog. I don't know. Let's see how it goes. :)
> 
> Anyway, for reverting to the original, we need a revert_to_original method
> that will clean self->hash, call photos_utils_remove_children_from_node and
> connect the input and output proxies.

I re-read my past conversation with jimmac, and I realize that your implementation is fine. Here is what he had written:
  "As it's not something you'd be doing all that often, I'd imagine having         
that 'revert all edits' in the properties modal as we discussed with the        
GIMP editing. The case where this wouldn't be so nice is doing some edits       
and mistakenly applying them instead of canceling. We could trigger a           
notification about applying edits and provide an undo button there, like we     
do in other apps."

So, what you have here should satisfy the "mistakenly clicked done instead of canceling" case. The revert_to_original method would be needed to implement the "Discard all edits" hammer in the properties dialog. There is no need for "Discard all edits" in the notification.

Note that we still need to save the pipeline (as I mentioned earlier) after revert.

(In reply to Debarshi Ray from comment #3)
> Related to this, jimmac mocked up this "discard all edits" UI for the
> properties dialog:
> https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/
> photos/wires-revert-to-original.png

Nice. I will open a new bug for this.

Created attachment 323209 [details] [review]
Add notification for edited item

Addressing raised points.

Review of attachment 323209 [details] [review]:

Thanks for the new patch, Rafael. It looks really good apart from some minor details:

::: src/photos-done-notification.c
@@ +57,3 @@
+enum
+{
+  DELETE_TIMEOUT = 10 /* s */

s/DELETE/DONE/ :)

@@ +105,3 @@
+    }
+
+  photos_done_notification_destroy (self);

Since we are going to call photos_done_notification_destroy irrespective of whether there is an error or not, I would move this to photos_done_notification_undo_clicked instead of having to deal with it twice. That's similar to what what ExportNotification does in the "empty trash" code path.

@@ +128,3 @@
+    }
+
+  photos_base_item_pipeline_save_async (item, NULL, photos_done_notification_pipeline_save_finish, self);

I think we should do a gtk_widget_queue_draw on the current PhotosImageView widget to ensure that the visible pixels are refreshed to match the state of the pipeline. I see that gtk+ master somehow queues a draw itself, but that is not the case with gtk-3-18. As far as I can tell, this is just a happy coincidence with gtk+ master. There is no guarantee that it won't change.

Since the PhotosImageView widget is entrenched deep inside PhotosPreviewView, a GAction might be an easy way to wire up the redraw.

@@ +141,3 @@
+
+  photos_base_item_operations_revert (self->item);
+  photos_base_item_process_async (self->item, NULL, photos_done_notification_edit_undo_process, self);

It will probably never hurt us, but, still, I would sleep better if we took a ref on self when starting the async operation and unreffed it in the callback. That way we are sure that self won't disappear behind our back.

Having said that, the memory management in all the notification classes are a bit sub-optimal. It would be better to use self->cancellable with all the async operations instead of doing the ref/unref trick. Then we would cancel self->cancellable during destruction, and teach each callback to be careful about G_IO_ERROR_CANCELLED. This is better because we won't be holding extra references to self behind the caller's back, and in-flight operations would be nicely cleaned up once the callers drops its ref. But, we are on the verge of a release, so let's not destabilize things by making such changes right now. :)

@@ +159,3 @@
+
+  name = photos_base_item_get_name_with_fallback (PHOTOS_BASE_ITEM (self->item));
+  msg = g_strdup_printf (_("%s edited"), name);

The %s should be enclosed in “” marks like it is in DeleteNotification and ExportNotification.

@@ +256,3 @@
+photos_done_notification_new (PhotosBaseItem *item)
+{
+  g_return_if_fail (item != NULL);

It would be more strict to assert PHOTOS_IS_BASE_ITEM (item), instead of 'item != NULL'.

@@ +263,3 @@
+                "margin-start", 12,
+                "margin-end", 12,
+                "orientation", GTK_ORIENTATION_HORIZONTAL,

I know where you copied this style from, but it is an anti-pattern. :) I would prefer to pass only those properties to g_object_new that were passed by the client code, because a direct g_object_new call should match the C wrapper as much as possible. The rest should be set "inside" in _init, _constructed, etc..

Created attachment 323291 [details] [review]
Add a GAction to redraw the current PhotosImageView

Created attachment 323292 [details] [review]
Add notification when an item is edited

I took the liberty to update the patch, and reformatted the commit message to not exceed 72 characters per line.

We need permission to commit because we are in String and UI Freeze: https://wiki.gnome.org/ThreePointNineteen

Created attachment 323293 [details]
Screenshot

New user facing string:
  "“%s” edited"

Approved by the release and translation teams! Pushed to master.