GNOME Bugzilla – Bug 751181
Photo upload/sharing
Last modified: 2016-08-22 15:57:07 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).
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
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.
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.
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.
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.
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.
(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.
(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.
(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).
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.
(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.
Created attachment 330729 [details] [review] Proof of concept built over wip/rishi/share-point
Created attachment 330950 [details] [review] Add Share Dialog
Created attachment 330951 [details] [review] Add google share point
Created attachment 330952 [details] [review] Add sharing action
Created attachment 330953 [details] [review] Add share icon in main toolbar
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. :)
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.
Review of attachment 330952 [details] [review]: Looks OK barring adjustments needed due to changes in the other patches.
Review of attachment 330953 [details] [review]: Looks OK, but let's wait for the other patches to get ready before we merge this.
(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.
Created attachment 331141 [details] [review] Add share dialog
Created attachment 331142 [details] [review] Add Google share point
Created attachment 331143 [details] [review] photos-application: Add share action
Created attachment 331144 [details] [review] main-toolbar: Add share icon
Created attachment 331145 [details] [review] photos-application: Mark application as busy while uploading
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.
(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.
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.
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.
> 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.
(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.
Created attachment 331584 [details] [review] Add share dialog
Created attachment 331585 [details] [review] Add Google share point
Created attachment 331588 [details] [review] Add photos share point
Created attachment 331589 [details] [review] utils: Add an extension point for PhotosSharePoint
Created attachment 331590 [details] [review] Add photos share point manager
Created attachment 331604 [details] [review] application: Add share action
Created attachment 331607 [details] [review] selection-toolbar: Add share icon
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.
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.
(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.
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.
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]
Review of attachment 331145 [details] [review]: This should be squashed with attachment 331604 [details] [review]
(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.
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.
I have fixed all the 'nitpicks' in wip/rishi/share-point, and left the other issues for you.
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.
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
Created attachment 332801 [details] [review] Failure notification for sharing
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.
Created attachment 332830 [details] [review] utils: Add photos_utils_app_info_launch_uri
Created attachment 332831 [details] [review] base-item: Use photos_utils_app_info_launch_uri
Created attachment 332832 [details] [review] filterable, source: Add get_builtin
Created attachment 332833 [details] [review] base-manager: Use photos_filterable_get_builtin
Created attachment 332834 [details] [review] base-item: Split the code to encode a GeglBuffer and copy the metadata
Created attachment 332835 [details] [review] base-item: Add photos_base_item_save_to_stream_async
Created attachment 332836 [details] [review] Add PhotosSharePoint
Created attachment 332837 [details] [review] utils: Add an extension point for PhotosSharePoint sub-classes
Created attachment 332838 [details] [review] Add PhotosSharePointOnline
Created attachment 332839 [details] [review] utils: Add an extension point for PhotosSharePointOnline sub-classes
Created attachment 332840 [details] [review] Add PhotosSharePointManager
Created attachment 332841 [details] [review] Add PhotosSharePointGoogle
Created attachment 332842 [details] [review] Add PhotosSharePointEmail
Created attachment 332843 [details] [review] Add PhotosShareDialog
Created attachment 332844 [details] [review] application: Add share action
Created attachment 332845 [details] [review] application: Enable Share action
Created attachment 332846 [details] [review] selection-toolbar: Add share icon
Created attachment 332847 [details] [review] Add share icon in main toolbar
Created attachment 332848 [details] [review] Add notification for sharing failure
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.
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.
Created attachment 333301 [details] [review] Add notification for sharing failure
Created attachment 333316 [details] [review] base-item: Add load_async to save_to_stream_async for selection mode sharing
Created attachment 333320 [details] [review] base-item: Add load_async to save_async for selection mode sharing
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.
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.
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 on attachment 332832 [details] [review] filterable, source: Add get_builtin Pushed to master.
Comment on attachment 332833 [details] [review] base-manager: Use photos_filterable_get_builtin Pushed to master.
Attaching new set of patches from here.
Created attachment 333787 [details] [review] utils: Add photos_utils_app_info_launch_uri
Created attachment 333788 [details] [review] base-item: Use photos_utils_app_info_launch_uri
Created attachment 333789 [details] [review] base-item: Split the code to encode a GeglBuffer and copy the metadata
Created attachment 333790 [details] [review] base-item: Add photos_base_item_save_to_stream_async
Created attachment 333791 [details] [review] base-item: Transparently load item in photos_base_item_save_async
Created attachment 333792 [details] [review] Add PhotosSharePoint
Created attachment 333793 [details] [review] utils: Add an extension point for PhotosSharePoint sub-classes
Created attachment 333794 [details] [review] Add PhotosSharePointOnline
Created attachment 333795 [details] [review] utils: Add an extension point for PhotosSharePointOnline sub-classes
Created attachment 333796 [details] [review] Add PhotosSharePointManager
Created attachment 333797 [details] [review] Add PhotosSharePointGoogle
Created attachment 333798 [details] [review] Add PhotosSharePointEmail
Created attachment 333799 [details] [review] Add PhotosShareDialog
Created attachment 333800 [details] [review] Add PhotosShareNotification
Created attachment 333801 [details] [review] application: Add share action
Created attachment 333802 [details] [review] application: Enable Share action
Created attachment 333803 [details] [review] selection-toolbar: Add Share button
Created attachment 333804 [details] [review] main-toolbar: Add Share button
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.
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.
Created attachment 333858 [details] [review] Add PhotosSharePoint
Created attachment 333859 [details] [review] Add PhotosSharePointManager
Created attachment 333862 [details] [review] Add PhotosSharePointGoogle
Created attachment 333863 [details] [review] Add PhotosSharePointEmail
Created attachment 333864 [details] [review] Add PhotosShareDialog
Created attachment 333865 [details] [review] Add PhotosShareNotification
Created attachment 333866 [details] [review] application: Add share action
Created attachment 333867 [details] [review] application: Add share action
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.