GNOME Bugzilla – Bug 743630
Add in app notification for move to trash action
Last modified: 2015-02-03 17:36:59 UTC
See patches. Not sure if I'm happy with the code dance of removing all notifications when the undo state changes. Either that, or make explicit calls from the application when we need to remoce all notifications. Or add support for undo "id's" so we can have multiple notifications at once, but I'm not sure how good would it be.
Created attachment 295629 [details] [review] general: Add libgd as a submodule We will use it for in app notifications.
Created attachment 295630 [details] [review] general: Add in app notification for move to trash We want to make sure the user has feedback when moving files to trash, so add an in app notification when some file has been moved to trash. Currently the notification manager only supports one notification at once, since the undo machinery only support to undo the last operation, not a specific operation. Even if we add support to it I'm not sure if multiple notifications of deleted files would be good, so instead of that just show one at once. To achieve that, connect to the undo state and delete all notification when the undo state changes. For that reason, we have to make sure that that's called before we add the new notification. So we set the undo action before the delete_callback, where the new notification is added.
Created attachment 295631 [details] [review] nautilus-view: Change move to trash accelerator Now that we have an in app notification, the user has a feedback if they push Delete key as an accident. So we no longer need to make the move to trash action difficult to do. So change the accelerator of move to trash from <shift>Delete to just Delete
Review of attachment 295630 [details] [review]: ::: libnautilus-private/nautilus-file-operations.c @@ +1907,3 @@ + * manager is removing all notifications when the undo state changes, since + * it only supports one notification at once. + */ I think a simpler way of fixing this that does not require changes here would be: - have whatever is in charge of creating the notification listen to the undo-changed signal from NautilusFileUndoManager - perhaps using g_signal_connect_after so that the notification manager code runs earlier - expose nautilus_file_undo_info_get_op_type() as a public API of NautilusFileUndoInfo - create the notification from the undo-changed callback if the operation type is NAUTILUS_FILE_UNDO_OP_MOVE_TO_TRASH - at the same time you can add an API to nautilus_file_undo_info_trash() to retrieve the list of files, and completely remove the logic from NautilusView ::: src/nautilus-notification-delete.c @@ +5,3 @@ + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or Nautilus is GPL v2+, so this would make the whole app GPL v3. Please change it to GPL v2 :) @@ +43,3 @@ +enum +{ + NOTIFICATION_TIMEOUT = 10 Should be a #define @@ +163,3 @@ + case PROP_FILE_NAME: + if (self->priv->file_name != NULL) + g_free (self->priv->file_name); You can remove this, since PROP_FILE_NAME is construct only. @@ +164,3 @@ + if (self->priv->file_name != NULL) + g_free (self->priv->file_name); + self->priv->file_name = g_strdup (g_value_get_string (value)); Can use g_value_dup_string() @@ +168,3 @@ + + case PROP_WINDOW: + self->priv->window = NAUTILUS_WINDOW (g_value_get_pointer (value)); Should use g_value_get_object() - and no cast is needed since it returns a gpointer @@ +177,3 @@ + +static void +nautilus_notification_delete_finalize (GObject *object) Should also remove the timeout if != 0 in here @@ +182,3 @@ + + if (self->priv->file_name != NULL) + g_free (self->priv->file_name); g_free() works fine on NULL ::: src/nautilus-notification-manager.c @@ +5,3 @@ + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or Same comment as NautilusNotificationDelete @@ +45,3 @@ + +static void +nautilus_notification_manager_remove (NautilusNotificationManager *self) Should probably be clearer to call this remove_all() @@ +101,3 @@ + */ + g_signal_connect_object (nautilus_file_undo_manager_get (), "undo-changed", + G_CALLBACK (nautilus_notification_manager_undo_manager_changed), self, 0); I think the "notification manager" object here should just be a container without special business logic or knowledge about undo. I like your idea of the undo notification being driven from an outside object, like NautilusWindow or NautilusApplication. ::: src/nautilus-view.c @@ +235,3 @@ + + guint notification_n_items; + gchar *notification_file_name; See the other comment; I believe this will not be needed if we change the architecture a bit @@ +3588,3 @@ + + if (view->details->notification_file_name != NULL) + g_free (view->details->notification_file_name); g_free() works fine on NULL ::: src/nautilus-window-slot.c @@ +600,3 @@ + slot->details->notification_manager = nautilus_notification_manager_new (); + gtk_overlay_add_overlay (GTK_OVERLAY (slot->details->view_overlay), + GTK_WIDGET (slot->details->notification_manager)); Conceptually I don't think this belongs in the overlay on top of the slot: undo operations are currently global. I think the message should not go away if you switch to another tab.
Review of attachment 295630 [details] [review]: ::: libnautilus-private/nautilus-file-operations.c @@ +1907,3 @@ + * manager is removing all notifications when the undo state changes, since + * it only supports one notification at once. + */ Sounds better ::: src/nautilus-notification-delete.c @@ +5,3 @@ + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or whops, that happens for using gnome-builder snippet for gpl @@ +43,3 @@ +enum +{ + NOTIFICATION_TIMEOUT = 10 Seems using enum is better because you can see the symbol while debugging. Blame rishi for it =P but ok, I guess is not that important to use enum.
Created attachment 295756 [details] [review] general: Add in app notification for move to trash We want to make sure the user has feedback when moving files to trash, so add an in app notification when some file has been moved to trash. Currently the notification manager only supports one notification at once, since the undo machinery only support to undo the last operation, not a specific operation. Even if we add support to it I'm not sure if multiple notifications of deleted files would be good, so instead of that just show one at once. To achieve that, connect to the undo state and delete all notification when the undo state changes and create the new notification if the undo operation is "move to trash" and if the undo manager state is "undo".
Review of attachment 295756 [details] [review]: Much better - still some comments inline. ::: src/nautilus-notification-delete.c @@ +83,3 @@ + NautilusNotificationDelete *self = NAUTILUS_NOTIFICATION_DELETE (object); + + nautilus_notification_delete_remove_timeout (self); Should chain up to parent here @@ +87,3 @@ + +static void +nautilus_notification_delete_constructed (GObject *object) You don't use the window in this function, so you can remove the constructed implementation and do it all in _init() @@ +102,3 @@ + G_OBJECT_CLASS (nautilus_notification_delete_parent_class)->constructed (object); + + undo_info = nautilus_file_undo_manager_get_action (); I would put a g_assert (NAUTILUS_IS_FILE_UNDO_INFO_TRASH (undo_info)) here. @@ +105,3 @@ + files = nautilus_file_undo_info_trash_get_files (NAUTILUS_FILE_UNDO_INFO_TRASH (undo_info)); + + if (g_list_length(files) == 1) Missing space @@ +107,3 @@ + if (g_list_length(files) == 1) + { + file_label = g_file_get_basename (files->data); For translation purposes, these two labels should belong in the same translated string; you could use an unicode space character if you need space out the two labels. I also think the 1 and > 1 cases should be separated into two separate strings because some languages could use completely different translations for numerals vs words. Finally, we should also use ngettext() in the > 1 case because some languages will use the singular case for > 1 numerals too... Something like this should work: length = g_list_length (files); if (length == 1) { file_label = g_file_get_basename (); /* Translators: %s is the basename of a file */ label = g_strdup_printf (_("%s deleted"), file_label); g_free (file_label); } else { label = g_strdup_printf (ngettext ("%d file deleted", "%d files deleted", length), length); } @@ +166,3 @@ + return g_object_new (NAUTILUS_TYPE_NOTIFICATION_DELETE, + "window", window, + "column-spacing", 0, Isn't it already 0 by default? ::: src/nautilus-notification-manager.c @@ +53,3 @@ + +static void +nautilus_notification_manager_class_init (NautilusNotificationManagerClass *class) Should be klass ::: src/nautilus-window-private.h @@ +56,3 @@ + /* Notifications */ + GtkWidget *overlay; This seems unused ::: src/nautilus-window.c @@ +1850,3 @@ + gtk_overlay_add_overlay (GTK_OVERLAY (window->details->main_view), + notebook); This doesn't look right. The notebook should be the base children of the overlay, and should be added with gtk_container_add()
Created attachment 295785 [details] [review] general: Add in app notification for move to trash We want to make sure the user has feedback when moving files to trash, so add an in app notification when some file has been moved to trash. Currently the notification manager only supports one notification at once, since the undo machinery only support to undo the last operation, not a specific operation. Even if we add support to it I'm not sure if multiple notifications of deleted files would be good, so instead of that just show one at once. To achieve that, connect to the undo state and delete all notification when the undo state changes and create the new notification if the undo operation is "move to trash" and if the undo manager state is "undo".
So, I wanted two labels because: - Markup bold only in the filename part. - The max_text_width_chars was mean to be applied only to the file name part. But couldn't figure it out how to achieve both with just one label, so I opt to make everything bold and apply max_width_chars to the entire label. I think it's okay anyway.
*** Bug 648658 has been marked as a duplicate of this bug. ***
Thanks Carlos; I pushed all these with a fix to only make the first part of the label bold too.