GNOME Bugzilla – Bug 693187
GNOME Printer Setup Tool: "-" button deletes printers without confirmation
Last modified: 2017-07-07 09:23:21 UTC
If I click the "-" button in the printer setup tool the selected printer gets immediately removed, without any confirmation pop-up. One can easily remove the wrong printer or easily click "-" instead of "+", so it happens easily that one accidentally removes a print queue. Also reported at Ubuntu: https://bugs.launchpad.net/ubuntu/+source/gnome-control-center/+bug/1115703
Created attachment 267618 [details] [review] Confirm removal of printer I was hit by this several times too. The attached patch adds the confirmation dialog. It is inspired by the users panel so I hope the wording is correct. It still needs ui review but I hope it won't change much.
Created attachment 267619 [details] screenshot of confirmation dialog for printer removal
Created attachment 267625 [details] updated screenshot The old screenshot was wrong (locally built gtk+-3.0).
Review of attachment 267618 [details] [review]: This needs updating to use a header bar, and the correct style for the delete button (should be in red).
Created attachment 295184 [details] Possible versions of Undo We've spoken about this with Allan. He thinks that the best solution of this would be to show an "Undo" button to the user for few seconds. Printer would be just hidden. It would be removed definitely after the timeout (we have to remove the removed printer notifications from g-s-d to not confuse users - see #664296). Attached are some proposal I've made. I prefer 2, 4 and 8. I should mention that gtk+ doesn't show the popover without the arrow, I had to hack it locally to not draw it. I would try to convince gtk+ developers to add a "show-arrow" property to the GtkPopover if that would be a preferred version. You can see that the GtkButton with label "Undo" is cropped on the left side when the OSD style is applied, I think that it is a bug in gtk+, so please don't evaluate the proposals based on this.
Hey Marek, thanks for looking at this. We have a number of undo implementations elsewhere in GNOME, which make use of a something called an "in app notification". In app notifications are used elsewhere in the control center, such as the region and language panel, when you change the language. It would be good to be consistent with those other implementations.
Created attachment 295430 [details] new screenshot (In reply to comment #6) > Hey Marek, thanks for looking at this. We have a number of undo implementations > elsewhere in GNOME, which make use of a something called an "in app > notification". In app notifications are used elsewhere in the control center, > such as the region and language panel, when you change the language. > > It would be good to be consistent with those other implementations. You are right, attached is a screenshot of a version which uses GdNotification. Do you agree with the wording there?
Created attachment 346375 [details] [review] printers: Allow undoing deletion of a printer Instead of directly applying the deletion of a printer, we should follow the GNOME in-app notification deletion guidelines. This patch introduces the in-app notification following the HIG[0] for the deletion of a printer. It allows to "undo" the deletion. The default behavior for these notification is to dismiss a previous notification. In doing so, when deleting multiple printers, the "Undo" button only restores the last deleted one. We don't do batch/ bulk removal in the printers panel. [0] https://developer.gnome.org/hig/stable/in-app-notifications.html.en
Created attachment 346376 [details] screenshot
Review of attachment 346375 [details] [review]: I took the liberty to re-implement the feature considering that the previous patches no longer apply clearly due to the big changes towards the new design. *Be aware that we are going through a UI freeze. ::: panels/printers/cc-printers-panel.c @@ +553,3 @@ + if (g_strcmp0 (printer.name, priv->deleted_printer_name) == 0) + return; Unfortunately this guard is necessary because the printer_delete () function is not async yet (which could cause the panel to be actualized before the printer gets fully deleted). ::: panels/printers/printers.ui @@ +47,3 @@ + <property name="visible">True</property> + <property name="wrap">True</property> + <object class="GtkBox"> These are exactly the same properties of notification widgets across control-center (UserAccounts, Region panels...)
Can we get this for 3.26?
Yes, we can. I'll look at it this week.
Review of attachment 346375 [details] [review]: Thank you for working on this. I see that we don't have async removal of the printer yet. Could you implement it? I would like to have it part of PpPrinter class (as pp_printer_delete_async() and pp_printer_delete_finish() methods). You can use the pp_printer_rename* methods as a boilerplate. And add a timeout for the Undo please. What about 10 seconds as in online accounts panel? ::: panels/printers/cc-printers-panel.c @@ +485,3 @@ + gtk_revealer_set_reveal_child (priv->notification, FALSE); + + priv->deleted_printer_name = NULL; You are leaking memory here. @@ +503,3 @@ + + g_free (priv->deleted_printer_name); + priv->deleted_printer_name = NULL; Use g_clear_pointer() here please. @@ +524,3 @@ + + notification_message = g_strdup_printf ("Printer \"%s\" has been deleted", + printer_name); Make this translatable and add comment for translators please. ::: panels/printers/pp-printer-entry.c @@ +74,3 @@ void (*printer_changed) (PpPrinterEntry *printer_entry); + void (*printer_deleted) (PpPrinterEntry *printer_entry, + gchar *printer_name); Lets commit this after the "printer-name" property is added so that we don't need the 2nd parameter. ::: panels/printers/printers.ui @@ +47,3 @@ + <property name="visible">True</property> + <property name="wrap">True</property> + <property name="max_width_chars">30</property> But it still could show more characters :). What about 50? But if this is something approved by designers then let it on 30. @@ +48,3 @@ + <property name="wrap">True</property> + <property name="max_width_chars">30</property> + <property name="label" translatable="yes">Printer deleted</property> Add comment for translators please. @@ +56,3 @@ + <property name="can_focus">True</property> + <property name="valign">GTK_ALIGN_CENTER</property> + <property name="label" translatable="yes">Undo</property> Add comment for translators please. @@ +107,3 @@ <property name="orientation">vertical</property> <property name="border-width">30</property> + <property name="margin-start">30</property> This is already part of different patch in different bug.
Created attachment 351547 [details] [review] printers: Remove printers asynchronously Introduce pp_printer_delete_async ()
Created attachment 351548 [details] [review] printers: Allow undoing deletion of a printer Instead of directly applying the deletion of a printer, we should follow the GNOME in-app notification deletion guidelines. This patch introduces the in-app notification following the HIG[0] for the deletion of a printer. It allows to "undo" the deletion. The default behavior for these notification is to dismiss a previous notification. In doing so, when deleting multiple printers, the "Undo" button only restores the last deleted one. We don't do batch/ bulk removal in the printers panel. [0] https://developer.gnome.org/hig/stable/in-app-notifications.html.en
Created attachment 351549 [details] [review] printers: Add 10s timeout for printer removal Dismisses the Printer removal notification after 10 seconds, removing the printer permanently. The 10 seconds value is taken from the online-accounts panel.
Review of attachment 351547 [details] [review]: This needs some tweaks yet. ::: panels/printers/pp-printer-entry.c @@ +571,3 @@ + printer = pp_printer_new (self->printer_name); + pp_printer_delete_async (printer, + self->remove_printer_cancellable, I don't think that we want to cancel the removal of printer, just pass NULL here. ::: panels/printers/pp-printer.c @@ +401,3 @@ + task = g_task_new (G_OBJECT (printer), cancellable, callback, user_data); + g_task_set_return_on_cancel (task, TRUE); + g_task_run_in_thread (task, printer_delete_thread); Don't do this in another thread, just call the DBus method with async call (and don't do a fallback like we do in rename, we don't need it for deleting of printer (the method is there since beginning)).
Review of attachment 351548 [details] [review]: ::: panels/printers/cc-printers-panel.c @@ +182,2 @@ static void +on_printer_removed_cb (GObject *source_object, Please select on_printer_removed() or printer_removed_cb() but not both. @@ +659,3 @@ + if (priv->remove_printer_cancellable != NULL) + { + g_cancellable_cancel (priv->remove_printer_cancellable); Don't cancel it, it is not needed (and hence the priv->remove_printer_cancellable too). @@ +691,3 @@ + on_notification_dismissed (NULL, self); + + notification_message = g_strdup_printf ("Printer \"%s\" has been deleted", What about the translations? @@ +699,3 @@ + g_free (notification_message); + + g_clear_pointer (&priv->deleted_printer_name, g_free); This is not needed, it was done in the on_notification_dismissed() call. @@ +722,3 @@ priv = PRINTERS_PANEL_PRIVATE (self); + if (g_strcmp0 (printer.name, priv->deleted_printer_name) == 0) Do this comparison in the for() cycle in actualize_printers_list_cb() to make it clear that we are not adding all printers. @@ +1176,3 @@ + widget = (GtkWidget*) + gtk_builder_get_object (priv->builder, "notification"); + priv->notification = GTK_REVEALER (widget); You can assign the value directly without the widget pointer. ::: panels/printers/pp-printer-entry.c @@ +83,3 @@ void (*printer_changed) (PpPrinterEntry *printer_entry); + void (*printer_deleted) (PpPrinterEntry *printer_entry, + gchar *printer_name); The printer name is redundant here. The signal should be named "printer-delete" because it was not deleted yet. @@ +541,1 @@ + gtk_widget_hide (GTK_WIDGET (self)); Do this in the handler of the printer-deleted signal please. ::: panels/printers/printers.ui @@ +61,3 @@ + <property name="visible">True</property> + <property name="wrap">True</property> + <property name="max_width_chars">30</property> So the 50 characters as I supposed are wrong? @@ +62,3 @@ + <property name="wrap">True</property> + <property name="max_width_chars">30</property> + <property name="label" translatable="yes">Printer deleted</property> What about the comment for translators? @@ +70,3 @@ + <property name="can_focus">True</property> + <property name="valign">GTK_ALIGN_CENTER</property> + <property name="label" translatable="yes">Undo</property> What about the comment for translators as I suggested in the previous review?
Review of attachment 351549 [details] [review]: ::: panels/printers/cc-printers-panel.c @@ +730,3 @@ gtk_revealer_set_reveal_child (priv->notification, TRUE); + + priv->remove_printer_timeout_id = g_timeout_add_seconds (10, on_remove_printer_timeout, self); Don't forget to remove the source in the dispose, otherwise it will crash if you quit printer panel to overview.
Created attachment 352353 [details] [review] printers: Remove printers asynchronously Introduce pp_printer_delete_async ()
Created attachment 352354 [details] [review] printers: Allow undoing deletion of a printer Instead of directly applying the deletion of a printer, we should follow the GNOME in-app notification deletion guidelines. This patch introduces the in-app notification following the HIG[0] for the deletion of a printer. It allows to "undo" the deletion. The default behavior for these notification is to dismiss a previous notification. In doing so, when deleting multiple printers, the "Undo" button only restores the last deleted one. We don't do batch/ bulk removal in the printers panel. [0] https://developer.gnome.org/hig/stable/in-app-notifications.html.en
Created attachment 352355 [details] [review] printers: Add 10s timeout for printer removal Dismisses the Printer removal notification after 10 seconds, removing the printer permanently. The 10 seconds value is taken from the online-accounts panel.
Review of attachment 352353 [details] [review]: Thank you for the modifications. There is just a minor thing we should solve yet. ::: panels/printers/pp-printer.c @@ +390,3 @@ + const gchar *ret_error; + + printer_name = g_task_get_task_data (task); The task data has not been set anywhere. You've probably wanted to call "g_object_get (g_task_get_source_object (task), "printer-name", &printer_name, NULL);", haven't you?
Review of attachment 352354 [details] [review]: This patch looks good. Just rename the enum name yet please. ::: panels/printers/pp-printer-entry.c @@ +95,3 @@ enum { IS_DEFAULT_PRINTER, + PRINTER_DELETED, Please change also this to "PRINTER_DELETE" before push.
Review of attachment 352355 [details] [review]: This patch looks good to me.
Created attachment 352633 [details] [review] printers: Remove printers asynchronously Introduce pp_printer_delete_async ()
Review of attachment 352633 [details] [review]: ::: panels/printers/pp-printer.c @@ +390,3 @@ + const gchar *ret_error; + + g_object_get (g_task_get_task_data (task), "printer-name", &printer_name, NULL); The task data has not been set anywhere. You've probably wanted to call "g_object_get (g_task_get_source_object (task), "printer-name", &printer_name, NULL);", haven't you?
Created attachment 352645 [details] [review] printers: Remove printers asynchronously Introduce pp_printer_delete_async ()
Review of attachment 352645 [details] [review]: Thank you for the patches. They look good to me now.
Thanks for your reviews! Attachment 352354 [details] pushed as f065f50 - printers: Allow undoing deletion of a printer Attachment 352355 [details] pushed as e5624f9 - printers: Add 10s timeout for printer removal Attachment 352645 [details] pushed as 5aca01c - printers: Remove printers asynchronously
*** Bug 784582 has been marked as a duplicate of this bug. ***