Created attachment 319835 [details] [review]
Add export option in selection mode

This is the first attempt at adding Export functionality into selection mode.

Since the export dialog needs the picture loaded to calculate its maximum dimensions, I basically just added the load method when it's constructed. I also tried to merge shared code to a separate method.

Let me know what you guys think.

Created attachment 319843 [details] [review]
Enable Export functionality on selection mode

This second version uses GAction instead of a click for the Export button, further merging the functionality between Application and SelectionToolbar.

Review of attachment 319843 [details] [review]:

Thanks for the patch, Rafael. This is looking quite good. I could only take a quick look at it, and didn't have time to actually run it. So, please accept my apologies if any of my comments don't match reality. :)

::: src/photos-application.c
@@ +313,3 @@
+  sel_cntrlr = photos_selection_controller_dup_singleton ();
+  if (!enable && photos_selection_controller_get_selection_mode (sel_cntrlr))
+    {

Does it work without connecting to the signals from SelectionController? We want to re-evaluate the status of the actions whenever the selection mode is toggled, or the number of selected items change.

Currently this function is only called when the window mode changes (ie. we go to the different views, preview, edit, etc.) or when an image has finished loading.

@@ +316,3 @@
+      GList *selection = photos_selection_controller_get_selection (sel_cntrlr);
+      /* Export only works for a single item selection for now */
+      enable = (selection != NULL && selection->next == NULL);

For the time being, we also need to ensure that the selected item is not a collection. This can happen if the user is selecting some search results.

@@ +1013,3 @@
+  PhotosSelectionController *sel_cntrlr;
+
+  sel_cntrlr = photos_selection_controller_dup_singleton ();

Since we need it in multiple places, I would suggest making it a class variable to reduce typing. Add it to PhotosApplicationPrivate and unref in dispose. It's a global singleton after all, it's not that we are going to save a lot of resources by keeping it local. :)

@@ +1028,3 @@
+    {
+      item = PHOTOS_BASE_ITEM (photos_base_manager_get_active_object (priv->state->item_mngr));
+    }

This logic to get the current selection or active object has been duplicated above. Good candidate for adding a utility function. I can imagine this being useful as we add more GActions that are usable from both the selection toolbar and preview.

::: src/photos-export-dialog.c
@@ +240,3 @@
+    {
+      photos_base_item_load_async (self->item, self->cancellable,
+                                   photos_export_dialog_constructed_do, self);

The load operation can fail. It would be nice to handle that by checking the error from photos_base_item_load_finish in the callback. Note that the return value from photos_base_item_load_finish should be unreffed even if we don't use it.

@@ +381,3 @@
+  const gchar *pictures_path;
+  gchar *export_path = NULL;
+

I will sleep better if we kept this in photos-application.c for the sake of keeping the Git history clean:
(a) It doesn't seem strictly necessary for this commit (ie. adding export to the selection toolbar), so if you really want to move it, then do it in a separate commit.
(b) I can imagine that we have to move quite some code around when we implement export for multiple images and albums. So, maybe we can move this once we know what the final outcome will look like. That will avoid some needless churn if we move it now and then have to move again later.

::: src/photos-selection-toolbar.c
@@ +40,3 @@
 #include "photos-selection-controller.h"
 #include "photos-selection-toolbar.h"
+#include "photos-export-dialog.h"

None of the changes in this file should be necessary. See below.

@@ +368,3 @@
+  app = g_application_get_default ();
+  action = g_action_map_lookup_action (G_ACTION_MAP (app), "save-current");
+  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), show_export);

Disabling a GAction should "disable" the widgets that proxy it. In this case, the "Export" button.

The fact that trash/delete still has some things in this file is a relic of the past. It didn't use GActions in the beginning. Then, as we started adding multiple UI controls for "delete" (selection toolbar, hamburger menu in preview) we migrated it to use GActions, but the migration wasn't complete. :/ We should clean it up.

(In reply to Debarshi Ray from comment #3)
> Review of attachment 319843 [details] [review] [review]:
> 
> @@ +368,3 @@
> +  app = g_application_get_default ();
> +  action = g_action_map_lookup_action (G_ACTION_MAP (app), "save-current");
> +  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), show_export);
> 
> Disabling a GAction should "disable" the widgets that proxy it. In this
> case, the "Export" button.
> 
> The fact that trash/delete still has some things in this file is a relic of
> the past. It didn't use GActions in the beginning. Then, as we started
> adding multiple UI controls for "delete" (selection toolbar, hamburger menu
> in preview) we migrated it to use GActions, but the migration wasn't
> complete. :/ We should clean it up.

Please, consider the attached patch for that.

Created attachment 319881 [details] [review]
Cleanup old trash widget code

(In reply to Debarshi Ray from comment #3)
> Review of attachment 319843 [details] [review] [review]:
> 
> ::: src/photos-application.c
> @@ +313,3 @@
> +  sel_cntrlr = photos_selection_controller_dup_singleton ();
> +  if (!enable && photos_selection_controller_get_selection_mode
> (sel_cntrlr))
> +    {
> 
> Does it work without connecting to the signals from SelectionController? We
> want to re-evaluate the status of the actions whenever the selection mode is
> toggled, or the number of selected items change.
> 
> Currently this function is only called when the window mode changes (ie. we
> go to the different views, preview, edit, etc.) or when an image has
> finished loading.

Well, it does work and I don't think I can explain exactly why. Maybe because we disable the action from the SelectionToolbar when the selection changes?

> @@ +316,3 @@
> +      GList *selection = photos_selection_controller_get_selection
> (sel_cntrlr);
> +      /* Export only works for a single item selection for now */
> +      enable = (selection != NULL && selection->next == NULL);
> 
> For the time being, we also need to ensure that the selected item is not a
> collection. This can happen if the user is selecting some search results.

I tried to address that in the new version, but I don't have any collections to test with.

> @@ +1013,3 @@
> +  PhotosSelectionController *sel_cntrlr;
> +
> +  sel_cntrlr = photos_selection_controller_dup_singleton ();
> 
> Since we need it in multiple places, I would suggest making it a class
> variable to reduce typing. Add it to PhotosApplicationPrivate and unref in
> dispose. It's a global singleton after all, it's not that we are going to
> save a lot of resources by keeping it local. :)

Done.

> @@ +1028,3 @@
> +    {
> +      item = PHOTOS_BASE_ITEM (photos_base_manager_get_active_object
> (priv->state->item_mngr));
> +    }
> 
> This logic to get the current selection or active object has been duplicated
> above. Good candidate for adding a utility function. I can imagine this
> being useful as we add more GActions that are usable from both the selection
> toolbar and preview.

Done.

> ::: src/photos-export-dialog.c
> @@ +240,3 @@
> +    {
> +      photos_base_item_load_async (self->item, self->cancellable,
> +                                   photos_export_dialog_constructed_do,
> self);
> 
> The load operation can fail. It would be nice to handle that by checking the
> error from photos_base_item_load_finish in the callback. Note that the
> return value from photos_base_item_load_finish should be unreffed even if we
> don't use it.

Done.

> @@ +381,3 @@
> +  const gchar *pictures_path;
> +  gchar *export_path = NULL;
> +
> 
> I will sleep better if we kept this in photos-application.c for the sake of
> keeping the Git history clean:
> (a) It doesn't seem strictly necessary for this commit (ie. adding export to
> the selection toolbar), so if you really want to move it, then do it in a
> separate commit.
> (b) I can imagine that we have to move quite some code around when we
> implement export for multiple images and albums. So, maybe we can move this
> once we know what the final outcome will look like. That will avoid some
> needless churn if we move it now and then have to move again later.
> 
> ::: src/photos-selection-toolbar.c
> @@ +40,3 @@
>  #include "photos-selection-controller.h"
>  #include "photos-selection-toolbar.h"
> +#include "photos-export-dialog.h"
> 
> None of the changes in this file should be necessary. See below.

Yes, those were my bad. Removed them now.

Created attachment 319882 [details] [review]
Enable Export functionality on selection mode

Created attachment 319942 [details] [review]
Enable Export functionality on selection mode

New version with better handling for enabling/disabling the Export action from the PhotosApplication.

Review of attachment 319942 [details] [review]:

This is looking very good. Other than the cosmetic stuff, there is one issue in the error handling of the asynchronous load:

::: src/photos-application.c
@@ +286,3 @@
+    /* Export does not work for collections atm */
+    if (item == NULL || photos_base_item_is_collection (item))
+      return NULL;

It would be slightly better to move the is_collection check to the caller. I can imagine using this function to wire the "properties" button to the "app.properties" action, and that works on collections too. So, it will be useful to keep this a little generic.

@@ +345,3 @@
+   */
+  if (!enable && photos_selection_controller_get_selection_mode (priv->sel_cntrlr))
+    enable = (item != NULL);

It would be nice to split this into a separate "paragraph". We are going to add more actions like this and it will be more readable.

::: src/photos-export-dialog.c
@@ +177,2 @@
 {
+  PhotosExportDialog *self = PHOTOS_EXPORT_DIALOG (user_data);

We can't touch user_data without making sure that the asynchronous load wasn't cancelled. Here is the full story.

We want to ensure that ExportDialog is not destroyed while one of the internal async operations (eg., load, guess_sizes) are going on. There are 2 ways to do that:
(a) Take a ref on self while starting the async and unref in the clallback. Or,
(b) Pass a class-wide cancellable which is cancelled during destruction.

(a) is easier to implement and we use it in quite a few places. The downside is that the caller/client/user of the object doesn't know that it can outlive the last ref known to the caller. So if the instance emitted signals and stuff, the caller would be surprised.

We are using (b) in this case. So, we need to check if the error was due to a cancellation or not. If it was, then we cannot touch self (ie. the ExportDialog object) because cancellation occurs when the object is getting destroyed.

@@ +185,3 @@
+      g_warning ("Error when loading item: %s", error->message);
+      g_error_free (error);
+      goto out;

Other than special casing G_IO_ERROR_CANCELLED (see above), it would be nice to set the text in self->dir_entry on error. Failing to load means we can't do the downscaling / size guessing, but we can still offer some basic functionality.

Otherwise, we will have to somehow visibly abort the dialog. 

Since we will anyway have to deal with failures during the actual export (ie. photos_base_item_save_async), I think it will be simpler to press on with the operation and handle it there. We could attempt the load once more in save_async, just in case. File I/O being inherently racy and all. *shrug*

@@ +236,3 @@
+
+ out:
+  g_object_unref (node);

g_clear_object, not g_object_unref because the node can be NULL in the error path.

::: src/photos-selection-toolbar.c
@@ +53,3 @@
   GtkWidget *toolbar_properties;
   GtkWidget *toolbar_trash;
+  GtkWidget *toolbar_export;

We don't need this either.

@@ +486,3 @@
   gtk_widget_class_bind_template_child (widget_class, PhotosSelectionToolbar, toolbar_properties);
   gtk_widget_class_bind_template_child (widget_class, PhotosSelectionToolbar, toolbar_collection);
+  gtk_widget_class_bind_template_child (widget_class, PhotosSelectionToolbar, toolbar_export);

Nor this. It's magic. ;)

Created attachment 320033 [details] [review]
Support exporting from selection mode

I took the liberty to fix it up and pushed. Thanks, Rafael!

Umm... I didn't think of one thing.

We should set the text in self->dir_entry before the asynchronous load, because we don't need the load for that. Otherwise, right now, you can see the entry being empty for a moment while the load is ongoing.

Secondly, we should show self->progress_label also when the load is going on.

Do you want to fix these?

(In reply to Debarshi Ray from comment #11)
> Secondly, we should show self->progress_label also when the load is going on.

This part is actually a bit tricky.

We only show the downscaling options for big images, where big is defined as greater than 1024 pixels. So far, we knew the size of the edited image immediately after opening the dialog, and we knew whether we want to show the options or not. This let us avoid changing the dialog's height while the user was looking at it.

Now, if we are opening the dialog from the selection mode, we won't know the size till we have loaded the image. Loading is asynchronous and adds a delay. The user will see the dialog's height change if we add or remove the rows for the options.

The alternative is to pad the dialog with empty space - like we do now when showing self->progress_label. However, it might look a bit odd to have so much empty space.

:/

(In reply to Debarshi Ray from comment #11)
> Umm... I didn't think of one thing.
> 
> We should set the text in self->dir_entry before the asynchronous load,
> because we don't need the load for that. Otherwise, right now, you can see
> the entry being empty for a moment while the load is ongoing.

Patch attached.

Created attachment 320168 [details] [review]
Set dialog entry before loading photo

Review of attachment 319881 [details] [review]:

::: src/photos-selection-toolbar.c
@@ +365,3 @@
+  action = g_action_map_lookup_action (G_ACTION_MAP (app), "delete");
+  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), show_trash);
+

Ideally, we should move the enable/disable logic for delete to photos_application_actions_update because:
(a) We want to disable the action while an item is being loaded.
(b) We don't want it to be enabled in the overview outside the selection mode.

Also, it is easier to guarantee that the code is correct if all the conditions are consolidated in one place instead of being distributed across various classes.

(We are approaching a point where the enable/disable logic in photos_application_actions_update can no longer be one-liners. eg., for delete we need a loop. Similarly for print and open.)

Review of attachment 320168 [details] [review]:

Thanks, Rafael. Looks good to me apart from some cosmetic adjustment.

::: src/photos-export-dialog.c
@@ +271,3 @@
     {
+      gchar *now_str = NULL;
+      GDateTime *now = g_date_time_new_now_local ();

Nitpick: we use a separate line for function calls.

Created attachment 320219 [details] [review]
export-dialog: Set the directory name before loading the item

Made the above adjustments, added the bug URL to the commit message and pushed.

(In reply to Debarshi Ray from comment #12)
> (In reply to Debarshi Ray from comment #11)
>> Secondly, we should show self->progress_label also when the load is
>> going on.
> 
> This part is actually a bit tricky.
>
> [...]
>
> The alternative is to pad the dialog with empty space - like we do now
> when showing self->progress_label. However, it might look a bit odd to
> have so much empty space.

Part of a conversation with aday in #gnome-design on GIMPNet:

10:37 <rishi> aday: Hey!
10:37 <rishi> Do you want to me to explain the issue with                       
      https://bugzilla.gnome.org/show_bug.cgi?id=760838#c12 ?
10:38 <aday> rishi: did you see that i pointed to the mockup the other day?
10:39 <rishi> aday: Yes.
10:39 <aday> rishi: don't the mockups do what you are proposing in the          
      comment? (padding out the dialog)
10:40 <rishi> aday: The problem is with 'small' images - the ones for which we  
      don't show the size options at all.
10:40 <rishi> I am wondering if  the padding would lead to too much empty       
      space in that case.
10:41 <aday> empty space isn't a problem
10:41 <aday> wouldn't want to show the "calculating export size" string in      
      that case
10:41 <aday> but maybe that isn't an issue, since it will be fast for small     
      images?
10:44 <rishi> aday: If empty space isn't an issue, then OK!

Created attachment 320494 [details] [review]
export-dialog: Show the progress message when loading

Comment on attachment 319881 [details] [review]
Cleanup old trash widget code

Patch submitted to the proper bug (#761587).

All patches have been committed. Closing this bug.