GNOME Bugzilla – Bug 745756
Make the delete notification strings consistent with gnome-documents and nautilus
Last modified: 2015-03-12 14:38:08 UTC
See bug 745658 The strings used by gnome-documents and nautilus for their in-app delete notifications are better. It makes sense to keep them consistent across applications. We were using: "Selected item has been deleted" "Selected items have been deleted" Instead, we should use: "“%s” deleted" "%d item deleted" "%d items deleted", where %s is the name of the item, if available, and %d is the count.
I would like to take up this bug.
Created attachment 298779 [details] Making delete notifications consistent with gnome-documents and nautilus. WIP: When single items are deleted, the notification string shows "1 item deleted", however when multiple items are deleted, the part of the delete notification string is not displayed as shown in the screenshot. I think there is error while casting the 'length' into character type. I am casting it by `label = gtk_label_new( (char)length + "items deleted" );`. Please let me know what you think should be done here.
Attaching a WIP patch would be helpful. And, label = gtk_label_new( (char)length + "items deleted" ); this is not how you should do the conversion.
Created attachment 298805 [details] Solution Screenshot: Make delete notifications consistent with gnome-documents and nautilus.
Created attachment 298806 [details] [review] Patch contributing to making the delete notifications consistent with gnome-documents and nautilus This patch maintains consistency between the delete notifications of gnome-documents and nautilus. Previously, the notifications in gnome-photos were: "Selected item has been deleted" "Selected items have been deleted" Gnome-documents and nautilus use: "“%s” deleted" "%d item deleted" "%d items deleted", where, %s is the name of the item, if available, and %d is the count. This has to be incorporated into gnome-photos. In gnome-photos, since names are not available (see Bug 733214 and Bug 734767), the notifications now, are as follows: "%d item deleted" "%d items deleted" Suggestions and thoughts welcome.
Review of attachment 298806 [details] [review]: Your patch is faulty. Please note that you have included files like aclocal.m4, output.0 ... Makefile.in.in Your commit should *only* include the files that you have made changes in. Please clean your patch and only include photos-delete-notification.c in your next patch. ::: src/photos-delete-notification.c @@ +141,3 @@ + // g_free(single); + // g_free(multiple); + // g_free(temp_var); If you are not using them, don't comment them out. Also note the spaces before the opening braces of the function call.
Created attachment 298810 [details] [review] Delete notification consistent with gnome-documents and nautilus Cleaned previous patch and made edits according to the coding standard.
Review of attachment 298810 [details] [review]: The commit message doesn't follow the guidelines. See: http://git-scm.com/book/ch5-2.html Basically: "As a general rule, your messages should start with a single line that’s no more than about 50 characters and 72 characters for the long description" ::: src/photos-delete-notification.c @@ +130,3 @@ guint length; + int temp_var; + char single[50], multiple[50] ; hmmm why 50? Sounds random and difficult to come with a good number for it.... look at g_strdup_printf, it allocates the required size for you! =) @@ +137,3 @@ + + temp_var = sprintf (single, "%d item deleted", length); + temp_var = sprintf (multiple, "%d items deleted", length); don't create a new variable like temp_var if you are not using it for something @@ +139,3 @@ + temp_var = sprintf (multiple, "%d items deleted", length); + label = gtk_label_new (ngettext(single, multiple, length)); + what we want here is actually: "“%s” deleted" "%d item deleted" "%d items deleted", where %s is the name of the item, if available, and %d is the count. So you still lack the handling when it's just one item and you want to put the name of the file instead of "%d item deleted". If maybe you didn't understand this part, the "%d item deleted" part, which is singular, is only needed for the ngettext () function which is used for translators, but it's actually not used at all in the end product.
Review of attachment 298810 [details] [review]: ::: src/photos-delete-notification.c @@ +130,3 @@ guint length; + int temp_var; + char single[50], multiple[50] ; Don't use basic C types. Use gint, gchar. @@ +137,3 @@ + + temp_var = sprintf (single, "%d item deleted", length); + temp_var = sprintf (multiple, "%d items deleted", length); * We use g_sprintf instead of sprintf. * If you are not using temp_var, there is no need to define and assign it. I am assuming now, that you are using gchar* instead of above char declaration. @@ +138,3 @@ + temp_var = sprintf (single, "%d item deleted", length); + temp_var = sprintf (multiple, "%d items deleted", length); + label = gtk_label_new (ngettext(single, multiple, length)); You are missing a space after ngettext again.
Review of attachment 298810 [details] [review]: ::: src/photos-delete-notification.c @@ +130,3 @@ guint length; + int temp_var; + char single[50], multiple[50] ; Also I forgot to say, this is a memory leak! You have to free the memory allocated by single and multiple. Use g_free () which does the same as the standard free () of c but also takes into account if the variable is NULL. Here is not necessary since we will always allocate memory for it, but we do it for convention and code style.
Review of attachment 298810 [details] [review]: ::: src/photos-delete-notification.c @@ +130,3 @@ guint length; + int temp_var; + char single[50], multiple[50] ; whops, my mistake. You are statically allocating them, so not need to free them. But when you change it to use g_strdup_printf you will need to free them. Sorry for the noise.
Created attachment 298881 [details] [review] Adding filename to delete notification Incorporating the above comments. The delete notifications now use: "“%s” deleted" "%d item deleted" "%d items deleted", where, %s is the name of the item and %d is the count.
Review of attachment 298881 [details] [review]: Your patch is not rebased to master, it is based on your earlier submitted patch. Please rebase to master and submit again.
Created attachment 298882 [details] Screenshot: Multiple Items Deleted Screenshot displaying "%d items deleted", when multiple items are deleted and where, "%d" is the number of items deleted.
Created attachment 298896 [details] [review] Delete Notifications consistent with gnome-documents Previous patch was incorrectly generated. Please ignore. The delete notifications now use: "“%s” deleted" "%d item deleted" "%d items deleted", where, %s is the name of the item and %d is the count.
Created attachment 298897 [details] Screenshot: Single Item Deleted Screenshot displaying "%s deleted", when single item is deleted and where, "%s" is the name of the file.
Review of attachment 298896 [details] [review]: ::: src/photos-delete-notification.c @@ +130,3 @@ guint length; + gint temp_var; + gchar single[50], multiple[50]; Don't use such static declarations as csoriano earlier pointed out. You can use gchar* here, allocate it dynamically below and then g_free() them at the end of the function. Observe this pattern in other functions. @@ +137,3 @@ length = g_list_length (priv->items); + PhotosBaseItem *item = PHOTOS_BASE_ITEM (l->data); + const gchar *name = photos_base_item_get_filename (item); All variable declaration should go in the start of the function. Please move the variable declarations above, and only assign them here. @@ +148,3 @@ + temp_var = sprintf (single, "%d item deleted", length); + temp_var = sprintf (multiple, "%d items deleted", length); + label = gtk_label_new (ngettext (single, multiple, length)); I don't think we need temp_var. If you are not using it, don't assign it. Moreover, don't use sprintf. As csoriano pointed out earlier, g_strdup_printf might be better choice.
Created attachment 298899 [details] [review] Delete Notifications consistent with gnome-documents Incorporating above comments.
Review of attachment 298899 [details] [review]: good work! Patch getting in shape now ! ::: src/photos-delete-notification.c @@ +125,3 @@ PhotosDeleteNotificationPrivate *priv = self->priv; + GList *l = priv->items; + PhotosBaseItem *item = PHOTOS_BASE_ITEM (l->data); Can't we just use priv->items->data ? This would eliminate one variable. Also, sort the variable declarations by variable names. @@ +126,3 @@ + GList *l = priv->items; + PhotosBaseItem *item = PHOTOS_BASE_ITEM (l->data); + const gchar *name = photos_base_item_get_filename (item); And by comments on previous patches, I meant moving only the variable declaration here, not the assignment. See this pattern in other functions, where variables are *declared* in the start. @@ +132,3 @@ GtkWidget *undo; guint length; + gchar *notification_label; This is getting leaked. g_free() it. @@ +140,3 @@ + if (length == 1) + { + notification_label = g_strdup_printf ("%s deleted", name); I am not sure about this, but I was wondering if we need to include the extension of the filename or not. We don't show the extension in header bar while in preview mode. @@ +145,3 @@ + { + notification_label = g_strdup_printf (ngettext ("%d item deleted", "%d items deleted", length), length); + } You don't need braces for single statement ifs. This code line is too long. Note how the earlier code broke this assignment into multiple lines. Lets break it too, makes it easier to read code.
Created attachment 298903 [details] [review] Delete Notifications consistent with gnome-documents Incorporating above comments. In reply to Comment 19: I will discuss stripping the extension in #gnome-design.
Review of attachment 298903 [details] [review]: Check for trailing spaces. Don't use tabs. Use only spaces. One more major thing : I think it would be better to wrap this functionality in a function. ::: src/photos-delete-notification.c @@ +140,3 @@ + notification_label = g_strdup_printf ("%s deleted", name); + else + notification_label = g_strdup_printf (ngettext ("%d item deleted", bad indentation. @@ +143,3 @@ + "%d items deleted", + length), + length); bad indentation
Review of attachment 298903 [details] [review]: ::: src/photos-delete-notification.c @@ +130,3 @@ GtkWidget *undo; guint length; + gchar *notification_label; Nitpick. Put it below the 'const gchar *name', and msg would be a better name because it would match gnome-documents. @@ +136,2 @@ length = g_list_length (priv->items); + name = photos_base_item_get_filename (PHOTOS_BASE_ITEM (priv->items->data)); We try not to reveal the raw file name anywhere in the UI. Please use photos_base_item_get_name_with_fallback @@ +138,3 @@ + + if (length == 1) + notification_label = g_strdup_printf ("%s deleted", name); The %s should be enclosed in proper Unicode quotes. See the gnome-documents code and this: https://wiki.gnome.org/Design/OS/Typography @@ +145,3 @@ + length); + label = gtk_label_new (notification_label); + g_free (notification_label); Nitpick. Put the g_free after the gtk_container_add
Created attachment 298989 [details] [review] Making delete notifications consistent with gnome-documents and nautilus. Added rishi's comments.
Review of attachment 298989 [details] [review]: Please put the URL to the bug in the commit message. See https://wiki.gnome.org/Git/CommitMessages ::: src/photos-delete-notification.c @@ +137,2 @@ length = g_list_length (priv->items); + name = photos_base_item_get_name_with_fallback (PHOTOS_BASE_ITEM (priv->items->data)); Trailing whitespace and extra white space before the '='. @@ +139,3 @@ + + if (length == 1) + msg = g_strdup_printf ("“%s” deleted", name); The string is not marked for translation. It should be inside _("...") @@ +144,3 @@ + "%d items deleted", + length), + length); Broken alignment.
Created attachment 299023 [details] [review] delete-notification: Improve the message I fixed the issues in the previous patch.
Created attachment 299028 [details] Screenshot: Single Item Deleted Screenshot: Single Item Deleted Notification Added quotes, "“%s” deleted".
String freeze exception granted. Committed.