GNOME Bugzilla – Bug 777505
Notify when an item has been successfully shared
Last modified: 2017-10-22 09:42:30 UTC
Currently, we only show a notification if the sharing failed due to an error. I think we should also notify when sharing succeeded. We do put the application in a busy state while we are uploading something, but that seems a bit too subtle and easy to miss. Moreover, one might want to further modify the uploaded copy in a web browser. eg., change permissions, pass the link around, etc.. Therefore some positive feedback, possibly with a link to open it in a web browser would be nice.
This bug could be seen in two parts, -showing the "Share Successful" notification and -adding link to open shared picture in web browser in the notification itself. But when we share any picture, it immediately appears in photos and when we select that particular picture it already has option "Open With Google" below it. Then, why do we need to add link to the notification ? If I am unable to understand the requirement, please correct me.
(In reply to Ankriti Sachan from comment #1) > This bug could be seen in two parts, > -showing the "Share Successful" notification and > -adding link to open shared picture in web browser in the notification > itself. Well, the notification would be largely empty without the link, so ... :) > But when we share any picture, it immediately appears in photos and when we > select that particular picture it already has option "Open With Google" > below it. Then, why do we need to add link to the notification ? First, the user might not be in the overview when the sharing is complete. Or, might be scrolling or searching for something. So she might miss the fact that the item has appeared in the overview. Plus, it's slightly more convenient to be able to directly open it in a web browser from the notification.
Note that, as far as I can see, it is not possible to get the URL to an image on Google Photos. So, we might have to settle for something like https://photos.google.com/
(In reply to Debarshi Ray from comment #3) > Note that, as far as I can see, it is not possible to get the URL to an > image on Google Photos. So, we might have to settle for something like > https://photos.google.com/ Okay ! That made everything simpler ! Thank you ! :)
Created attachment 347531 [details] [review] Showing notification when an item is shared successfully along with a link to open it in web-browser
Review of attachment 347531 [details] [review]: Thanks for the patch, Ankriti. It is a good attempt. The commit message needs to be reformatted. See https://wiki.gnome.org/Newcomers/CodeContributionWorkflow Further comments below: ::: .buildconfig @@ +4,3 @@ +runtime=host +prefix=/home/ankriti/.cache/gnome-builder/install/Photos/host +default=true This file shouldn't be part of the patch. ::: src/photos-share-notification.c @@ +85,3 @@ + gchar *uri; + + uri = "https://photos.google.com"; Sadly, we can't hard-code the URL here because the notification is not specific to a particular SharePoint. eg., there is one for e-mail (src/photos-share-point-email.[ch]). We have to let the specific SharePoint sub-class define any such text or URL. eg., photos_share_point_get_name will get you the user-visible name ("Google" or "E-mail") of the SharePoint. @@ +123,3 @@ gtk_orientable_set_orientation (GTK_ORIENTABLE (self), GTK_ORIENTATION_HORIZONTAL); msg = photos_share_point_parse_error (self->share_point, self->error); photos_share_point_parse_error is not supposed to be called with a NULL self->error. @@ +127,3 @@ + if (msg == NULL) + { + msg = g_strdup("Photo shared successfully"); All user visible strings should be enclosed in _(...) so that they are marked for translation. For example see src/photos-export-notification.c. You'll have to "#include <glib/gi18n.h>". @@ +128,3 @@ + { + msg = g_strdup("Photo shared successfully"); + success = 1; It will be better to use "self->error == NULL" to separate the failure and success code paths instead of "success". @@ +140,3 @@ + GtkWidget *open; + + open = gtk_button_new_with_label ("Click to view in browser"); Maybe use "Open with Google" like we do elsewhere. eg., see photos_selection_toolbar_set_item_visibility and photos_main_toolbar_create_preview_menu. But again, we can't hard code "Google" here. @@ +205,3 @@ self->share_point = PHOTOS_SHARE_POINT (g_value_dup_object (value)); break; + Spurious whitespace change. @@ -209,3 @@ G_PARAM_CONSTRUCT_ONLY | G_PARAM_WRITABLE)); - - It's better to use a separate patch for stylistic fixes. Such as removing needless newlines, as you have done here. That way actual functional changes are kept separate from cosmetic ones. ::: src/photos-share-notification.h @@ +34,2 @@ void photos_share_notification_new_with_error (PhotosSharePoint *share_point, GError *error); +void photos_share_notification_new (PhotosSharePoint *share_point); photos_share_notification_new should go first. There should be a newline between the two declarations and the opening parentheses should be aligned. See some of the other headers.
I tried to correct most of the code section as you recommended me in review. But I am getting an error while running it. As soon as I click the link to open in browser, the link opens, but the photos application gets closed abruptly with "Aborted (core dumped)" message on terminal. I have pasted the modified code of notification_constructed and notification_open functions here: https://paste.gnome.org/pc12m5gxs I am not getting why is this happening. Please tell me what is the error in code ?
>> uri = (gchar*)"https://photos.google.com"; You should use g_strdup("https://photos.google.com") to return a gchar*. Currently the uri variable is of type (const gchar*) and you are freeing it in the last line. const ghar* are *not* supposed to be freed that way. :) I hope that resolves the issue.
(In reply to Umang Jain from comment #8) > >> uri = (gchar*)"https://photos.google.com"; > > You should use g_strdup("https://photos.google.com") to return a gchar*. > Currently the uri variable is of type (const gchar*) and you are freeing it > in the last line. const ghar* are *not* supposed to be freed that way. :) > > I hope that resolves the issue. Yes. That did work. Thank you so much.
Created attachment 347724 [details] [review] Notifying when an item has been shared successfully. Earlier, a notification was shown only if sharing of an item failed due to an error. No notification was shown on successful sharing. In this patch, code segment has been added to show notification on successful sharing of an item. The notification also provides a link to open it in a web-browser.
Review of attachment 347531 [details] [review]: @Ankriti Sachan Please mark the previous patch as "obsolete" if you are attaching a new version of the patch. Marking it obsolete now.
(In reply to Umang Jain from comment #11) > Review of attachment 347531 [details] [review] [review]: > > @Ankriti Sachan Please mark the previous patch as "obsolete" if you are > attaching a new version of the patch. Marking it obsolete now. Oh ! Okay ! I was not knowing that.
Comment on attachment 347724 [details] [review] Notifying when an item has been shared successfully. Thanks for the patch Ankriti. I have some overall comments here. First of all, make the commit message still shorter. May be: share-point-notification: Notifying when an item has been shared successfully There is no need of verbose explanation here as it is quite evident by just reading the patch :) > guint timeout_id; >+ const gchar *app_name; > }; We generally keep the names sorted alphabetically. See other class structs varibles to get an idea. > > enum >@@ -84,14 +86,15 @@ photos_share_notification_open (PhotosShareNotification *self) photos_share_notification_open_link provides a more readable name, no? > GError *error; > gchar *uri; > >- uri = "https://photos.google.com"; >+ if (g_strcmp0 (self->app_name, "Google" ) == 0) >+ uri = g_strdup(_("https://photos.google.com")); > Again we _cannot_ have "Google" hard-coded here. PhotosShareNotification will need to interact with other sharepoints (like "Facebook") in future. So, there it will break it. I can imagine a vfunc "photos_share_point_get_base_url" implemented by every other share point to return their base URL (e.g. https://photos.google.com). So, the hard-coding is removed by calling : uri = photos_share_point_get_base_url(self->share_point); //share-point may be Google, Facebook etc.. Plus, we don't need Translation (_) here I guess.. >+ if(g_strcmp0 (self->app_name, "E-Mail" ) != 0) Again, no hard coding here "Email". Refer to the discussion we had in #photos where rishi suggested to have a gboolean photos_share_point_needs_notification. So this boils down to: if (photos_share_point_needs_notification) { //logic } >+ if ((self->error == NULL) && (g_strcmp0 (self->app_name, "E-Mail" ) != 0)) Ditto.
Created attachment 347860 [details] [review] share-point-notification: Notifying when an item has been shared successfully
Created attachment 347874 [details] [review] share-point-notification: Notifying when an item has been shared successfully
(In reply to Ankriti Sachan from comment #7) > I tried to correct most of the code section as you recommended me in review. > But I am getting an error while running it. As soon as I click the link to > open in browser, the link opens, but the photos application gets closed > abruptly with "Aborted (core dumped)" message on terminal. Try to use GDB: https://www.cs.cmu.edu/~gilpin/tutorial/
Review of attachment 347874 [details] [review]: ::: src/photos-share-notification.c @@ +130,1 @@ + if(photos_share_point_needs_notification (self->share_point)) Nitpick: missing whitespace before opening parenthesis. @@ +141,3 @@ + gtk_container_add (GTK_CONTAINER (self), label); + + if ((self->error == NULL) && (photos_share_point_needs_notification (self->share_point))) We have already checked whether "needs notification" is TRUE. There is no need to do it again. ::: src/photos-share-point-google.c @@ +74,3 @@ + +static gboolean * +photos_share_point_email_needs_notification (PhotosSharePoint *share_point) Nitpick: it should be photos_share_point_google_needs_notification, not *_email_*.
(In reply to Debarshi Ray from comment #16) > (In reply to Ankriti Sachan from comment #7) > > I tried to correct most of the code section as you recommended me in review. > > But I am getting an error while running it. As soon as I click the link to > > open in browser, the link opens, but the photos application gets closed > > abruptly with "Aborted (core dumped)" message on terminal. > > Try to use GDB: > https://www.cs.cmu.edu/~gilpin/tutorial/ Okay ! Thank you.
Created attachment 347927 [details] [review] share-point-notification: Notifying when an item has been shared successfully
Created attachment 348471 [details] Screenshot Screenshot of the current patch.
Review of attachment 347927 [details] [review]: Thanks for the new patch, Ankriti! It is getting better. Some comments below: ::: src/photos-share-notification.c @@ +33,3 @@ { GtkGrid parent_instance; + const gchar *app_name; Nitpick: would be nice to retain the alphabetical ordering. Could you move it further below? Just above the guint. @@ +88,3 @@ + gchar *uri; + + uri = g_strdup(photos_share_point_get_base_url(self->share_point)); There is no need to g_strdup the base URL. We can directly use the 'const gchar *' that was returned. @@ +130,1 @@ + if (photos_share_point_needs_notification (self->share_point)) We won't see a notification if sharing via e-mail failed due to some reason. If you read src/photos-share-point-email.c then you'll see that it can fail under certain circumstances. We always want to see the notification if we have a GError. @@ +130,2 @@ + if (photos_share_point_needs_notification (self->share_point)) + { Nitpick: the alignment is off. We use 2 spaces to indent the brace and then another 2 for the statements. See the rest of the code for examples. @@ +135,2 @@ + else + msg = g_strdup(_("Photo shared successfully")); It might be nice to be consistent with the other notifications. They use strings in this format: * "“%s” edited" (src/photos-done-notification.c) * "“%s” exported" (src/photos-export-notification.c) The %s is the string returned by photos_base_item_get_name_with_fallback. Have you discussed this with the GNOME designers so far? @@ +147,1 @@ + open = gtk_button_new_with_label (_(g_strconcat("Open with ", self->app_name, NULL))); The string returned by g_strconcat should be g_free'd. Therefore, we need to use a separate statement for it so that we can hold the string in a separate variable. ::: src/photos-share-point-google.c @@ +69,3 @@ +photos_share_point_google_get_base_url (PhotosSharePoint *share_point) +{ + return _("https://photos.google.com"); This URI shouldn't be marked for translation with _(...) because the URI won't serve its purpose if it is replaced by any other string. Only those strings that are actually visible to the user should be enclosed in _(...). ::: src/photos-share-point.h @@ +46,3 @@ + GIcon *(*get_icon) (PhotosSharePoint *self); + const gchar *(*get_name) (PhotosSharePoint *self); + gboolean (*needs_notification) (PhotosSharePoint *self); It is good that you also realigned the other rows, but it would be nicer if you could do that in a separate subsequent patch. Otherwise it makes the diff needlessly longer and harder to spot the actual change.
> > @@ +135,2 @@ > + else > + msg = g_strdup(_("Photo shared successfully")); > > It might be nice to be consistent with the other notifications. They use > strings in this format: > * "“%s” edited" (src/photos-done-notification.c) > * "“%s” exported" (src/photos-export-notification.c) > > The %s is the string returned by photos_base_item_get_name_with_fallback. > Have you discussed this with the GNOME designers so far? Yes. I did see why and how '%s' is used. But I am not really sure, here, about what I should be discussing with GNOME designers? Please explain a bit more. I am unable to understand the designing related part here.
(In reply to Ankriti Sachan from comment #22) > Yes. I did see why and how '%s' is used. But I am not really sure, here, > about what I should be discussing with GNOME designers? Please explain a bit > more. I am unable to understand the designing related part here. We are adding new UI in this bug. So, it would be good to run the final outcome of the change past the designers. That's why it is also useful to attach a screenshot so that one can easily discuss the UI.
Created attachment 349252 [details] [review] share-point-notification: Notifying when an item has been shared successfully
Created attachment 349254 [details] Screenshot
Comment on attachment 349252 [details] [review] share-point-notification: Notifying when an item has been shared successfully Hi, the patch looks in good shape to me other than few comments below: typedef struct _PhotosApplicationCreateData PhotosApplicationCreateData; +typedef struct _PhotosApplicationCreateDataSecond PhotosApplicationCreateDataSecond; typedef struct _PhotosApplicationRefreshData PhotosApplicationRefreshData; May be more meaningful name? PhotosApplicationNotifyData ? @@ -1362,10 +1391,15 @@ photos_application_set_bg_common (PhotosApplication *self, GVariant *parameter, static void photos_application_share_share (GObject *source_object, GAsyncResult *res, gpointer user_data) { - PhotosApplication *self = PHOTOS_APPLICATION (user_data); + PhotosApplicationCreateDataSecond *data = (PhotosApplicationCreateDataSecond *) user_data; + PhotosApplication *self = data->application; PhotosSharePoint *share_point = PHOTOS_SHARE_POINT (source_object); + PhotosBaseItem *item; GError *error = NULL; + item = data->item; + g_return_if_fail (item != NULL); + Directly assign the data->item while declaring ( as you did for data->application ) @@ -33,7 +35,9 @@ struct _PhotosShareNotification GtkGrid parent_instance; GError *error; GtkWidget *ntfctn_mngr; + PhotosBaseItem *item; PhotosSharePoint *share_point; + const gchar *app_name; guint timeout_id; }; I wonder if we need app_name as a member as it can be easily retrieved by photos_share_point_get_name and passing share_point (which is already a member) + if (self->error == NULL && photos_share_point_needs_notification(self->share_point)) + { + GtkWidget *open; + gchar *app_label; + + self->app_name = photos_share_point_get_name (self->share_point); + app_label = g_strdup_printf (_("Open with “%s”"), self->app_name); Strip "" around %s. We want | Open with Google | and not |Open with "Google"| + + if(msg != NULL) + { image = gtk_image_new_from_icon_name (PHOTOS_ICON_WINDOW_CLOSE_SYMBOLIC, GTK_ICON_SIZE_INVALID); gtk_widget_set_margin_bottom (image, 2); gtk_widget_set_margin_top (image, 2); Wrong formatting of the "if" block. @@ -128,6 +181,7 @@ photos_share_notification_constructed (GObject *object) self->timeout_id = g_timeout_add_seconds (SHARE_TIMEOUT, photos_share_notification_timeout, self); g_free (msg); + } Ditto. } @@ -201,6 +264,12 @@ photos_share_notification_class_init (PhotosShareNotificationClass *class) G_TYPE_ERROR, G_PARAM_CONSTRUCT_ONLY | G_PARAM_WRITABLE)); g_object_class_install_property (object_class, + PROP_ITEM, + g_param_spec_pointer ("item", + "PhotosBaseItem instance", + "The edited PhotosBaseItem", + G_PARAM_CONSTRUCT_ONLY | G_PARAM_WRITABLE)); + g_object_class_install_property (object_class, Inappropriate description. It should be "PhotosBaseItem that is shared/uploaded" ... +static gboolean * +photos_share_point_email_needs_notification (PhotosSharePoint *share_point) +{ + return FALSE; +} + You are returning a gboolean pointer. Return only a gboolean (variable) not a pointer +static gboolean * +photos_share_point_google_needs_notification (PhotosSharePoint *share_point) +{ + return TRUE; +} Ditto.
Created attachment 349310 [details] [review] share-point-notification: Notifying when an item has been shared successfully
Created attachment 349311 [details] Screenshot
A passing thought. While this UI looks nice to me for Google, something like an Imgur share point should show the shared URL more prominently because the primary purpose there would be to pass the URL around. I wonder if we want to enforce consistency among the sharing notifications. Or not.
(In reply to Debarshi Ray from comment #29) > A passing thought. > > While this UI looks nice to me for Google, something like an Imgur share > point should show the shared URL more prominently because the primary > purpose there would be to pass the URL around. I wonder if we want to > enforce consistency among the sharing notifications. Or not. Also, if we can automatically, copy the URL to the clipboard, so it's reading for pasting. That might be good too (I think).
> Also, if we can automatically, copy the URL to the clipboard, so it's > reading for pasting. That might be good too (I think). s/reading/ready/
(In reply to Umang Jain from comment #30) > (In reply to Debarshi Ray from comment #29) > > A passing thought. > > > > While this UI looks nice to me for Google, something like an Imgur share > > point should show the shared URL more prominently because the primary > > purpose there would be to pass the URL around. I wonder if we want to > > enforce consistency among the sharing notifications. Or not. > > Also, if we can automatically, copy the URL to the clipboard, so it's > reading for pasting. That might be good too (I think). Yeah. Although, we must be careful not to surprise the user. It can be an unwelcome surprise if we overwrite the contents of the clipboard without the user's permission.
Good job, looks good to me!
(In reply to Debarshi Ray from comment #29) > A passing thought. > > While this UI looks nice to me for Google, something like an Imgur share > point should show the shared URL more prominently because the primary > purpose there would be to pass the URL around. I wonder if we want to > enforce consistency among the sharing notifications. Or not. I would definitely require an explicit action to copy anything to the clipboard. So I don't like the idea of providing a button that takes you to a website while copying something to the paste buffer in the background. But you're right that Google isn't a particularly good use case as it isn't a straight sharing platform. The blame comes on my shoulders as the wiki clearly lacks the common use cases. * To share photos using Facebook would mean uploading the photo set and take you to the second stage of writing a little summary and sharing it on your timeline, group or specific people. The action button in the notification would then read something along "Your photos have been uploaded. [Share using Facebook]" Which would hopefully take you to Facebook with the photos prepopulated to the entry. * To share photos using twitter, might mean to upload it to an image service and create a new tweet pointing to it. So again, the notification would read "Photos uploaded. [Tweet]" * Sharing to Flickr might mimic what we do for Google.
Created attachment 361823 [details] [review] share-point: Re-align Split out the alignment changes into a separate commit.
Created attachment 361824 [details] [review] share-point-notification: Notifying when an item has been shared successfully
Review of attachment 361824 [details] [review]: ::: src/photos-application.c @@ +262,3 @@ + data = g_slice_new0 (PhotosApplicationShareNotifyData); + g_application_hold (G_APPLICATION (application)); + data->application = application; On second thoughts, we can avoid this structure and an memory (de-)allocation cycle by using g_application_get_default. ::: src/photos-share-notification.c @@ +95,3 @@ + + error = NULL; + if (!g_app_info_launch_default_for_uri (uri, NULL, &error)) We should use gtk_show_uri_on_window these days. Otherwise portals cannot parent their dialogs when running sandboxed. See commit d3c00fa78272e7da for examples. @@ +131,3 @@ + if(self->error != NULL) + msg = photos_share_point_parse_error (self->share_point, self->error); + Nitpick: use braces instead of a newline. @@ +132,3 @@ + msg = photos_share_point_parse_error (self->share_point, self->error); + + else if (photos_share_point_needs_notification(self->share_point)) I'd push the responsibility of checking needs_notification on the code that creates the ShareNotification. That will simplify a few things. @@ +137,3 @@ + + name = photos_base_item_get_name_with_fallback (PHOTOS_BASE_ITEM (self->item)); + msg = g_strdup_printf (_("“%s” shared successfully"), name); po/POTFILES.in needs to be updated. @@ +160,3 @@ + } + + if(msg != NULL) Pushing needs_notification on the caller's shoulder will make this condition unnecessary. @@ +177,1 @@ + photos_notification_manager_add_notification (PHOTOS_NOTIFICATION_MANAGER (self->ntfctn_mngr), GTK_WIDGET (self)); Most notifications in gnome-photos, not all, are self-managing. It means that once added to the NotificationManager they are resposible for destroying themselves. An unfortunate side-effect of this conditional branch is that the ShareNotification will be leaked when sharing via email. This is the biggest advantage of pushing the responsibility of checking needs_notification on the caller. @@ +223,3 @@ + { + PhotosBaseItem *item = (PhotosBaseItem *) g_value_get_pointer (value); + self->item = g_object_ref (item); Using g_param_spec_object will let us use g_value_dup_object. See below. @@ +264,3 @@ + g_object_class_install_property (object_class, + PROP_ITEM, + g_param_spec_pointer ("item", Use g_param_spec_object, not g_param_spec_pointer. See the property below. ::: src/photos-share-point.h @@ +44,2 @@ /* virtual methods */ + const gchar *(*get_base_url) (PhotosSharePoint *self); I think a better option is to return the URI directly from the share_finish method when the operation is complete. One can imagine cases (eg., an Imgur share-point) where the URL won't be constant for all shares done by the same share-point. The URI is inherently coupled with a particular operation. @@ +46,3 @@ GIcon *(*get_icon) (PhotosSharePoint *self); const gchar *(*get_name) (PhotosSharePoint *self); + gboolean *(*needs_notification) (PhotosSharePoint *self); As Umang pointed out in comment 26, it should return a gboolean, not a pointer to a gboolean. @@ +62,3 @@ const gchar *photos_share_point_get_name (PhotosSharePoint *self); +gboolean *photos_share_point_needs_notification (PhotosSharePoint *self); Ditto. gboolean, not gboolean *.
Created attachment 362032 [details] [review] Extend the SharePoint API to enable notifications on success Split out the SharePoint part of the patch.
Created attachment 362034 [details] [review] Show a notification when an item has been successfully shared Fixed everything up and pushed.
Review of attachment 362032 [details] [review]: Eek, I messed up the authorship of the patch. :(
Thanks for your patches, Ankriti, and everybody else who chipped in with help.