After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 751181 - Photo upload/sharing
Photo upload/sharing
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
3.17.x
Other All
: Normal enhancement
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on: 768291
Blocks:
 
 
Reported: 2015-06-18 20:43 UTC by Allan Day
Modified: 2016-08-22 15:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
share-image: Implementation with blocking version (17.65 KB, patch)
2016-06-15 06:07 UTC, Umang Jain
none Details | Review
share-image: Implementation with blocking version (19.77 KB, patch)
2016-06-15 08:58 UTC, Umang Jain
none Details | Review
Asynchronous sharing of a single image (24.17 KB, patch)
2016-06-20 15:26 UTC, Umang Jain
none Details | Review
Proof of concept built over wip/rishi/share-point (17.16 KB, patch)
2016-07-01 15:08 UTC, Umang Jain
none Details | Review
Add Share Dialog (17.58 KB, patch)
2016-07-06 15:33 UTC, Umang Jain
none Details | Review
Add google share point (13.39 KB, patch)
2016-07-06 15:35 UTC, Umang Jain
none Details | Review
Add sharing action (2.95 KB, patch)
2016-07-06 15:35 UTC, Umang Jain
none Details | Review
Add share icon in main toolbar (1.90 KB, patch)
2016-07-06 15:36 UTC, Umang Jain
none Details | Review
Add share dialog (16.88 KB, patch)
2016-07-09 19:21 UTC, Umang Jain
none Details | Review
Add Google share point (11.13 KB, patch)
2016-07-09 19:22 UTC, Umang Jain
none Details | Review
photos-application: Add share action (4.11 KB, patch)
2016-07-09 19:23 UTC, Umang Jain
none Details | Review
main-toolbar: Add share icon (1.95 KB, patch)
2016-07-09 19:23 UTC, Umang Jain
none Details | Review
photos-application: Mark application as busy while uploading (1.12 KB, patch)
2016-07-09 19:29 UTC, Umang Jain
none Details | Review
Add share dialog (12.27 KB, patch)
2016-07-15 13:13 UTC, Umang Jain
none Details | Review
Add Google share point (11.18 KB, patch)
2016-07-15 13:13 UTC, Umang Jain
none Details | Review
Add photos share point (14.95 KB, patch)
2016-07-15 14:12 UTC, Umang Jain
none Details | Review
utils: Add an extension point for PhotosSharePoint (1.86 KB, patch)
2016-07-15 14:13 UTC, Umang Jain
none Details | Review
Add photos share point manager (8.64 KB, patch)
2016-07-15 14:14 UTC, Umang Jain
none Details | Review
application: Add share action (3.68 KB, patch)
2016-07-15 17:01 UTC, Umang Jain
none Details | Review
selection-toolbar: Add share icon (1.38 KB, patch)
2016-07-15 17:43 UTC, Umang Jain
none Details | Review
application: Enable share action (2.44 KB, patch)
2016-07-15 18:25 UTC, Umang Jain
none Details | Review
Failure notification for sharing (12.24 KB, patch)
2016-08-05 13:29 UTC, Umang Jain
none Details | Review
utils: Add photos_utils_app_info_launch_uri (2.04 KB, patch)
2016-08-06 15:17 UTC, Umang Jain
none Details | Review
base-item: Use photos_utils_app_info_launch_uri (1.13 KB, patch)
2016-08-06 15:17 UTC, Umang Jain
none Details | Review
filterable, source: Add get_builtin (2.76 KB, patch)
2016-08-06 15:18 UTC, Umang Jain
committed Details | Review
base-manager: Use photos_filterable_get_builtin (928 bytes, patch)
2016-08-06 15:19 UTC, Umang Jain
committed Details | Review
base-item: Split the code to encode a GeglBuffer and copy the metadata (12.55 KB, patch)
2016-08-06 15:20 UTC, Umang Jain
none Details | Review
base-item: Add photos_base_item_save_to_stream_async (11.71 KB, patch)
2016-08-06 15:21 UTC, Umang Jain
none Details | Review
Add PhotosSharePoint (6.68 KB, patch)
2016-08-06 15:21 UTC, Umang Jain
none Details | Review
utils: Add an extension point for PhotosSharePoint sub-classes (1.87 KB, patch)
2016-08-06 15:22 UTC, Umang Jain
none Details | Review
Add PhotosSharePointOnline (9.20 KB, patch)
2016-08-06 15:23 UTC, Umang Jain
none Details | Review
utils: Add an extension point for PhotosSharePointOnline sub-classes (1.90 KB, patch)
2016-08-06 15:25 UTC, Umang Jain
none Details | Review
Add PhotosSharePointManager (10.20 KB, patch)
2016-08-06 15:25 UTC, Umang Jain
none Details | Review
Add PhotosSharePointGoogle (12.28 KB, patch)
2016-08-06 15:26 UTC, Umang Jain
none Details | Review
Add PhotosSharePointEmail (11.13 KB, patch)
2016-08-06 15:26 UTC, Umang Jain
none Details | Review
Add PhotosShareDialog (12.57 KB, patch)
2016-08-06 15:27 UTC, Umang Jain
none Details | Review
application: Add share action (3.79 KB, patch)
2016-08-06 15:33 UTC, Umang Jain
none Details | Review
application: Enable Share action (2.44 KB, patch)
2016-08-06 15:34 UTC, Umang Jain
none Details | Review
selection-toolbar: Add share icon (1.39 KB, patch)
2016-08-06 15:35 UTC, Umang Jain
none Details | Review
Add share icon in main toolbar (1.94 KB, patch)
2016-08-06 15:35 UTC, Umang Jain
none Details | Review
Add notification for sharing failure (12.29 KB, patch)
2016-08-06 15:36 UTC, Umang Jain
none Details | Review
Add notification for sharing failure (13.13 KB, patch)
2016-08-14 22:42 UTC, Umang Jain
none Details | Review
base-item: Add load_async to save_to_stream_async for selection mode sharing (4.69 KB, patch)
2016-08-15 09:40 UTC, Umang Jain
none Details | Review
base-item: Add load_async to save_async for selection mode sharing (4.85 KB, patch)
2016-08-15 09:49 UTC, Umang Jain
none Details | Review
utils: Add photos_utils_app_info_launch_uri (2.04 KB, patch)
2016-08-21 11:00 UTC, Umang Jain
committed Details | Review
base-item: Use photos_utils_app_info_launch_uri (1.13 KB, patch)
2016-08-21 11:00 UTC, Umang Jain
committed Details | Review
base-item: Split the code to encode a GeglBuffer and copy the metadata (12.55 KB, patch)
2016-08-21 11:01 UTC, Umang Jain
committed Details | Review
base-item: Add photos_base_item_save_to_stream_async (12.28 KB, patch)
2016-08-21 11:02 UTC, Umang Jain
committed Details | Review
base-item: Transparently load item in photos_base_item_save_async (4.81 KB, patch)
2016-08-21 11:02 UTC, Umang Jain
committed Details | Review
Add PhotosSharePoint (7.13 KB, patch)
2016-08-21 11:03 UTC, Umang Jain
committed Details | Review
utils: Add an extension point for PhotosSharePoint sub-classes (1.87 KB, patch)
2016-08-21 11:03 UTC, Umang Jain
committed Details | Review
Add PhotosSharePointOnline (9.20 KB, patch)
2016-08-21 11:04 UTC, Umang Jain
committed Details | Review
utils: Add an extension point for PhotosSharePointOnline sub-classes (1.87 KB, patch)
2016-08-21 11:04 UTC, Umang Jain
committed Details | Review
Add PhotosSharePointManager (10.20 KB, patch)
2016-08-21 11:06 UTC, Umang Jain
committed Details | Review
Add PhotosSharePointGoogle (13.19 KB, patch)
2016-08-21 11:07 UTC, Umang Jain
committed Details | Review
Add PhotosSharePointEmail (11.26 KB, patch)
2016-08-21 11:08 UTC, Umang Jain
committed Details | Review
Add PhotosShareDialog (12.57 KB, patch)
2016-08-21 11:09 UTC, Umang Jain
committed Details | Review
Add PhotosShareNotification (9.33 KB, patch)
2016-08-21 11:09 UTC, Umang Jain
committed Details | Review
application: Add share action (3.90 KB, patch)
2016-08-21 11:10 UTC, Umang Jain
committed Details | Review
application: Enable Share action (2.44 KB, patch)
2016-08-21 11:10 UTC, Umang Jain
committed Details | Review
selection-toolbar: Add Share button (1.39 KB, patch)
2016-08-21 11:12 UTC, Umang Jain
committed Details | Review
main-toolbar: Add Share button (1.94 KB, patch)
2016-08-21 11:12 UTC, Umang Jain
committed Details | Review
Add PhotosSharePoint (7.15 KB, patch)
2016-08-22 06:18 UTC, Debarshi Ray
committed Details | Review
Add PhotosSharePointManager (12.28 KB, patch)
2016-08-22 06:20 UTC, Debarshi Ray
committed Details | Review
Add PhotosSharePointGoogle (13.19 KB, patch)
2016-08-22 08:25 UTC, Umang Jain
committed Details | Review
Add PhotosSharePointEmail (11.26 KB, patch)
2016-08-22 08:26 UTC, Umang Jain
committed Details | Review
Add PhotosShareDialog (12.84 KB, patch)
2016-08-22 08:27 UTC, Umang Jain
committed Details | Review
Add PhotosShareNotification (9.28 KB, patch)
2016-08-22 08:28 UTC, Umang Jain
committed Details | Review
application: Add share action (5.49 KB, patch)
2016-08-22 08:29 UTC, Umang Jain
committed Details | Review
application: Add share action (5.49 KB, patch)
2016-08-22 08:33 UTC, Umang Jain
committed Details | Review

Description Allan Day 2015-06-18 20:43:54 UTC
Sharing is critical for a photos application. We've been waiting for a system-wide sharing framework for a while, and it's unclear when one will emerge. Until then, Photos could provide something itself. This could allow upload to online accounts, as well as attaching photos to an email, or sending them via Bluetooth.

Initial mockups can be found here:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/photos/wires-sharing.png

When uploading images to accounts that we also display content from (such as Google or Flickr), we need to ensure that we don't end up showing multiple copies of the image. Ideally, we would also indicate if a photo has been shared/uploaded (possibly in the details sidebar described in bug 751116).
Comment 1 Umang Jain 2016-06-15 06:07:42 UTC
Created attachment 329839 [details] [review]
share-image: Implementation with blocking version

This is a single image sharing patch:

1) Creates a demo album and inserts it.
2) Uploads the current previewing image to "Dropbox" folder of Google photos
3) Blocking version - The dialog will freeze for few seconds. 

I will land the _async version in the next 24 hours with further UI refinement. But the proof of concept can be grasped using this patch.

Wrapping in 30 seconds - https://youtu.be/h45ZSc0WnQw
Comment 2 Umang Jain 2016-06-15 08:58:33 UTC
Created attachment 329848 [details] [review]
share-image: Implementation with blocking version

Earlier attached won't "git apply" due a minor error occurred from my side. Apologies.
Comment 3 Umang Jain 2016-06-20 15:26:08 UTC
Created attachment 330080 [details] [review]
Asynchronous sharing of a single image

Asynchronous sharing of a single image [draft patch]. The patch is fully functional. Attaching this patch is just for mere concern for UI refinement and advice for code architecture and subsequent changes.

To Rishi,
Please let me know here the function of GTask and cancellable here. I am not very clear about them.
Comment 4 Debarshi Ray 2016-06-21 17:02:09 UTC
Review of attachment 330080 [details] [review]:

Thanks for the patch Umang. Looks like you have made a good start.

From a very high level, it would be nicer to split this into at least three patches:

::: src/photos-base-item.h
@@ +256,3 @@
 void                photos_base_item_trash                   (PhotosBaseItem *self);
 
+void                photos_base_item_share                   (PhotosBaseItem *self);

One that adds this PhotosBaseItem API.

::: src/photos-share-dialog.h
@@ +55,3 @@
+GType               photos_share_dialog_get_type           (void) G_GNUC_CONST;
+
+GtkWidget          *photos_share_dialog_new                (GtkWindow *parent, PhotosBaseItem *item);

One that adds the PhotosShareDialog.

And then one more that has the code to glue both of them together. ie. the code that adds the share button, creates the dialog and makes the backend calls.
Comment 5 Debarshi Ray 2016-06-21 17:29:50 UTC
Review of attachment 330080 [details] [review]:

Some comments about the photos_base_item_share code:

::: src/photos-base-item.c
@@ +2711,3 @@
+    }
+  app = g_application_get_default();
+  g_application_unmark_busy (app);

I wouldn't do the g_application_mark/unmark_busy (ie. UI) here. Instead I would call them where photos_base_item_share_async/_finish are invoked. That way the backend and UI bits would have a clear separation.

@@ +2774,3 @@
+  error = NULL;
+  upload_stream = gdata_picasaweb_service_upload_file (service, NULL, file_entry, g_file_info_get_display_name (file_info),
+                                                        g_file_info_get_content_type (file_info), self->priv->cancellable, &error);

One general comment about all the GData calls here ...

It will be better to split them out into the PhotosGoogleItem class. We should try to keep PhotosBaseItem as generic as possible. Whenever we need code specific to a storage backend (eg., local, google, facebook, flickr, etc.) we should split it out into the respective sub-classes.

For example, you can add a virtual method called "share" in PhotosBaseItemClass.

@@ +2871,3 @@
+          break;
+        }
+    }

You should be able to use photos_source_manager_get_for_provider_type instead.

Note: we need to figure out how to handle multiple accounts with the same provider type.

@@ +2886,3 @@
+  g_object_set_data (G_OBJECT (self), "service", g_object_ref (service));
+  gdata_authorizer_refresh_authorization_async (GDATA_AUTHORIZER (authorizer), self->priv->cancellable,
+                                                  photos_base_item_auth_refresh_cb, self);

Nitpick: misaligned lines; either put everything in one line if they are within 120 characters or put each argument on a separate line.

As is now mentioned in the libgdata documentation (see bug 767784), it will be better to first try gdata_picasaweb_service_upload_file, and then handle GDATA_SERVICE_ERROR_AUTHENTICATION_REQUIRED, because a previous call might have already refreshed the tokens. However, this isn't a big deal, so don't worry too much about it right now.

@@ +2895,3 @@
+
+
+void photos_base_item_share (PhotosBaseItem *self)

Why not turn this into a photos_base_item_share_async/_finish pair? Like photos_base_item_async/_finish.

Right now, the caller of this function doesn't have a way to know the outcome of the operation. Ideally, it should.

I know that we have things like photos_base_item_set_favorite and photos_base_item_trash, but it is probably better to not spread that style any further. We inherited those from gnome-documents.
Comment 6 Debarshi Ray 2016-06-21 18:57:49 UTC
Review of attachment 330080 [details] [review]:

Some comments about the PhotosShareDialog code:

::: src/photos-share-dialog.c
@@ +59,3 @@
+  g_return_if_fail (self != NULL);
+
+  photos_base_item_share (self->item);

It will be better to make this call where photos_share_dialog_new, etc. are called. ie. in photos-application.c. Among other things, it is nice to consolidate all the backend calls in a handful of places.

@@ +60,3 @@
+
+  photos_base_item_share (self->item);
+  g_signal_emit_by_name (self, "response");

The GtkDialog::response signal needs an integer argument, which is often one of GtkResponseType values. In this case, I would suggest GTK_RESPONSE_OK.

Pedantically speaking, the correct way to emit GtkDialog::response is to call gtk_dialog_response. It is a thin wrapper around g_signal_emit, so it doesn't really matter, except that g_signal_emit is marginally faster than g_signal_emit_by_name.

@@ +74,3 @@
+  GtkWidget *label;
+
+  G_OBJECT_CLASS (photos_share_dialog_parent_class)->constructed (object);

Nitpick: a newline after chaining up will be nice.

@@ +88,3 @@
+  image = gtk_image_new_from_icon_name ("goa-account-google", GTK_ICON_SIZE_INVALID);
+  gtk_image_set_pixel_size (GTK_IMAGE (image), 96);
+  gtk_label_set_text (GTK_LABEL (label), "Google");

Don't hard code "goa-account-google" and "Google". We have all the information inside PhotosSourceManager.

Since GoaClient can have a lot of different kinds of accounts that are irrelevant for us, I suggest looking at all the GIOExtensions implementing PHOTOS_BASE_ITEM_EXTENSION_POINT_NAME. Note that the names of the extensions are the same as the provider-type property of the GoaAccounts.

Broad steps:
(a) get a list of extensions implementing PHOTOS_BASE_ITEM_EXTENSION_POINT_NAME
(b) get the name of each GIOExtension
(c) use photos_source_manager_get_for_provider_type to get a list of PhotosSources (if any) with name as provider type
(d) take the first PhotosSource, and look at its name and icon

@@ +95,3 @@
+  gtk_grid_attach_next_to (GTK_GRID (grid), label, button, GTK_POS_BOTTOM, 1, 1);
+  gtk_container_add (GTK_CONTAINER (self->flow_box), child);
+  g_signal_connect_swapped (button, "clicked", G_CALLBACK (photos_share_dialog_cb), self);

Nitpick: photos_share_dialog_clicked would be a better name.
Comment 7 Debarshi Ray 2016-06-21 19:04:06 UTC
(In reply to Umang Jain from comment #3)
> Created attachment 330080 [details] [review] [review]
> To Rishi,
> Please let me know here the function of GTask and cancellable here. I am not
> very clear about them.

Can you point me to the exact code? The name of the function will do.
Comment 8 Umang Jain 2016-06-21 19:17:42 UTC
(In reply to Debarshi Ray from comment #7)
> (In reply to Umang Jain from comment #3)
> > Created attachment 330080 [details] [review] [review] [review]
> > To Rishi,
> > Please let me know here the function of GTask and cancellable here. I am not
> > very clear about them.
> 
> Can you point me to the exact code? The name of the function will do.

g_file_read_async is the function. Look at the g_output_stream_splice_async invoked in the callback of g_file_read_async.
Comment 9 Debarshi Ray 2016-06-22 09:42:56 UTC
(In reply to Umang Jain from comment #8)
> (In reply to Debarshi Ray from comment #7)
> > (In reply to Umang Jain from comment #3)
> > > Created attachment 330080 [details] [review] [review] [review] [review]
> > > To Rishi,
> > > Please let me know here the function of GTask and cancellable here. I am not
> > > very clear about them.
> > 
> > Can you point me to the exact code? The name of the function will do.
> 
> g_file_read_async is the function. Look at the g_output_stream_splice_async
> invoked in the callback of g_file_read_async.

Ok. I will leave a comment as a review (via splinter).
Comment 10 Debarshi Ray 2016-06-22 09:52:26 UTC
Review of attachment 330080 [details] [review]:

::: src/photos-base-item.c
@@ +2739,3 @@
+                                G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE | G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET,
+                                G_PRIORITY_DEFAULT,
+                                NULL,   //use cancellable here.

See below about using a GCancellable.

@@ +2800,3 @@
+  g_object_set_data (G_OBJECT (upload_stream), "service", g_object_ref (service));
+  g_object_ref (file_data);
+  g_file_read_async (file_data, G_PRIORITY_DEFAULT, self->priv->cancellable, photos_base_item_upload_stream, upload_stream);

It is incorrect to pass self->priv->cancellable here.

Once you turn photos_base_item_share into a photos_base_item_async/_finish pair, you should pass the GCancellable that you received from the photos_base_item_async caller.
Comment 11 Debarshi Ray 2016-06-29 11:37:15 UTC
(In reply to Debarshi Ray from comment #5)
> Review of attachment 330080 [details] [review] [review]:
> One general comment about all the GData calls here ...
> 
> It will be better to split them out into the PhotosGoogleItem class. We
> should try to keep PhotosBaseItem as generic as possible. Whenever we need
> code specific to a storage backend (eg., local, google, facebook, flickr,
> etc.) we should split it out into the respective sub-classes.

As you pointed out, we can't really use the BaseItem hierarchy for this because we would be calling the share method of a LocalItem instance, instead of a GoogleItem.

Also not all share points will correspond to an online PhotosSource (or a GoaObject). eg., share points for e-mail and removable devices.

How about introducing a separate base-class called SharePoint? I sketched out a possible solution in gnome-photos:wip/rishi/share-point. It won't even compile because, well, it is a sketch. But you can pick it up and start filling in the missing pieces to get started.
Comment 12 Umang Jain 2016-07-01 15:08:04 UTC
Created attachment 330729 [details] [review]
Proof of concept built over wip/rishi/share-point
Comment 13 Umang Jain 2016-07-06 15:33:53 UTC
Created attachment 330950 [details] [review]
Add Share Dialog
Comment 14 Umang Jain 2016-07-06 15:35:06 UTC
Created attachment 330951 [details] [review]
Add google share point
Comment 15 Umang Jain 2016-07-06 15:35:38 UTC
Created attachment 330952 [details] [review]
Add sharing action
Comment 16 Umang Jain 2016-07-06 15:36:05 UTC
Created attachment 330953 [details] [review]
Add share icon in main toolbar
Comment 17 Debarshi Ray 2016-07-07 08:45:59 UTC
Review of attachment 330950 [details] [review]:

Thanks for splitting up the patch a bit. This looks better. A few comments:

::: src/photos-share-dialog.c
@@ +84,3 @@
+  g_return_if_fail (PHOTOS_IS_BASE_ITEM (self->item));
+
+  photos_share_point_share_async (share_point, self->cancellable, photos_share_dialog_item_shared, self->item);

It will be better to make this call where photos_share_dialog_new, etc. are called. ie. in photos-application.c. Among other things, it is nice to consolidate all the backend calls in a handful of places.

I would suggest using gtk_dialog_response to emit GTK_RESPONSE_OK here, and keep track of the selected SharePoint in an instance variable. Then, in photos_application_share_response, you can call photos_share_dialog_get_share_point to get it and invoke photos_share_point_share_async.

@@ +106,3 @@
+  const gchar *name;
+
+  G_OBJECT_CLASS (photos_share_dialog_parent_class)->constructed (object);

Nitpick: a newline after chaining up will be nice.

@@ +117,3 @@
+    {
+      grid = gtk_grid_new ();
+      button = gtk_button_new ();

We don't need a GtkButton.

@@ +128,3 @@
+
+      icon = photos_share_point_get_icon (share_point);
+      image = gtk_image_new_from_gicon (icon, GTK_ICON_SIZE_DIALOG);

Instead, you can directly add the GtkImage to the grid.

@@ +137,3 @@
+
+      g_object_set_data (G_OBJECT (button), "share-point", share_point);
+      g_signal_connect (button, "clicked", G_CALLBACK (photos_share_dialog_button_clicked), self);

And use GtkListBox::row-activated, instead of GtkButton::clicked.

@@ +151,3 @@
+
+  g_clear_object (&self->cancellable);
+  g_clear_object (&self->item);

We should unref the SharePointManager too.

::: src/photos-share-point.c
@@ +307,3 @@
+  g_return_if_fail (PHOTOS_IS_BASE_ITEM (item));
+
+  photos_share_point_share (self, g_object_ref (task), cancellable, &error);

Instead of passing the GTask pointer, I would pass the BaseItem pointer.

This is because:
(a) In future, if needed, we can use photos_share_point_share as a public sync variant of photos_share_point_share_async. If we do that, it can't take a GTask as an argument.
(b) The GTask represents the photos_share_point_share_async by keeping track of the lifecycles of various objects, the callback, the cancellable and the result/error. It should be opaque to the internals of any lower layers. eg., any other async calls that we might be chaining or any sync calls run in a thread.

@@ +311,3 @@
+  {
+    g_task_return_error (task, error);
+    g_warning ("Error occured %s\n", error->message);

We don't need this g_warning because the photos_share_point_share_async/finish caller is already doing that.

::: src/photos-share-point.h
@@ +53,3 @@
+#define PHOTOS_SHARE_POINT_GET_CLASS(obj) \
+  (G_TYPE_INSTANCE_GET_CLASS ((obj), \
+   PHOTOS_TYPE_SHARE_POINT, PhotosSharePointClass))

Thanks for spotting this.

@@ +81,3 @@
 
+void                    photos_share_point_share_async            (PhotosSharePoint *self,
+                                                                   GCancellable *cancellable,

I have a feeling that you wanted to add a 'PhotosBaseItem *item' argument here. :)
Comment 18 Debarshi Ray 2016-07-07 08:58:50 UTC
Review of attachment 330951 [details] [review]:

::: src/photos-google-share-point.c
@@ +218,3 @@
+  GError *error = NULL;
+
+  gdata_authorizer_refresh_authorization_finish (GDATA_AUTHORIZER (self->authorizer), res, &error);

We won't need these callbacks anymore because all this code can move to photos_google_share_point_share. See below.

@@ +231,3 @@
+
+static void
+photos_google_share_point_share (PhotosSharePoint *share_point, GTask *task, GCancellable *cancellable, GError **error)

We should accept a BaseItem pointer, not a GTask pointer here. See the comments on attachment 330950 [details] [review].

@@ +250,3 @@
+
+  g_object_set_data (G_OBJECT (self), "task", task);
+  gdata_authorizer_refresh_authorization_async (GDATA_AUTHORIZER (self->authorizer),

We should only use synchronous calls here, because this virtual function is not being called from the main/UI thread (thanks to g_task_run_in_thread) [*].

[*] Note that this is another way of implementing an asynchronous operation. The other alternative is to chain together a series of existing async calls.

::: src/photos-google-share-point.h
@@ +25,3 @@
+#include <goa/goa.h>
+#include <gdata/gdata.h>
+#include <gdata/gdata-goa-authorizer.h>

We won't need any of these headers, except glib-object.h.

@@ +27,3 @@
+#include <gdata/gdata-goa-authorizer.h>
+
+#include "photos-source.h"

Ditto.
Comment 19 Debarshi Ray 2016-07-07 09:13:47 UTC
Review of attachment 330952 [details] [review]:

Looks OK barring adjustments needed due to changes in the other patches.
Comment 20 Debarshi Ray 2016-07-07 09:14:52 UTC
Review of attachment 330953 [details] [review]:

Looks OK, but let's wait for the other patches to get ready before we merge this.
Comment 21 Debarshi Ray 2016-07-07 15:40:35 UTC
(In reply to Debarshi Ray from comment #17)
> @@ +137,3 @@
> +
> +      g_object_set_data (G_OBJECT (button), "share-point", share_point);
> +      g_signal_connect (button, "clicked", G_CALLBACK
> (photos_share_dialog_button_clicked), self);
> 
> And use GtkListBox::row-activated, instead of GtkButton::clicked.

I meant GtkFlowBox::child-activated. Sorry for the confusion.
Comment 22 Umang Jain 2016-07-09 19:21:21 UTC
Created attachment 331141 [details] [review]
Add share dialog
Comment 23 Umang Jain 2016-07-09 19:22:17 UTC
Created attachment 331142 [details] [review]
Add Google share point
Comment 24 Umang Jain 2016-07-09 19:23:23 UTC
Created attachment 331143 [details] [review]
photos-application: Add share action
Comment 25 Umang Jain 2016-07-09 19:23:59 UTC
Created attachment 331144 [details] [review]
main-toolbar: Add share icon
Comment 26 Umang Jain 2016-07-09 19:29:56 UTC
Created attachment 331145 [details] [review]
photos-application: Mark application as busy while uploading
Comment 27 Debarshi Ray 2016-07-11 09:07:03 UTC
Review of attachment 331141 [details] [review]:

Thanks for the new patches, Umang. This is looking much better. It is almost there except some minor issues.

::: src/photos-share-point.c
@@ +286,3 @@
+
+static void
+photos_share_point_share (PhotosSharePoint *self, PhotosBaseItem *item, GCancellable *cancellable, GError **error)

Nitpick: the private static methods should be moved further up in the file.

@@ +302,3 @@
+{
+  PhotosSharePoint *self = PHOTOS_SHARE_POINT (source_object);
+  PhotosBaseItem *item = PHOTOS_BASE_ITEM (g_task_get_task_data (task));

We can use task_data instead of calling the getter method.

@@ +307,3 @@
+  photos_share_point_share (self, item, cancellable, &error);
+  if (error != NULL)
+  {

Nitpick: the identation is off.

@@ +324,3 @@
+{
+  GTask *task;
+

Nitpick: we should have some g_return_* guards here.

::: src/photos-share-point.h
@@ +70,2 @@
   /* virtual methods */
+  void        (*share)    (PhotosSharePoint *self, PhotosBaseItem *item, GCancellable *cancellable, GError **error);

It will be more idiomatic if the vfunc returned a gboolean to match the _finish method.

@@ +84,3 @@
+                                                                   GCancellable *cancellable,
+                                                                   GAsyncReadyCallback callback,
+                                                                   gpointer user_data);

We should start merging this with the patches in wip/rishi/share-point.
Comment 28 Debarshi Ray 2016-07-11 09:08:54 UTC
(In reply to Debarshi Ray from comment #27)
> ::: src/photos-share-point.h
> @@ +70,2 @@
>    /* virtual methods */
> +  void        (*share)    (PhotosSharePoint *self, PhotosBaseItem *item,
> GCancellable *cancellable, GError **error);
> 
> It will be more idiomatic if the vfunc returned a gboolean to match the
> _finish method.
> 
> @@ +84,3 @@
> +                                                                  
> GCancellable *cancellable,
> +                                                                  
> GAsyncReadyCallback callback,
> +                                                                   gpointer
> user_data);
> 
> We should start merging this with the patches in wip/rishi/share-point.

I took the liberty to split out the SharePoint changes and merged them with patches in the branch. Let me know if something is wrong.
Comment 29 Debarshi Ray 2016-07-11 09:36:10 UTC
Review of attachment 331142 [details] [review]:

::: src/photos-google-share-point.c
@@ +88,3 @@
+  gchar *filename = NULL;
+
+  g_return_if_fail (PHOTOS_IS_GOOGLE_SHARE_POINT (self));

No need for the g_return_* guard here. We usually use it for public (and public-ish) functions. This one is purely an internal convenience method.

@@ +94,3 @@
+                                 G_FILE_QUERY_INFO_NONE,
+                                 NULL,
+                                 NULL);

Can't we can avoid this g_file_query_info call? photos_base_item_get_mime_type will give us the content/MIME type [*], and photos_base_item_get_filename will give us the filename.

[*] Content and MIME types are the same on UNIX. They are different on Windows.

@@ +99,3 @@
+  fn = photos_base_item_get_filename (item);
+  filename = photos_utils_filename_strip_extension (fn);
+  gdata_entry_set_title (GDATA_ENTRY (file_entry), filename);

I think the 'title' creation can be replaced with photos_base_item_get_name_with_fallback.

@@ +121,3 @@
+        g_warning ("Unable to create upload stream: %s", error->message);
+
+      g_error_free (error);

We should propagate the GError.

@@ +129,3 @@
+  if (error != NULL)
+    {
+      g_warning ("Failed to read file: %s\n", error->message);

Ditto.

@@ +166,3 @@
+
+
+static void

Should return a gboolean. See the comments for attachment 331141 [details] [review].

@@ +173,3 @@
+  GoaObject *goa_object;
+
+  g_return_if_fail (PHOTOS_IS_GOOGLE_SHARE_POINT (self));

No need for the g_return_* guard here because we have already checked it in the _async method and the 'in thread' sync method.

@@ +179,3 @@
+
+  self->authorizer = gdata_goa_authorizer_new (goa_object);
+  self->service = gdata_picasaweb_service_new (GDATA_AUTHORIZER (self->authorizer));

We should create self->authorizer and self->service in photos_google_share_point_init.

@@ +180,3 @@
+  self->authorizer = gdata_goa_authorizer_new (goa_object);
+  self->service = gdata_picasaweb_service_new (GDATA_AUTHORIZER (self->authorizer));
+  if (!gdata_service_is_authorized (GDATA_SERVICE (self->service)))

Can this really fail unless it is a programming error or a very old version of libgdata?

@@ +189,3 @@
+  if (!gdata_authorizer_refresh_authorization (GDATA_AUTHORIZER (self->authorizer), cancellable, error))
+     {
+        g_warning ("Failed to refresh GData service");

We don't need to print a WARNING here because we are propagating the GError upwards. The photos_share_point_share_async caller can print a WARNING if required.

@@ +193,3 @@
+     }
+
+  photos_google_share_point_share_image (self, cancellable, item);

We need to pass the GError ** to _share_image. Although, I think that after these changes, the code will become shorter and we can combine everything in one function.
Comment 30 Debarshi Ray 2016-07-11 09:42:04 UTC
Review of attachment 331143 [details] [review]:

::: src/photos-application.c
@@ +398,3 @@
             && photos_base_item_can_edit (item));
   g_simple_action_set_enabled (self->edit_action, enable);
+  g_simple_action_set_enabled (self->share_action, enable);

This won't work in the overview's selection mode.
Comment 31 Umang Jain 2016-07-15 09:49:00 UTC
 
> We should create self->authorizer and self->service in
> photos_google_share_point_init.
> 
> @@ +180,3 @@
> +  self->authorizer = gdata_goa_authorizer_new (goa_object);
> +  self->service = gdata_picasaweb_service_new (GDATA_AUTHORIZER
> (self->authorizer));
> +  if (!gdata_service_is_authorized (GDATA_SERVICE (self->service)))
> 

I receive a segfault when I try to initialize authorizer and service in the photos_google_share_point_init function.

According to what I had in mind, the share point pointer is passed from photos_share_point_share to photos_google_share_point_share and then to create its authorizer and service. So, that is how I coded that part.
Comment 32 Debarshi Ray 2016-07-15 10:01:18 UTC
(In reply to Umang Jain from comment #31)
>  
> > We should create self->authorizer and self->service in
> > photos_google_share_point_init.
> > 
> > @@ +180,3 @@
> > +  self->authorizer = gdata_goa_authorizer_new (goa_object);
> > +  self->service = gdata_picasaweb_service_new (GDATA_AUTHORIZER
> > (self->authorizer));
> > +  if (!gdata_service_is_authorized (GDATA_SERVICE (self->service)))
> > 
> 
> I receive a segfault when I try to initialize authorizer and service in the
> photos_google_share_point_init function.

That's because photos_share_point_get_source returns NULL in google_share_point_init because the SharePoint:source property hasn't been set yet. Try to do it in the GoogleSharePoint:constructed vfunc instead.
Comment 33 Umang Jain 2016-07-15 13:13:32 UTC
Created attachment 331584 [details] [review]
Add share dialog
Comment 34 Umang Jain 2016-07-15 13:13:59 UTC
Created attachment 331585 [details] [review]
Add Google share point
Comment 35 Umang Jain 2016-07-15 14:12:50 UTC
Created attachment 331588 [details] [review]
Add photos share point
Comment 36 Umang Jain 2016-07-15 14:13:19 UTC
Created attachment 331589 [details] [review]
utils: Add an extension point for PhotosSharePoint
Comment 37 Umang Jain 2016-07-15 14:14:01 UTC
Created attachment 331590 [details] [review]
Add photos share point manager
Comment 38 Umang Jain 2016-07-15 17:01:31 UTC
Created attachment 331604 [details] [review]
application: Add share action
Comment 39 Umang Jain 2016-07-15 17:43:48 UTC
Created attachment 331607 [details] [review]
selection-toolbar: Add share icon
Comment 40 Umang Jain 2016-07-15 18:25:13 UTC
Created attachment 331609 [details] [review]
application: Enable share action

An attempt to avoid sharing "google-item" to its own source(i.e. Google) via Google share point.

In the future, this should be more appropriately designed such that an image retrieved from a source shouldn't be able to get uploaded to its own source.
Comment 41 Debarshi Ray 2016-07-19 10:14:29 UTC
Review of attachment 331585 [details] [review]:

Thanks, Umang. This looks really good. Just a couple of questions below:

::: src/photos-google-share-point.c
@@ +94,3 @@
+
+static void
+photos_google_share_point_share_image (PhotosGoogleSharePoint *self, GCancellable *cancellable, PhotosBaseItem *item, GError **error)

Nitpick: We usually put the GCancellable after the actual parameters.

@@ +111,3 @@
+                                 G_FILE_QUERY_INFO_NONE,
+                                 NULL,
+                                 NULL);

Can't we can avoid this g_file_query_info call? photos_base_item_get_mime_type will give us the content/MIME type [*], and photos_base_item_get_filename will give us the filename.
 
[*] Content and MIME types are the same on UNIX. They are different on Windows.

@@ +184,3 @@
+  g_return_val_if_fail (PHOTOS_IS_GOOGLE_SHARE_POINT (self), FALSE);
+
+  if (!gdata_service_is_authorized (GDATA_SERVICE (self->service)))

Can this really fail unless it is a programming error or a very old version of libgdata?

::: src/photos-google-share-point.h
@@ +24,3 @@
+#include <glib-object.h>
+
+#include "photos-source.h"

Nitpick: We only need photos-source.h in the .c file.

@@ +48,3 @@
+#define PHOTOS_GOOGLE_SHARE_POINT_GET_CLASS(obj) \
+  (G_TYPE_INSTANCE_GET_CLASS ((obj), \
+   PHOTOS_TYPE_GOOGLE_SHARE_POINT, PhotosGoogleSharePointClass))

Nitpick: We don't need the *CLASS macros because this is a final class.
Comment 42 Debarshi Ray 2016-07-19 10:20:54 UTC
(In reply to Debarshi Ray from comment #41)
> Review of attachment 331585 [details] [review] [review]:
> @@ +111,3 @@
> +                                 G_FILE_QUERY_INFO_NONE,
> +                                 NULL,
> +                                 NULL);
> 
> Can't we can avoid this g_file_query_info call?
> photos_base_item_get_mime_type will give us the content/MIME type [*], and
> photos_base_item_get_filename will give us the filename.
>  
> [*] Content and MIME types are the same on UNIX. They are different on
> Windows.

Oh, I see that you already made the change but forgot to remove the g_file_query_info call.
Comment 43 Debarshi Ray 2016-07-20 08:43:33 UTC
Review of attachment 331609 [details] [review]:

::: src/photos-base-item.c
@@ +1859,3 @@
+
+  if (PHOTOS_IS_GOOGLE_ITEM (self))
+    return FALSE;

It should also check for collections. ie. FALSE when priv->collection is TRUE.
Comment 44 Debarshi Ray 2016-07-20 09:02:44 UTC
Review of attachment 331144 [details] [review]:

::: src/photos-main-toolbar.c
@@ +572,3 @@
 
+  share_button = gtk_button_new_from_icon_name (PHOTOS_ICON_IMAGE_SHARE_SYMBOLIC, GTK_ICON_SIZE_BUTTON);
+  gtk_actionable_set_action_name (GTK_ACTIONABLE (share_button), "app.share-current");

The order of the buttons doesn't match the mockups.

We have:
[star][share][edit][hamburger]

The mockups have:
[star][edit][share][hamburger]
Comment 45 Debarshi Ray 2016-07-20 09:13:43 UTC
Review of attachment 331145 [details] [review]:

This should be squashed with attachment 331604 [details] [review]
Comment 46 Debarshi Ray 2016-07-20 09:18:06 UTC
(In reply to Debarshi Ray from comment #41)
> @@ +184,3 @@
> +  g_return_val_if_fail (PHOTOS_IS_GOOGLE_SHARE_POINT (self), FALSE);
> +
> +  if (!gdata_service_is_authorized (GDATA_SERVICE (self->service)))
> 
> Can this really fail unless it is a programming error or a very old version
> of libgdata?

No, this can't really fail at run-time unless there is a programming error.

gdata_service_is_authorized checks if the GDataAuthorizer has all the GDataAuthorizationDomains needed by the GDataService. The list of GDataAuthorizationDomains changes when the photos switch is toggled in Settings -> Online Accounts, and we re-create the Sources and SharePoints when that happens. Hence it should be fine.
Comment 47 Debarshi Ray 2016-07-20 09:26:57 UTC
Review of attachment 331584 [details] [review]:

::: src/photos-share-dialog.c
@@ +36,3 @@
+  PhotosBaseManager *mngr;
+  PhotosSharePoint *selected;
+  GCancellable *cancellable;

Nitpick: 'cancellable' is unused.

@@ +60,3 @@
+photos_share_dialog_child_activated (GtkFlowBox *box, GtkFlowBoxChild *child, PhotosShareDialog *self)
+{
+  g_return_if_fail (self != NULL);

Nitpick: PHOTOS_IS_SHARE_DIALOG would be even better than just a NULL-check. We can do the same for 'child'.

@@ +78,3 @@
+  GtkWidget *grid;
+  GtkWidget *image;
+  GtkWidget *label;

Nitpick: some of these can go into the inner while.

@@ +87,3 @@
+  gtk_flow_box_set_selection_mode (GTK_FLOW_BOX (self->flow_box), GTK_SELECTION_NONE);
+  gtk_flow_box_set_homogeneous (GTK_FLOW_BOX (self->flow_box), TRUE);
+  gtk_container_set_border_width (GTK_CONTAINER (self->flow_box), 25);

Can't these be done in the UI file?

@@ +94,3 @@
+  while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &share_point))
+    {
+      grid = gtk_grid_new ();

Nitpick: I prefer to set the orientation to VERTICAL and just use gtk_container_add below.

@@ +107,3 @@
+      image = gtk_image_new_from_gicon (icon, GTK_ICON_SIZE_DIALOG);
+      gtk_grid_attach (GTK_GRID (grid), image, 0, 0, 1, 1);
+      gtk_grid_attach_next_to (GTK_GRID (grid), label, image, GTK_POS_BOTTOM, 1, 1);

Looking at the mockups, we need some row-spacing between the icon and the label.

::: src/photos-share-dialog.ui
@@ +5,3 @@
+    <template class="PhotosShareDialog" parent="GtkDialog">
+    <property name="border_width">5</property>
+    <property name="title" translatable="yes" context="dialog title">Share</property>

Should be "Share 1 Photo". We don't support sharing multiple photos, so we could just hard-code it. Also, this file should be listed in po/POTFILES.in.
Comment 48 Debarshi Ray 2016-07-20 09:30:56 UTC
I have fixed all the 'nitpicks' in wip/rishi/share-point, and left the other issues for you.
Comment 49 Debarshi Ray 2016-07-24 11:37:46 UTC
Review of attachment 331585 [details] [review]:

::: src/photos-google-share-point.c
@@ +135,3 @@
+
+  local_error = NULL;
+  file_stream = g_file_read (file_data, cancellable, &local_error);

The BaseItem might have been edited, and we want to upload the changes too. This will only read the source file but not the edits (if any). We need to do something similar to photos_base_item_save_async, but instead of saving to disk, it should write to a GOutputStream (which is what GDataUploadStream is). Thus, we need a photos_base_item_save_to_stream_async.

I wonder if we can continue to implement share_async in terms of sync operations in a thread because it is unsafe to touch BaseItem->edit_graph, etc. from anything but the main thread. We'll see.
Comment 50 Debarshi Ray 2016-07-27 21:41:11 UTC
I added a e-mail share point in gnome-photos/wip/rishi/share-point. It attaches an image to the default mail application. I also fixed up some of the remaining issues. Most importantly, edits are now preserved.

Some things left to do:
  * merging uploaded copies with their local counterparts
  * some CSS to draw the grey boxes around icons: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/photos/wires-sharing.png
Comment 51 Umang Jain 2016-08-05 13:29:25 UTC
Created attachment 332801 [details] [review]
Failure notification for sharing
Comment 52 Umang Jain 2016-08-06 15:15:48 UTC
A lot of restructuring took place in wip/rishi/share-point including small iterations that had been discussed on IRC. Therefore, from this point it's better to have fresh versions of all patches.
Comment 53 Umang Jain 2016-08-06 15:17:18 UTC
Created attachment 332830 [details] [review]
utils: Add photos_utils_app_info_launch_uri
Comment 54 Umang Jain 2016-08-06 15:17:59 UTC
Created attachment 332831 [details] [review]
base-item: Use photos_utils_app_info_launch_uri
Comment 55 Umang Jain 2016-08-06 15:18:42 UTC
Created attachment 332832 [details] [review]
filterable, source: Add get_builtin
Comment 56 Umang Jain 2016-08-06 15:19:00 UTC
Created attachment 332833 [details] [review]
base-manager: Use photos_filterable_get_builtin
Comment 57 Umang Jain 2016-08-06 15:20:49 UTC
Created attachment 332834 [details] [review]
base-item: Split the code to encode a GeglBuffer and copy the metadata
Comment 58 Umang Jain 2016-08-06 15:21:14 UTC
Created attachment 332835 [details] [review]
base-item: Add photos_base_item_save_to_stream_async
Comment 59 Umang Jain 2016-08-06 15:21:38 UTC
Created attachment 332836 [details] [review]
Add PhotosSharePoint
Comment 60 Umang Jain 2016-08-06 15:22:17 UTC
Created attachment 332837 [details] [review]
utils: Add an extension point for PhotosSharePoint sub-classes
Comment 61 Umang Jain 2016-08-06 15:23:41 UTC
Created attachment 332838 [details] [review]
Add PhotosSharePointOnline
Comment 62 Umang Jain 2016-08-06 15:25:11 UTC
Created attachment 332839 [details] [review]
utils: Add an extension point for PhotosSharePointOnline sub-classes
Comment 63 Umang Jain 2016-08-06 15:25:43 UTC
Created attachment 332840 [details] [review]
Add PhotosSharePointManager
Comment 64 Umang Jain 2016-08-06 15:26:11 UTC
Created attachment 332841 [details] [review]
Add PhotosSharePointGoogle
Comment 65 Umang Jain 2016-08-06 15:26:49 UTC
Created attachment 332842 [details] [review]
Add PhotosSharePointEmail
Comment 66 Umang Jain 2016-08-06 15:27:42 UTC
Created attachment 332843 [details] [review]
Add PhotosShareDialog
Comment 67 Umang Jain 2016-08-06 15:33:39 UTC
Created attachment 332844 [details] [review]
application: Add share action
Comment 68 Umang Jain 2016-08-06 15:34:38 UTC
Created attachment 332845 [details] [review]
application: Enable Share action
Comment 69 Umang Jain 2016-08-06 15:35:09 UTC
Created attachment 332846 [details] [review]
selection-toolbar: Add share icon
Comment 70 Umang Jain 2016-08-06 15:35:36 UTC
Created attachment 332847 [details] [review]
Add share icon in main toolbar
Comment 71 Umang Jain 2016-08-06 15:36:23 UTC
Created attachment 332848 [details] [review]
Add notification for sharing failure
Comment 72 Debarshi Ray 2016-08-09 17:18:43 UTC
Review of attachment 332848 [details] [review]:

Thanks for the notification, Umang. Some comments:

::: src/photos-share-notification.c
@@ +40,3 @@
+  GtkWidget *ntfctn_mngr;
+  GError *error;
+  GFile *file; // Why ?

Indeed, the GFile *file doesn't make sense here. It only made sense in ExportNotification. ;)

@@ +124,3 @@
+
+      if (g_error_matches (self->error, GDATA_SERVICE_ERROR,  GDATA_SERVICE_ERROR_ENTRY_ALREADY_INSERTED))
+        msg = g_strdup (_("Failed to upload photo: Image already inserted "));

When can this happen? I see that repeatedly sharing the same item again and again doesn't throw this error.

@@ +127,3 @@
+
+      else if (g_error_matches (self->error, GDATA_SERVICE_ERROR, GDATA_SERVICE_ERROR_AUTHENTICATION_REQUIRED))
+        msg = g_strdup (_("Failed to upload photo: Service not authorized"));

I'd like to avoid putting SharePoint-specific code in the notification. These error domains and error codes are specific to each SharePoint implementation, so, ideally, they should be parsed in the sub-classes themselves.

How about adding a new vfunc to SharePoint:
gchar *parse_error (GError *error);

@@ +269,3 @@
+
+void
+photos_share_notification_new (GList *items, GFile *file)

The 'file' parameter doesn't make sense. It should be removed. See above.

I would actually remove this function and everything that is only for the 'success case'. It isn't clear if we will ever add a notification for success, so let's not needlessly write placeholder code for it.

@@ +279,3 @@
+
+void
+photos_share_notification_new_with_error (GError *error)

I think we will need to pass the SharePoint object. See above.

::: src/photos-share-notification.h
@@ +36,3 @@
+#define PHOTOS_IS_SHARE_NOTIFICATION(obj) \
+  (G_TYPE_CHECK_INSTANCE_TYPE ((obj), \
+   PHOTOS_TYPE_SHARE_NOTIFICATION))

Let's use G_DECLARE_FINAL_TYPE for new code.
Comment 73 Debarshi Ray 2016-08-09 17:44:23 UTC
Review of attachment 332835 [details] [review]:

I just realized that sharing doesn't work from the selection mode because the item might not have been loaded yet.

::: src/photos-base-item.c
@@ +3181,3 @@
+  g_task_set_task_data (task, data, (GDestroyNotify) photos_base_item_save_to_stream_data_free);
+
+  graph = photos_pipeline_get_graph (priv->pipeline);

We should first call photos_base_item_load_async (like we did in commit c6aed42e817886754b0d16eeaaf40d88577b2787) before doing anything.

Similarly in photos_base_item_save_async.
Comment 74 Umang Jain 2016-08-14 22:42:59 UTC
Created attachment 333301 [details] [review]
Add notification for sharing failure
Comment 75 Umang Jain 2016-08-15 09:40:02 UTC
Created attachment 333316 [details] [review]
base-item: Add load_async to save_to_stream_async for selection mode sharing
Comment 76 Umang Jain 2016-08-15 09:49:27 UTC
Created attachment 333320 [details] [review]
base-item: Add load_async to save_async for selection mode sharing
Comment 77 Debarshi Ray 2016-08-20 16:32:57 UTC
Review of attachment 333301 [details] [review]:

Thanks for the new patch, Umang. It would be nicer to move the photos-share-point-google.c and photos-share-point.[ch] changes to the commits where those files are created. A few more comments below:

::: src/photos-share-notification.c
@@ +23,3 @@
+
+#include <gio/gdesktopappinfo.h>
+#include <glib/gi18n.h>

These two headers are not used.

@@ +26,3 @@
+#include <gtk/gtk.h>
+#include <gdata/gdata.h>
+#include <gdata/gdata-goa-authorizer.h>

Same here.

@@ +32,3 @@
+#include "photos-notification-manager.h"
+
+struct _PhotosShareNotification

Missing newline between struct definition and #includes.

@@ +44,3 @@
+{
+  GtkGridClass parent_class;
+};

G_DECLARE_FINAL_TYPE already defines the PhotosShareNotificationClass struct. No need to define it again.

@@ +225,3 @@
+photos_share_notification_new_with_error (PhotosSharePoint *share_point, GError *error)
+{
+  g_return_if_fail (error != NULL);

We should also check for PHOTOS_IS_SHARE_POINT.

::: src/photos-share-point-google.c
@@ +227,3 @@
+{
+  gchar *msg;
+  g_assert_nonnull (error);

This should be a g_return_val_if_fail in the base class. See below.

@@ +230,3 @@
+
+  if (g_error_matches (error, GDATA_SERVICE_ERROR, GDATA_SERVICE_ERROR_AUTHENTICATION_REQUIRED))
+    msg = g_strdup (("Failed to upload photo: Service not authorized"));

We should #include <glib/gi18n.h> and the _ is missing. Also this file should be in po/POTFILES.in.

@@ +232,3 @@
+    msg = g_strdup (("Failed to upload photo: Service not authorized"));
+  else
+    msg = g_strdup (("Failed to upload photo"));

Ditto - missing _.

::: src/photos-share-point.c
@@ +91,3 @@
+photos_share_point_parse_error (PhotosSharePoint *self, GError *error)
+{
+  g_return_val_if_fail (PHOTOS_IS_SHARE_POINT (self), NULL);

We should check the GError too.

::: src/photos-share-point.h
@@ +51,3 @@
                                      gpointer user_data);
   gboolean        (*share_finish)   (PhotosSharePoint *self, GAsyncResult *res, GError **error);
+  gchar          *(*parse_error)    (PhotosSharePoint *self, GError *error);

Nitpick: Move it upwards to maintain the ordering.

@@ +68,3 @@
                                                                    GError **error);
 
+gchar                  *photos_share_point_parse_error            (PhotosSharePoint *self, GError *error);

Ditto.
Comment 78 Debarshi Ray 2016-08-20 16:36:15 UTC
Review of attachment 333316 [details] [review]:

::: src/photos-base-item.c
@@ +3164,3 @@
 
+static void
+photos_base_item_loaded_save_to_stream (GObject *source_object, GAsyncResult *res, gpointer user_data)

Nitpick: Should be photos_base_item_save_to_stream_load to match the convention.

@@ +3181,2 @@
   g_return_if_fail (!priv->collection);
+  g_return_if_fail (G_IS_OUTPUT_STREAM (data->ostream));

These assertions should remain in photos_base_item_save_to_stream_async.

@@ +3183,3 @@
   g_return_if_fail (priv->edit_graph != NULL);
   g_return_if_fail (priv->filename != NULL && priv->filename[0] != '\0');
   g_return_if_fail (priv->load_graph != NULL);

Now that we implicitly load the item, only the priv->filename check makes sense.

@@ +3190,1 @@
   graph = photos_pipeline_get_graph (priv->pipeline);

We need to call photos_base_item_load_finish, and handle any errors.
Comment 79 Debarshi Ray 2016-08-20 16:38:48 UTC
Review of attachment 333320 [details] [review]:

::: src/photos-base-item.c
@@ +3127,3 @@
   g_return_if_fail (priv->edit_graph != NULL);
   g_return_if_fail (priv->filename != NULL && priv->filename[0] != '\0');
   g_return_if_fail (priv->load_graph != NULL);

Same thing about the assertions as in attachment 333316 [details] [review]

@@ +3134,1 @@
   graph = photos_pipeline_get_graph (priv->pipeline);

We should call photos_base_item_load_finish and handle any error.
Comment 80 Debarshi Ray 2016-08-20 16:39:37 UTC
Comment on attachment 332832 [details] [review]
filterable, source: Add get_builtin

Pushed to master.
Comment 81 Debarshi Ray 2016-08-20 16:39:56 UTC
Comment on attachment 332833 [details] [review]
base-manager: Use photos_filterable_get_builtin

Pushed to master.
Comment 82 Umang Jain 2016-08-21 10:59:26 UTC
Attaching new set of patches from here.
Comment 83 Umang Jain 2016-08-21 11:00:15 UTC
Created attachment 333787 [details] [review]
utils: Add photos_utils_app_info_launch_uri
Comment 84 Umang Jain 2016-08-21 11:00:53 UTC
Created attachment 333788 [details] [review]
base-item: Use photos_utils_app_info_launch_uri
Comment 85 Umang Jain 2016-08-21 11:01:26 UTC
Created attachment 333789 [details] [review]
base-item: Split the code to encode a GeglBuffer and copy the metadata
Comment 86 Umang Jain 2016-08-21 11:02:19 UTC
Created attachment 333790 [details] [review]
base-item: Add photos_base_item_save_to_stream_async
Comment 87 Umang Jain 2016-08-21 11:02:50 UTC
Created attachment 333791 [details] [review]
base-item: Transparently load item in photos_base_item_save_async
Comment 88 Umang Jain 2016-08-21 11:03:22 UTC
Created attachment 333792 [details] [review]
Add PhotosSharePoint
Comment 89 Umang Jain 2016-08-21 11:03:51 UTC
Created attachment 333793 [details] [review]
utils: Add an extension point for PhotosSharePoint sub-classes
Comment 90 Umang Jain 2016-08-21 11:04:20 UTC
Created attachment 333794 [details] [review]
Add PhotosSharePointOnline
Comment 91 Umang Jain 2016-08-21 11:04:51 UTC
Created attachment 333795 [details] [review]
utils: Add an extension point for PhotosSharePointOnline sub-classes
Comment 92 Umang Jain 2016-08-21 11:06:41 UTC
Created attachment 333796 [details] [review]
Add PhotosSharePointManager
Comment 93 Umang Jain 2016-08-21 11:07:23 UTC
Created attachment 333797 [details] [review]
Add PhotosSharePointGoogle
Comment 94 Umang Jain 2016-08-21 11:08:20 UTC
Created attachment 333798 [details] [review]
Add PhotosSharePointEmail
Comment 95 Umang Jain 2016-08-21 11:09:07 UTC
Created attachment 333799 [details] [review]
Add PhotosShareDialog
Comment 96 Umang Jain 2016-08-21 11:09:46 UTC
Created attachment 333800 [details] [review]
Add PhotosShareNotification
Comment 97 Umang Jain 2016-08-21 11:10:11 UTC
Created attachment 333801 [details] [review]
application: Add share action
Comment 98 Umang Jain 2016-08-21 11:10:56 UTC
Created attachment 333802 [details] [review]
application: Enable Share action
Comment 99 Umang Jain 2016-08-21 11:12:05 UTC
Created attachment 333803 [details] [review]
selection-toolbar: Add Share button
Comment 100 Umang Jain 2016-08-21 11:12:34 UTC
Created attachment 333804 [details] [review]
main-toolbar: Add Share button
Comment 101 Debarshi Ray 2016-08-21 23:33:45 UTC
Review of attachment 333802 [details] [review]:

::: src/photos-base-item.c
@@ +2299,3 @@
+
+  if (PHOTOS_IS_GOOGLE_ITEM (self))
+    return FALSE;

This was hacky to begin with, and now that we have an e-mail share-point, this isn't going to work. For example, a Google item should still be shareable over e-mail as long as we have an e-mail client.

This decision should be taken in SharePointManager after matching the BaseItem with the available SharePoints.
Comment 102 Debarshi Ray 2016-08-21 23:43:04 UTC
Review of attachment 333799 [details] [review]:

::: src/Makefile.am
@@ +204,3 @@
 	photos-settings.h \
+	photos-share-dialog.c \
+	photos-share-dialog.h \

We should also add photos-share-dialog.ui to EXTRA_DIST.

::: src/photos-share-dialog.c
@@ +75,3 @@
+  G_OBJECT_CLASS (photos_share_dialog_parent_class)->constructed (object);
+
+  share_points = photos_base_manager_get_objects (PHOTOS_BASE_MANAGER (self->shr_pnt_mngr));

Now that we have an e-mail share-point (see attachment 333802 [details] [review] for some background), we can't just do this. eg., for a Google item we don't want to offer sharing to Google but still offer e-mail. We need to filter the share-points based on the BaseItem.
Comment 103 Debarshi Ray 2016-08-22 06:18:31 UTC
Created attachment 333858 [details] [review]
Add PhotosSharePoint
Comment 104 Debarshi Ray 2016-08-22 06:20:33 UTC
Created attachment 333859 [details] [review]
Add PhotosSharePointManager
Comment 105 Umang Jain 2016-08-22 08:25:53 UTC
Created attachment 333862 [details] [review]
Add PhotosSharePointGoogle
Comment 106 Umang Jain 2016-08-22 08:26:46 UTC
Created attachment 333863 [details] [review]
Add PhotosSharePointEmail
Comment 107 Umang Jain 2016-08-22 08:27:28 UTC
Created attachment 333864 [details] [review]
Add PhotosShareDialog
Comment 108 Umang Jain 2016-08-22 08:28:23 UTC
Created attachment 333865 [details] [review]
Add PhotosShareNotification
Comment 109 Umang Jain 2016-08-22 08:29:11 UTC
Created attachment 333866 [details] [review]
application: Add share action
Comment 110 Umang Jain 2016-08-22 08:33:13 UTC
Created attachment 333867 [details] [review]
application: Add share action
Comment 111 Debarshi Ray 2016-08-22 15:57:07 UTC
It's been over 110 comments. Let's use separate bugs for the remaining items:
(a) "merging" shared items with the originals in the UI
(b) CSS love in the sharing dialog
...

Thanks for your work so far, Umang.