GNOME Bugzilla – Bug 683229
implement new design for 'add printer' dialog
Last modified: 2013-02-15 17:42:33 UTC
This is the continuation of bug 678637, with the pieces that didn't get done there.
Created attachment 223219 [details] [review] asynchronous operations needed for redesigned new printer dialog
Created attachment 223220 [details] [review] redesign of the new printer dialog
Created attachment 223221 [details] [review] add support for samba printers
Created attachment 223222 [details] Samba printer installation workflow 1/4
Created attachment 223223 [details] Samba printer installation workflow 2/4
Created attachment 223224 [details] Samba printer installation workflow 3/4
Created attachment 223226 [details] Samba printer installation workflow 4/4
Created attachment 223290 [details] [review] Add PpCups object This patch add PpCups object which represents local CUPS server. It contains asynchronous method pp_cups_get_dests_async() which is an asynchronous version of cupsGetDests() and corresponding finish function.
Created attachment 223292 [details] [review] Add methods for getting print devices This patch adds PpHost object which represents a remote host from which we want to get printers. It contains pp_host_get_snmp_devices_async() function, pp_host_get_remote_cups_devices_async() function and corresponding finish functions. pp_host_get_snmp_devices_async() gets printers list from the specified host using snmp backend of CUPS. pp_host_get_remote_cups_devices_async() connects directly to the CUPS server of the remote host and get printers list from it. It also adds get_cups_devices_async() function which consequently executes CUPS callbacks and returns found devices by a callback.
Created attachment 223293 [details] [review] Add PpNewPrinter object for installation of new printer The PpNewPrinter object contains function pp_new_printer_add_async() which installs new printer. The new printer is specified by given parameters (e.g. name, device-id, device-uri, ppd-name, info, location, ...). The object also contains function function pp_new_printer_add_finish() which returns TRUE if the addition of new printer succeeded.
Created attachment 223295 [details] [review] Redesign of new printer dialog This is the last patch needed for replacement of patches https://bugzilla.gnome.org/attachment.cgi?id=223219 and https://bugzilla.gnome.org/attachment.cgi?id=223220. This patch and the three patches attached right before this one are modified version of previous patches for redesign of new printer dialog. These should agree with Bastien's requests. The PpNewPrinterDialog is a descendant of GObject now. It emits signal "pre-response" when dialog is closed and the printer is being added and signal "response" when the the dialog added new printer, failed to add new printer or was cancelled. Regards Marek
Review of attachment 223290 [details] [review]: ::: panels/printers/pp-utils.h @@ +24,3 @@ #include <gtk/gtk.h> +#include <cups/cups.h> +#include <cups/ppd.h> Why do we add the headers in pp-utils.h instead of pp-cups.[ch] directly?
Review of attachment 223292 [details] [review]: You don't seem to check anywhere whether the cancellable has actually been cancelled. The commit message is also uncomprehensible. You don't need to repeat the name of the functions you're adding, tell us what those functions do. And if the pp-utils.[ch] additions are stand-alone, make sure to commit them in a separate commit.
Review of attachment 223293 [details] [review]: Same comment about the commit log. Please split up your changes (especially the new objects/files) into more commits. ::: panels/printers/pp-utils.h @@ +293,3 @@ void pp_print_device_free (PpPrintDevice *device); +const gchar *get_paper_size_from_locale (void); This isn't something from this commit...
Review of attachment 223295 [details] [review]: > The PpNewPrinterDialog is a descendant of GObject. Stating the obvious here :) Please keep the commit messages informative, you don't need to explain the design of the printer dialogue for example, put a link to the design page. ::: panels/printers/pp-utils.h @@ -86,3 @@ const char *attr); -ipp_t *execute_maintenance_command (const char *printer_name, Remove that function in a separate cleanup commit please.
The patches looks good overall, and no UI changes should happen between this patchset and the release. Can you please request freeze break approval for this?
Hi, thank you for those reviews. I'm going to request the approval in a minute. I'll just prepare a screenshot to have a quick reference to this change. Marek
Created attachment 223347 [details] screenshot of new new printer dialog
(In reply to comment #12) > Review of attachment 223290 [details] [review]: > > ::: panels/printers/pp-utils.h > @@ +24,3 @@ > #include <gtk/gtk.h> > +#include <cups/cups.h> > +#include <cups/ppd.h> > > Why do we add the headers in pp-utils.h instead of pp-cups.[ch] directly? For cups.h, there are functions in pp-utils.h which needs types defined in cups.h. The ppd.h shouldn't be included there. I've created a separate commit for it. (In reply to comment #13) > Review of attachment 223292 [details] [review]: > > You don't seem to check anywhere whether the cancellable has actually been > cancelled. I think that I check it correctly. The get_cups_devices_async() checks the cancellable in several places and in the PpHost I set g_simple_async_result_set_check_cancellable() and check it by g_simple_async_result_propagate_error() later. > The commit message is also uncomprehensible. You don't need to repeat the name > of the functions you're adding, tell us what those functions do. Fixed. > And if the pp-utils.[ch] additions are stand-alone, make sure to commit them > in a separate commit. Done. (In reply to comment #14) > Review of attachment 223293 [details] [review]: > > Same comment about the commit log. Please split up your changes (especially > the new objects/files) into more commits. Done. > ::: panels/printers/pp-utils.h > @@ +293,3 @@ > void pp_print_device_free (PpPrintDevice *device); > > +const gchar *get_paper_size_from_locale (void); > > This isn't something from this commit... Done. (In reply to comment #15) > Review of attachment 223295 [details] [review]: > > > The PpNewPrinterDialog is a descendant of GObject. > > Stating the obvious here :) Removed. > Please keep the commit messages informative, you don't need to explain the > design of the printer dialogue for example, put a link to the design page. Tried :). > ::: panels/printers/pp-utils.h > @@ -86,3 @@ > const char *attr); > > -ipp_t *execute_maintenance_command (const char *printer_name, > > Remove that function in a separate cleanup commit please. Done. (In reply to comment #16) > The patches looks good overall, and no UI changes should happen between this > patchset and the release. Can you please request freeze break approval for > this? Thank you very much. I've requested the approval from release, i18n and docs teams. I've split those changes to 13 patches :).
Created attachment 223364 [details] [review] Include missing header The first patch.
Created attachment 223365 [details] [review] Move common constants to pp-utils.h
Created attachment 223366 [details] [review] Add PpCups object for getting destinations
Created attachment 223367 [details] [review] Add async method for getting print devices
Created attachment 223368 [details] [review] Add PpHost object for getting print devices
Created attachment 223369 [details] [review] Add PpMaintenanceCommand object for
Created attachment 223370 [details] [review] Make get_paper_size_from_locale() available to other source files
Created attachment 223371 [details] [review] Allow printer_get_ppd_async() to get PPD from remote host
Created attachment 223372 [details] [review] Add PpNewPrinter object for installation of new printer
Created attachment 223373 [details] [review] Return after callback
Created attachment 223374 [details] [review] Set longer timeouts
Created attachment 223375 [details] [review] Redesign of new printer dialog
Created attachment 223376 [details] [review] Remove redundant functions The last patch.
Review of attachment 223364 [details] [review]: +
Review of attachment 223365 [details] [review]: ++
Review of attachment 223366 [details] [review]: "destinations"? Other than that, looks good.
Review of attachment 223367 [details] [review]: "getting" print devices? Do you mean listing, enumerating, discovering?
Review of attachment 223368 [details] [review]: Again "getting".
Review of attachment 223369 [details] [review]: "PpMaintenanceCommand object represents CUPS' maintenance command." You already said that in the subject line.
Review of attachment 223370 [details] [review]: ++
Review of attachment 223371 [details] [review]: Don't repeat in the commit message information that's already in the subject line.
Review of attachment 223372 [details] [review]: ++
Review of attachment 223373 [details] [review]: ++
Review of attachment 223374 [details] [review]: ++
Review of attachment 223375 [details] [review]: Looks good (though you shouldn't start a sentence with "Also").
Review of attachment 223376 [details] [review]: ++
(In reply to comment #35) > Review of attachment 223366 [details] [review]: > > "destinations"? > > Other than that, looks good. CUPS uses "destinations" for elements of the array it returns by the cupsGetDests() function. I would like to keep it as it is. (In reply to comment #36) > Review of attachment 223367 [details] [review]: > > "getting" print devices? Do you mean listing, enumerating, discovering? I replaced this by listing. (In reply to comment #37) > Review of attachment 223368 [details] [review]: > > Again "getting". Replaced by listing. (In reply to comment #38) > Review of attachment 223369 [details] [review]: > > "PpMaintenanceCommand object represents CUPS' maintenance command." > You already said that in the subject line. I changed it a little. I usually try to be more specific in the main message than in the title. It means that I need to repeat part of the information sometimes. (In reply to comment #40) > Review of attachment 223371 [details] [review]: > > Don't repeat in the commit message information that's already in the subject > line. I changed this. (In reply to comment #44) > Review of attachment 223375 [details] [review]: > > Looks good (though you shouldn't start a sentence with "Also"). I removed the "Also". I'm waiting for approvals from i18n and docs teams right now. I'll commit the patches as soon as I'll have those approvals.
I've committed those patches to the master. I'm leaving this bug opened for the samba support yet.
Created attachment 235409 [details] samba installation workflow screenshot 1/6 Attached are new screenshots of workflow of addition of samba printer using Printers panel. There are 3 workflows, one for the case when password is needed to access the server to which the printer is attached. Second one for the case when password is needed just for the printer. And third one which doesn't need a password at all. First workflow is 1,2,3,4,6, the second one is 1,3,4,5,6 and the third one is 1,3,4,6. Maybe we could use the shell auth dialog for the first case as noted in https://bugzilla.gnome.org/show_bug.cgi?id=690494. It is not needed to enter an address in both cases, it finds the samba servers automatically (if they are visible). But you can enter an address for a specific server or a workgroup for a specific workgroup. I would need review from design point of view so I can finish the patch and send it for review. Regards Marek
Created attachment 235410 [details] samba installation workflow screenshot 2/6
Created attachment 235412 [details] samba installation workflow screenshot 3/6
Created attachment 235413 [details] samba installation workflow screenshot 4/6
Created attachment 235414 [details] samba installation workflow screenshot 5/6
Created attachment 235416 [details] samba installation workflow screenshot 6/6
Hi Marek, thanks for the screenshots - they're really useful. In general this is looking good. The dialogs are suffering from bug 690447 - fixing that will be an improvement. Apart from that, the main issue is the password dialogs. I won't go through the issues with those here, but will instead point you to the mockup we already have for shell modal dialogs here: http://bugzilla-attachments.gnome.org/attachment.cgi?id=231873 In the case where you need to authenticate in order to list samba printers, there should be an extra string to explain. Something like "Authentication needed to view remote printers."
Created attachment 235813 [details] mockups for password dialogs After some debate on IRC, it seems that using GTK for the password dialogs is the best option right now. Here's some design guidance for how the dialogs should look at behave.
Created attachment 236056 [details] password dialog This is the password dialog. There will be just one such dialog which will ask user for password when a samba server needs authentication to show its printers. We've agreed that it won't have the "Remember Password" checkbox because it would save the password in plaintext to "/etc/cups/printers.conf" which is not good. (The icon of the key is not just available on my local build of 3.8 libraries but it will be there when running it in full 3.8 session).
Created attachment 236094 [details] [review] add function for clearing of passwords Following patches implement searching for samba printers in new printer dialog.
Created attachment 236095 [details] [review] add authentication dialog for samba printers
Created attachment 236096 [details] [review] add class for searching for samba printers
Created attachment 236097 [details] [review] search for samba printers in new printer dialog
The first patch just implements a function for clearing password strings. The second one implements the authentication dialog used for listing printers on authenticated samba server. The third one is a class similar to PpHost and PpCups for searching for samba printers. The last patch uses previous 3 patches in new printer dialog.
Btw, we agreed with Allan on the design of the authentication dialog.
Review of attachment 236094 [details] [review]: ::: panels/printers/pp-utils.c @@ +3965,3 @@ + if (password && *password) + { + memset (*password, 0, strlen (*password)); Why the memset? Just the g_clear_pointer() call should be enough.
Review of attachment 236095 [details] [review]: ::: panels/printers/Makefile.am @@ +37,1 @@ pp-options-dialog.h \ You forgot to add the .ui file somewhere in Makefile.am, it would fail distcheck. ::: panels/printers/pp-authentication-dialog.c @@ +210,3 @@ + if (response_id == GTK_RESPONSE_OK) + { + username = g_strdup (gtk_entry_get_text ( Split those off in separate functions, the code for getting the password and the username are the exact same. @@ +244,3 @@ + + password_entry = (GtkWidget *) + gtk_builder_get_object (priv->builder, "password-entry"); Maybe a "WID()" macro as in some other panels would be useful. ::: panels/printers/pp-authentication-dialog.h @@ +56,3 @@ +PpAuthenticationDialog *pp_authentication_dialog_new (GtkWindow *parent, + const gchar *text, + const gchar *username); default_username, as it can be changed in the dialogue itself.
Review of attachment 236096 [details] [review]: ::: configure.ac @@ +148,3 @@ PKG_CHECK_MODULES(PRINTERS_PANEL, $COMMON_MODULES + polkit-gobject-1 >= $POLKIT_REQUIRED_VERSION + smbclient) Don't forget to send a mail to the release-team about this, and ensure that libsmbclient is in jhbuild. ::: panels/printers/pp-host.h @@ -38,3 @@ -typedef struct{ - GList *devices; -} PpDevicesList; You should move it in a separate patch if it's unused outside pp-utils.c
Review of attachment 236097 [details] [review]: ::: panels/printers/pp-new-printer-dialog.c @@ +317,3 @@ priv->remote_cups_searching = FALSE; priv->snmp_searching = FALSE; + priv->samba_host_searching = FALSE; Those shouldn't be necessary, the private section of GObjects is zeroed when instantiated. @@ +1653,3 @@ + priv->new_device->network_device); + +#ifdef GDK_WINDOWING_X11 We don't support running on non-X11 for now, remove. @@ +1755,2 @@ #ifdef GDK_WINDOWING_X11 + window_id = GDK_WINDOW_XID (gtk_widget_get_window (GTK_WIDGET (_dialog))); Ditto.
(In reply to comment #63) > Review of attachment 236094 [details] [review]: > > ::: panels/printers/pp-utils.c > @@ +3965,3 @@ > + if (password && *password) > + { > + memset (*password, 0, strlen (*password)); > > Why the memset? > Just the g_clear_pointer() call should be enough. The memset overwrites the password so nobody will be able to read it by dumping memory. This should improve security.
(In reply to comment #67) > The memset overwrites the password so nobody will be able to read it by dumping > memory. This should improve security. It wouldn't, really. If you're interested in enhancing security, please file a separate bug and talk to stefw about what to do here. I really doubt that running memset() is sufficient.
Created attachment 236269 [details] [review] authentication dialog for samba printers - modified (In reply to comment #64) > Review of attachment 236095 [details] [review]: > > ::: panels/printers/Makefile.am > @@ +37,1 @@ > pp-options-dialog.h \ > > You forgot to add the .ui file somewhere in Makefile.am, it would fail > distcheck. All ui files are listed in printers.gresource.xml. > ::: panels/printers/pp-authentication-dialog.c > @@ +210,3 @@ > + if (response_id == GTK_RESPONSE_OK) > + { > + username = g_strdup (gtk_entry_get_text ( > > Split those off in separate functions, the code for getting the password and > the username are the exact same. Done. > @@ +244,3 @@ > + > + password_entry = (GtkWidget *) > + gtk_builder_get_object (priv->builder, "password-entry"); > > Maybe a "WID()" macro as in some other panels would be useful. Done. > ::: panels/printers/pp-authentication-dialog.h > @@ +56,3 @@ > +PpAuthenticationDialog *pp_authentication_dialog_new (GtkWindow > *parent, > + const gchar *text, > + const gchar > *username); > > default_username, as it can be changed in the dialogue itself. Done.
(In reply to comment #68) > (In reply to comment #67) > > The memset overwrites the password so nobody will be able to read it by dumping > > memory. This should improve security. > > It wouldn't, really. If you're interested in enhancing security, please file a > separate bug and talk to stefw about what to do here. I really doubt that > running memset() is sufficient. ok, I'm going to remove the function and modify the rest of the patches to not include it.
Comment on attachment 236269 [details] [review] authentication dialog for samba printers - modified I have to remove using of the clear_password() yet.
Created attachment 236270 [details] [review] authentication dialog for samba printers - modified (In reply to comment #71) > (From update of attachment 236269 [details] [review]) > I have to remove using of the clear_password() yet. I've removed usage of the clear_password().
Created attachment 236273 [details] [review] class for searching for samba printers - modified (In reply to comment #65) > Review of attachment 236096 [details] [review]: > > ::: configure.ac > @@ +148,3 @@ > PKG_CHECK_MODULES(PRINTERS_PANEL, $COMMON_MODULES > + polkit-gobject-1 >= $POLKIT_REQUIRED_VERSION > + smbclient) > > Don't forget to send a mail to the release-team about this, and ensure that > libsmbclient is in jhbuild. I will handle this at Monday. > ::: panels/printers/pp-host.h > @@ -38,3 @@ > -typedef struct{ > - GList *devices; > -} PpDevicesList; > > You should move it in a separate patch if it's unused outside pp-utils.c It is used outside pp-utils.c. This is needed by PpSamba for return value of pp_samba_get_devices_finish(). The value is used in PpNewPrinterDialog. I removed the usage of clear_password().
Created attachment 236275 [details] [review] search for samba printers in new printer dialog - modififed (In reply to comment #66) > Review of attachment 236097 [details] [review]: > > ::: panels/printers/pp-new-printer-dialog.c > @@ +317,3 @@ > priv->remote_cups_searching = FALSE; > priv->snmp_searching = FALSE; > + priv->samba_host_searching = FALSE; > > Those shouldn't be necessary, the private section of GObjects is zeroed when > instantiated. I've removed them. > @@ +1653,3 @@ > + priv->new_device->network_device); > + > +#ifdef GDK_WINDOWING_X11 > > We don't support running on non-X11 for now, remove. > > @@ +1755,2 @@ > #ifdef GDK_WINDOWING_X11 > + window_id = GDK_WINDOW_XID (gtk_widget_get_window (GTK_WIDGET > (_dialog))); > > Ditto. Done.
I've committed those patches and hence I'm closing this bug. Thank you all for your help :).
Great news, Marek!