There is some discussion at https://bugzilla.gnome.org/show_bug.cgi?id=661599

Created attachment 280138 [details] [review]
Enhancement: Enable deletion of photos

Created attachment 280274 [details] [review]
Enhancement: Enable deletion of photos

Created attachment 280279 [details] [review]
Enhancement: Enable deletion of photos

Review of attachment 280279 [details] [review]:

Thanks for the patch, Pranav!

::: src/photos-base-item.c
@@ +342,3 @@
+{
+  /* Subclasses must provide the implementation if they allows deletion */
+}

I would not define a default that is no-op because we should not offer the UI in the first place if an item can't be deleted, and we have can_trash to determine that.

@@ -1048,3 @@
-{
-  return self->priv->collection;
-}

Instead of using a separate virtual method, we can define it as 'PHOTOS_BASE_ITEM_GET_CLASS (self)->delete != NULL'

That would make it easier to implement new subclasses of BaseItem, and avoid logical errors. I know that gnome-documents uses virtual methods, but in C we can be slightly clever.

::: src/photos-base-item.h
@@ +77,3 @@
+  void      (*delete)                  (PhotosBaseItem *self);
+  void      (*set_favorite)            (PhotosBaseItem *self, gboolean favorite);
+  void      (*update_type_description) (PhotosBaseItem *self);

Please use a separate commit for style / formatting changes.

::: src/photos-delete-notification.c
@@ +258,3 @@
+  PhotosDeleteNotificationPrivate *priv = self->priv;
+
+  priv->deleted_objects = objects;

Please use a separate G_CONSTRUCT_ONLY property for objects and use g_list_deep_copy with g_object_ref to copy them in set_property.

@@ +259,3 @@
+
+  priv->deleted_objects = objects;
+  photos_delete_notification_display (self, _("Selected item(s) have been deleted"), NULL);

Umm... why can't this be in _init? Use _constructed if you need the value of priv->deleted_objects.

Review of attachment 280279 [details] [review]:

::: src/photos-local-item.c
@@ +125,3 @@
+  file = g_file_new_for_uri (uri);
+  g_file_trash (file, NULL, NULL);
+}

It does not handle deletion of collections.

Review of attachment 280279 [details] [review]:

::: src/photos-local-item.c
@@ +123,3 @@
+
+  uri = photos_base_item_get_uri (item);
+  file = g_file_new_for_uri (uri);

This needs to be unref-ed and we should use the async API to minimize blocking. Think of /home mounted over the network, slow disks, etc..

Created attachment 281318 [details] [review]
base-item: Make trash a virtual method and implement it for local items

I split out the back-end portions of the previous patch. Instead of defining a separate delete vfunc, I reused the existing trash method, and defined can_trash in terms of whether trash is implemented or not.

Review of attachment 280279 [details] [review]:

::: src/photos-delete-notification.c
@@ +122,3 @@
+static gboolean
+photos_delete_notification_timeout (PhotosDeleteNotification *self)
+{

The self needs to be typecast from a gpointer. See below.

@@ +137,3 @@
+
+  if (priv->on_display)
+    photos_delete_notification_delete_objects (self);

Is priv->on_display really useful?

@@ +140,3 @@
+
+  if (priv->closed)
+    return;

Ditto. Doesn't look useful.

@@ +148,3 @@
+  priv->timeout_id = g_timeout_add_seconds_full (G_PRIORITY_DEFAULT,
+                                                 DELETE_TIMEOUT,
+                                                 photos_delete_notification_timeout,

This will generate a warning.

@@ +212,3 @@
+  context = gtk_widget_get_style_context (priv->secondary_label);
+  gtk_style_context_add_class (context, "dim-label");
+  gtk_container_add (GTK_CONTAINER (labels), priv->secondary_label);

The secondary label is unused. I don't think Boxes uses it either, so lets drop it.

::: src/photos-selection-toolbar.c
@@ +384,2 @@
   selection = photos_selection_controller_get_selection (priv->sel_cntrlr);
+  tmp_list = g_list_copy_deep (selection, (GCopyFunc) g_strdup, NULL);

Do you really need tmp_list?

Review of attachment 280279 [details] [review]:

A few more comments while playing with your patch.

::: src/photos-delete-notification.c
@@ +219,3 @@
+  gtk_image_set_pixel_size (GTK_IMAGE (image), 16);
+
+  close = gtk_button_new ();

The properties 'relief' and 'focus-on-click' should be turned off.

Actually, I wonder, why we don't use GdNotification for each notification, instead of packing them into one big GdNotification. Something to talk about during the BoF.

@@ +227,3 @@
+  gtk_button_set_label (undo, _("Undo"));
+  gtk_widget_set_valign (undo, GTK_ALIGN_CENTER);
+  gtk_container_add (GTK_CONTAINER (self), undo);

The undo button should be on the left of the close button.

@@ +259,3 @@
+
+  priv->deleted_objects = objects;
+  photos_delete_notification_display (self, _("Selected item(s) have been deleted"), NULL);

Use ngettext to solve the singular vs. plural problem.

(In reply to comment #9)
> ::: src/photos-selection-toolbar.c
> @@ +384,2 @@
>    selection = photos_selection_controller_get_selection (priv->sel_cntrlr);
> +  tmp_list = g_list_copy_deep (selection, (GCopyFunc) g_strdup, NULL);
> 
> Do you really need tmp_list?

You were right, Pranav. We need it because removing items from item_mngr changes selection. Adding a comment would be nice.

Created attachment 281570 [details] [review]
Add PhotosDeleteNotification

Now that we have undo-able deletion of photos, we need to allow doing it from the preview. Let's use a separate bug for that. Small bite-sized bugs are easier for new contributors. I filed bug 733660

Created attachment 282081 [details] [review]
local-item: Guard for collections