GNOME Bugzilla – Bug 733660
Let photos be deleted from the preview
Last modified: 2015-11-04 19:28:26 UTC
From bug 720222 - it should be possible to delete photos while previewing them. Hint: we need to be careful about the iters and paths used to navigate within the preview while we change the data in the model underneath.
s/photos/documents/g ?
Oops, yes, this should be against gnome-photos.
Created attachment 296686 [details] [review] added delete button
Review of attachment 296686 [details] [review]: Thanks for the patch. ::: src/photos-main-toolbar.c @@ +477,3 @@ + +static void +photos_main_toolbar_trash_button_clicked (PhotosMainToolbar *self) Since you have used g_signal_connect, the prototype or signature of this function should have been: void func (GtkButton *button, gpointer user_data); See: https://developer.gnome.org/gtk3/unstable/GtkButton.html#GtkButton-clicked However, if you want to PhotosMainToolbar * to be the first parameter of the callback, then you need to use g_signal_connect_swapped below. @@ +677,3 @@ + "clicked", + G_CALLBACK (photos_main_toolbar_trash_button_clicked), + self); If you use g_signal_connect_swapped here, then self will be the first parameter in your callback, otherwise it will be a GtkButton *, which in this case will be the same as priv->trash_button.
Created attachment 297293 [details] [review] changed g_signal_connect to g_signal_connect_swapped
Created attachment 298814 [details] [review] changed the comment line
Review of attachment 298814 [details] [review]: Ok, this one doesn't crash. The commit message needs some work. Most importantly it is missing the URL of this bug, and the subject doesn't represent what this patch is about. See https://wiki.gnome.org/Git/CommitMessages Some more comments ... ::: src/photos-main-toolbar.c @@ +483,3 @@ + item = PHOTOS_BASE_ITEM (photos_base_manager_get_active_object(self->priv->item_mngr)); + items = g_list_prepend (items, g_object_ref (item)); + photos_base_manager_remove_object (self->priv->item_mngr, G_OBJECT (item)); Removing an item from the manager also modifies the model, which can mess up the path/iter from underneath you. I think it might be easier to adjust the path/iter before touching the manager. @@ +485,3 @@ + photos_base_manager_remove_object (self->priv->item_mngr, G_OBJECT (item)); + photos_delete_notification_new (items); + gtk_tree_path_next (self->priv->current_path); You have not initialized priv->current_path anywhere, which causes: Gtk-CRITICAL **: gtk_tree_path_next: assertion 'path != NULL' failed @@ +669,3 @@ self, G_CONNECT_SWAPPED); + priv->trash_button=gtk_button_new_with_label (_("Delete")); Instead of using a button like this, try to move this to the menu (ie. the one with print, properties, etc..).
Created attachment 299713 [details] [review] added delete button in menu box
Created attachment 299714 [details] [review] removed delete label from preview
Review of attachment 299714 [details] [review]: Could you please combine everything into one patch? Try git commit --amend. As mentioned before, the commit message still needs work. eg., the bug URL is missing. See https://wiki.gnome.org/Git/CommitMessages
Created attachment 299753 [details] [review] added the delete button to the menu
Created attachment 299785 [details] [review] Added the delete button to the menu.
Review of attachment 299785 [details] [review]: Thanks for the new patch. Could you please mark the older versions as 'obsolete' when you attach a new patch? ::: src/photos-app-menu.ui @@ +20,3 @@ + <attribute name="label" translatable="yes">Delete</attribute> + <attribute name="accel"><Primary>p</attribute> + </item> This is the app menu, not "the one with print, properties, etc.". ::: src/photos-application.c @@ +73,3 @@ GSimpleAction *open_action; + GtkTreeModel *model; + PhotosBaseManager *item_mngr; item_mngr is already a part of PhotosApplication. See how it is used elsewhere in the file. @@ +369,3 @@ + GList *items = NULL; + + item = PHOTOS_BASE_ITEM (photos_base_manager_get_active_object(self->priv->item_mngr)); This will most likely [1] crash because priv->item_mngr has not been initialized and is NULL. See how item_mngr is used elsewhere in the file. [1] I say "most likely" because the code doesn't compile. ::: src/photos-main-toolbar.h @@ +87,2 @@ void photos_main_toolbar_set_stack (PhotosMainToolbar *self, GtkStack *stack); +void photos_main_toolbar_trash_button_clicked (PhotosMainToolbar *self) Lack of a trailing semi-colon prevents the code from compiling.
Created attachment 299867 [details] [review] added the delete button to the preview
Review of attachment 299867 [details] [review]: Please learn how to use git commit --amend and submit one coherent patch. Also see: https://wiki.gnome.org/GnomeLove/CodeContributionWorkflow#Follow_Up_on_the_Feedback
Created attachment 303117 [details] [review] add the delete button to the menu box in preview
Created attachment 303196 [details] [review] add the delete button to the menu box in preview
Created attachment 304766 [details] [review] added the delete button in the menu box of preview
Created attachment 304884 [details] [review] added the delete button to preview menu
Review of attachment 304884 [details] [review]: ::: src/photos-application.c @@ +820,3 @@ const gchar *gear_menu_accels[2] = {"F10", NULL}; const gchar *print_current_accels[2] = {"<Primary>p", NULL}; + const gchar *delete_current_accels[2] = {"<Primary>p", NULL}; This is wrong. You are having same shortcut key to do two actions.
Created attachment 305046 [details] [review] added the delete button in the menu box of preview. Made primary key "d" as shortcut key for deletion.
Review of attachment 305046 [details] [review]: Thanks for the new patch, Ankita. It compiles and doesn't crash, which is good. However, it has a few shortcomings: (a) There is no notification and it is not possible to undo the action. (b) The preview doesn't switch to the next available item. The combination of the above two also means that the user has no way of knowing if the item actually got deleted or not. Some of the earlier patches did attempt to do (b). Why did you remove that? :) For (a), see how PhotosDeleteNotification is used in photos-selection-toolbar.c
Created attachment 306158 [details] [review] added the delete button in preview menu added the undo button for delete
Created attachment 314094 [details] [review] Use GActions to represent the next and previous operations
Created attachment 314095 [details] [review] application, selection-toolbar: Use a GAction for delete
Created attachment 314096 [details] [review] preview-menu, preview-nav-buttons: Support deleting from the preview
Created attachment 314131 [details] [review] Use GActions to represent the next and previous operations
Created attachment 314132 [details] [review] application, selection-toolbar: Use a GAction for delete
Created attachment 314133 [details] [review] preview-nav-buttons: Rename the parameters for clarity
Created attachment 314134 [details] [review] preview-nav-buttons: Don't touch the path if there is no model
Created attachment 314135 [details] [review] preview-nav-buttons: Track the active item across changes to the model
Created attachment 314136 [details] [review] preview-menu, preview-nav-buttons: Support deleting from the preview
Created attachment 314852 [details] [review] enums: Remove redundant header