Created attachment 321788 [details] [review]
Export: Add notifications when photos are exported

Created attachment 321826 [details] [review]
Export: Add notifications when photos are exported

In attachment 321826 [details] [review]:

A workaround has been implemented: If there is an error in exporting, the GList  argument in photos_export_notification_new will be passed as NULL and the location argument will contain the error string.

Also, a bug still remains: When we try to export a another photo while the previous photo's exported notification is still in display; both of the notification overlap each other. 

There is already 
photos_selection_controller_set_selection_mode (self->priv->sel_cntrlr, FALSE); 
in photos_application_save_save but still it allows to select the photo and export it  while the export notification is in display.

Review of attachment 321826 [details] [review]:

Thanks for the patch, Umang. A few quick comments:

::: src/photos-application.c
@@ +987,3 @@
     {
       g_warning ("Unable to save: %s", error->message);
+      photos_export_notification_new (NULL, g_strdup ("Failed to Export"));

Should be a small 'e': "Failed to export"

@@ +1052,3 @@
     {
       g_warning ("Unable to create %s: %s", export_path, error->message);
+      photos_export_notification_new (NULL, "Failed to export: Not enough space");

We can't assume that a failure to create the directory is due to lack of space. Also, photos_base_item_save_async can fail due to lack of space. File I/O is racy - multiple processes can access the filesystem at the same time.

So, we need to explicitly check for G_IO_ERROR_NO_SPACE. See g_error_matches. There are some examples in the code.

::: src/photos-export-notification.c
@@ +132,3 @@
+  
+  else if (length > 1)
+    msg = g_strdup_printf (ngettext ("%d item exported", "%d items exported", length), length);

Nice. :)

@@ +152,3 @@
+
+  if(length == 1)
+  {

The brace is misaligned.

Created attachment 322182 [details] [review]
export: add notifications when photos are exported

Review of attachment 322182 [details] [review]:

This is not in "git format-patch" format.

::: src/photos-application.c
@@ +996,3 @@
+  photos_export_notification_new (items, g_file_get_uri (export));
+
+  g_list_free_full (items, g_object_unref);

Put it at the end of the function. That way we know we won't leak anything even if the code changes a bit.

::: src/photos-export-notification.c
@@ +18,3 @@
+ * 02110-1301, USA.
+ */
+

Nitpick: needs another new line.

@@ +30,3 @@
+#include "photos-notification-manager.h"
+#include "photos-search-context.h"
+

Ditto.

@@ +127,3 @@
+
+  if ((length == 0) && (self->error != NULL))
+  {

The indentation is off.

@@ +129,3 @@
+  {
+    if (g_error_matches ((self->error), G_IO_ERROR, G_IO_ERROR_NO_SPACE))
+        msg = g_strdup ("Failed to export: not enough space");

Ditto.

@@ +132,3 @@
+
+    else
+        msg = g_strdup ("Failed to export");

These user visible strings need to be enclosed in _(), and this file should be listed in po/POTFILES.in.

@@ +150,3 @@
+    }
+  
+  else

No need for the new line. Enclose the else in braces too.

@@ +158,3 @@
+  g_free (msg);
+
+  if(length == 1)

Missing space between if and (.

@@ +225,3 @@
+  PhotosExportNotification *self = PHOTOS_EXPORT_NOTIFICATION (object);
+
+  g_return_if_fail (self->export_loc != NULL);

We don't usually do this. It is not idiomatic. :)

@@ +287,3 @@
+  self->ntfctn_mngr = g_object_ref_sink (photos_notification_manager_dup_singleton ());
+  self->item_mngr = g_object_ref (state->item_mngr);
+  self->error = NULL;

No need to initialize it to NULL. GObject will zero-initialize the whole structure.

Created attachment 322203 [details] [review]
export: add notifications when photos are exported

https://bugzilla.gnome.org/show_bug.cgi?id=762371

Review of attachment 322203 [details] [review]:

Thanks, this looks better.

I don't see the "analyze", "empty trash" buttons, though. Grep for DISK_SPACE_ANALYZER in gnome-settings-daemon's plugins/housekeeping/gsd-disk-space.c file to see an example implementation.

::: src/photos-export-notification.c
@@ +49,3 @@
+};
+
+

Nitpick: extra newline.

@@ +57,3 @@
+  PROP_ERROR
+};
+

Missing a newline.

@@ +59,3 @@
+
+G_DEFINE_TYPE (PhotosExportNotification, photos_export_notification, GTK_TYPE_GRID);
+

Ditto.

Created attachment 322247 [details] [review]
export: add notifications when photos are exported

https://bugzilla.gnome.org/show_bug.cgi?id=762371

Created attachment 322280 [details] [review]
export: add notifications when photos are exported

Created attachment 322281 [details] [review]
export: add notifications when photos are exported

Review of attachment 322281 [details] [review]:

Thanks, Umang. This looks better. A few comments:

::: src/photos-export-notification.c
@@ +168,3 @@
+  G_OBJECT_CLASS (photos_export_notification_parent_class)->constructed (object);
+
+  length = g_list_length (self->items);

It would be nice to avoid g_list_length if possible because it is O(n). In this case we can avoid it.

@@ +170,3 @@
+  length = g_list_length (self->items);
+
+  if ((length == 0) && (self->error != NULL))

I would add a g_assert_nonnull to enforce that self->error is non-NULL if self->items is NULL and vice-versa. Otherwise, it would be a programming error.

@@ +281,3 @@
+
+static void
+photos_export_notification_get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec)

We don't need this. See below.

@@ +316,3 @@
+
+    case PROP_EXPORT_LOCATION:
+      {

Brace not needed.

@@ +317,3 @@
+    case PROP_EXPORT_LOCATION:
+      {
+        self->export_loc = g_value_dup_string (value);

We are not freeing the string anywhere.

@@ +319,3 @@
+        self->export_loc = g_value_dup_string (value);
+        break;
+        return;

Typo alert!

@@ +323,3 @@
+
+    case PROP_ERROR:
+      {

Brace not needed.

@@ +372,3 @@
+                                                        "Location of exported items",
+                                                        NULL,
+                                                        G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE));

Why does this property need to be readable? We are not reading it via g_object_get or a getter method.

@@ +380,3 @@
+                                                       "Error object which contains the error details",
+                                                       G_TYPE_ERROR,
+                                                       G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE));

Ditto.

@@ +391,3 @@
+  g_object_new (PHOTOS_TYPE_EXPORT_NOTIFICATION,
+                "items", items,
+                "export_location", location,

How about just calling it 'location' like the parameter?

@@ +394,3 @@
+                "error", NULL,
+                "column-spacing", 12,
+                "margin-start", 12,

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..

(In reply to Debarshi Ray from comment #12)
> Review of attachment 322281 [details] [review] [review]:
> ::: src/photos-export-notification.c
> @@ +168,3 @@
> +  G_OBJECT_CLASS (photos_export_notification_parent_class)->constructed
> (object);
> +
> +  length = g_list_length (self->items);
> 
> It would be nice to avoid g_list_length if possible because it is O(n). In
> this case we can avoid it.

Ignore this. I was mistaken. I can see why you really need the length.

Created attachment 322345 [details] [review]
export: add notifications when photos are exported

Fixed the above issues.

Review of attachment 322345 [details] [review]:

Somethings left for you to address:

::: src/photos-export-notification.c
@@ +112,3 @@
+
+  g_free (file_uri);
+}

We should close the notification when the open buttons are clicked.

@@ +121,3 @@
+  const gchar *argv[] = { DISK_SPACE_ANALYZER, "/", NULL };
+
+  g_spawn_async (NULL, (gchar **) argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, NULL, NULL);

I am inclined to think that g_desktop_app_info_new and g_app_info_launch are better options than g_spawn_async. I am not 100% sure, yet.

@@ +151,3 @@
+photos_export_notification_empty_trash (PhotosExportNotification *self)
+{
+  g_bus_get (G_BUS_TYPE_SESSION, NULL, photos_export_notification_trash_callback, self);

Since we are not using a GCancellable to hold the asynchronous operations together, we should take a reference on self when we pass it as user_data and unref it in the callback.

(In reply to Debarshi Ray from comment #15)
> @@ +121,3 @@
> +  const gchar *argv[] = { DISK_SPACE_ANALYZER, "/", NULL };
> +
> +  g_spawn_async (NULL, (gchar **) argv, NULL, G_SPAWN_SEARCH_PATH, NULL,
> NULL, NULL, NULL);
> 
> I am inclined to think that g_desktop_app_info_new and g_app_info_launch are
> better options than g_spawn_async. I am not 100% sure, yet.

I spoke to Bastien in #control-center on GIMPNet, and, indeed, we should use g_desktop_app_info_new and g_app_info_launch.

> 
> I spoke to Bastien in #control-center on GIMPNet, and, indeed, we should use
> g_desktop_app_info_new and g_app_info_launch.

Yes, I have already ported the code for using that.

Created attachment 322397 [details] [review]
export: add notifications when photos are exported

Review of attachment 322397 [details] [review]:

Thanks, Umang. This is looking better.

::: src/photos-application.c
@@ +994,3 @@
 
+  items = g_list_prepend (items, g_object_ref (item));
+  export =  G_FILE (g_object_get_data (G_OBJECT (item), "export_dir"));

Trying to reconstruct the location of the exported file won't be possible once we land the patches in bug 762318 We need to change photos_base_item_save_async/finish to return the exported file, and then use it here.

@@ +995,3 @@
+  items = g_list_prepend (items, g_object_ref (item));
+  export =  G_FILE (g_object_get_data (G_OBJECT (item), "export_dir"));
+  photos_export_notification_new (items, g_file_get_uri (export));

We need to free the value returned by g_file_get_uri. Otherwise we are leaking it.

::: src/photos-export-notification.c
@@ +22,3 @@
+#include "config.h"
+
+#include <gio/gdesktopappinfo.h>

We should also add gio-unix-2.0 to configure.ac.

@@ +32,3 @@
+#include "photos-notification-manager.h"
+#include "photos-search-context.h"
+#include "photos-utils.h"

Unused header.

@@ +38,3 @@
+{
+  GtkGrid parent_instance;
+  PhotosBaseManager *item_mngr;

item_mngr is unused.

@@ +100,3 @@
+
+  self->timeout_id = 0;
+  photos_export_notification_close (self);

Why not directly call photos_export_notification_destroy?

@@ +118,3 @@
+    }
+
+  photos_export_notification_close (self);

Ditto.

@@ +163,3 @@
+  GError *error = NULL;
+
+  connection = g_bus_get_finish (result, &error);

We need to unref connection once we are done with it.

@@ +172,3 @@
+    }
+
+    g_dbus_connection_call (connection,

The indentation is off.

@@ +177,3 @@
+                            "org.gnome.SettingsDaemon.Housekeeping",
+                            "EmptyTrash",
+                            NULL, NULL, 0, -1, NULL, NULL, NULL);

One parameter per line, and G_DBUS_CALL_FLAGS_NONE instead of 0.

@@ +236,3 @@
+  g_free (msg);
+
+  if (g_error_matches (self->error, G_IO_ERROR, G_IO_ERROR_NO_SPACE))

It would be more readable if we use the same structure for the if...else as above.

@@ +271,3 @@
+      gtk_widget_set_halign (open, GTK_ALIGN_CENTER);
+      gtk_container_add (GTK_CONTAINER (self), open);
+      g_object_set_data (G_OBJECT (self), "file_loc", file_loc);

Instead of g_object_set_data, we could just use a member variable for self. However, we need to do this differently, anyway. See above.

Created attachment 322612 [details] [review]
Make photos_base_item_save_finish return the exported file

Created attachment 322613 [details] [review]
Add notifications when photos are exported

Fixed the above issues, but we need to ask for permission before pushing.

Since we are in UI Freeze and String Change Announcement Period [1], could you please send the email asking for permission to add the new UI and strings?

[1] https://wiki.gnome.org/ThreePointNineteen

(In reply to Debarshi Ray from comment #21)
> Created attachment 322613 [details] [review] [review]
> Add notifications when photos are exported
> 
> Fixed the above issues, but we need to ask for permission before pushing.

Pushed to master.

The freeze break request [1] was sent two days ago, and so far we have only received only one approval (which was unfortunately not CCed to the release-team mailing list). However, given the following reasons, I am not going to wait any further:
  (a) We don't need approval from the documentation team or the translators.
  (b) This is a relatively self-contained follow-up to a feature (editing) that we worked on this cycle. Hence should be safe.
  (c) I am not going to release 3.20 without this patch and it is holding up our 3.19.91 release.

I wish the release team was more active in handling paperwork related to the release, but, at the end of the day, as maintainer, I am on the hook for the well-being of this module and I am confident that this patch is good enough to go in.

[1] https://mail.gnome.org/archives/release-team/2016-February/msg00046.html