GNOME Bugzilla – Bug 760783
Redesign the Add New Printer dialog
Last modified: 2017-02-07 09:22:29 UTC
Created attachment 319268 [details] screencast of "Add New Printer" dialog I have updated the "Add New Printer" dialog in order to match the new designs at https://wiki.gnome.org/Design/SystemSettings/Printers I have attached a screencast of it. * Currently it does not embed the Authenticate (Unlock Print Server) view within the dialog, it still pops up an authentication dialog. As soon as I setup a test environment for a password protected printing server, I will patch this component properly.
Created attachment 319269 [details] [review] printers: redesign the Add New Printer dialog Update the Add New Printer dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
Review of attachment 319269 [details] [review]: Thank you for the redesign implementation. I think that the spinner is too small and maybe we would need a symbolic version of it to match the printer-symbolic used when there are no printers found. Making the spinner insensitive does part of this for me. Also the Add button is much wider than the Cancel button, could you make them equally wide? I see that the design has Unlock instead of Authenticate, it would allow us to have the same width of the Cancel and Unlock button probably. Do you plan to change it? (I will test the authentication dialog tomorrow but I suppose that it will work as before) I was able to close the dialog by pressing Esc before. It is not possible with your patch. The design should cover the situation when we already have some printers found and we are still searching for other ones like when you have a local USB printer and you enter a server name. In this situation we are still searching but nothing indicates that. From the mock-ups it seems that the icons of found printers are centered between edge of the dialog and the description but they are not when I test the patch. There is a visible edge (1px) around the space in which the SearchEntry is embedded which doubles the edge in this part. Would it be possible to keep just the upper edge? It would correspond with the HeaderBar better and mock-up. ::: panels/printers/new-printer-dialog.ui @@ +25,3 @@ <object class="GtkDialog" id="dialog"> + <property name="width_request">450</property> + <property name="height_request">450</property> I see that you are requesting 450x450 but size on my machine is ~400x400 (but I test it on 3.18 so maybe it is a local problem). @@ +29,2 @@ <property name="can_focus">False</property> + <property name="title" translatable="yes">Add Printer</property> Add a comment for translators here please. @@ +55,3 @@ <child> + <object class="GtkButton" id="new-printer-add-button"> + <property name="label" translatable="yes">_Add</property> Add a comment for translators here please. ::: panels/printers/pp-new-printer-dialog.c @@ +730,3 @@ gtk_spinner_start (GTK_SPINNER (spinner)); + gtk_stack_set_visible_child_name (GTK_STACK (stack), "loading-page"); + gtk_header_bar_set_subtitle (GTK_HEADER_BAR (header_bar), _("Searching for Printers")); Add a comment for translators here please. @@ +1702,3 @@ if (pp_print_device_get_device_location (device) != NULL && pp_print_device_get_device_location (device)[0] != '\0') { /* Translators: Location of found network printer (e.g. Kitchen, Reception) */ You can remove this comment now. @@ +1707,3 @@ else if (pp_print_device_get_host_name (device) != NULL && pp_print_device_get_host_name (device)[0] != '\0') { /* Translators: Network address of found printer */ You can remove this comment now.
(In reply to Marek Kašík from comment #2) > Review of attachment 319269 [details] [review] [review]: > I see that the design has Unlock instead of Authenticate, it would allow us > to have the same width of the Cancel and Unlock button probably. Do you plan > to change it? (I will test the authentication dialog tomorrow but I suppose > that it will work as before) I've just tried to add printers from an authenticated samba server. There are several problems. The first one is that once an user presses authenticate he waits and the authentication dialog shows up with some delay. I think that this can be solved by the redesign. The second one is that when user authenticate such a server only the printers from the server are shown (which is probably related to the fact that address of the printer is entered in to the entry). For testing this you basically need to setup 3 samba servers. One which needs authentication and has the printers installed. Another one which you setup as a master and don't need authentication so it is able to tell others that there is an authenticated one. And the local one on your machine so that it is in the configured workgroup. For the authenticated server I have "valid users = mkasik" (right after the === Share Definitions ===) in my /etc/samba/smb.conf which ensures that the whole share will need authentication. For the master I've setup "domain master = yes". And for the local machine I think that I left everything on default. Just be sure that all the configs has the same workgroup (there should be MYGROUP by default). It is also good to set "netbios name" for each of the machines. After setting of this you need to restart services smb and nmb and wait some time before it updates all information. For addition of a user to samba I use system-config-samba (on the authenticated share only). You can test the setting by running "smbtree -N" which browses local network and should show you available shares (it sometimes shows them, sometimes not). You can try to list shares offered by the authenticated machine by running "smbclient -L IP-ADDRESS". Regards Marek
Thanks for your review. (In reply to Marek Kašík from comment #2) > Review of attachment 319269 [details] [review] [review]: > > Thank you for the redesign implementation. > > I think that the spinner is too small and maybe we would need a symbolic > version of it to match the printer-symbolic used when there are no printers > found. Making the spinner insensitive does part of this for me. > > Also the Add button is much wider than the Cancel button, could you make > them equally wide? > > I see that the design has Unlock instead of Authenticate, it would allow us > to have the same width of the Cancel and Unlock button probably. Do you plan > to change it? (I will test the authentication dialog tomorrow but I suppose > that it will work as before) > > I was able to close the dialog by pressing Esc before. It is not possible > with your patch. Sure. I will update these on my next patch. > The design should cover the situation when we already have some printers > found and we are still searching for other ones like when you have a local > USB printer and you enter a server name. In this situation we are still > searching but nothing indicates that. According to the mockups at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines "Spinner is displayed while search is taking place and nothing has been found". And "Ongoing search is indicated by the sub-heading". These are notes at the bottom of each dialog mockup. > From the mock-ups it seems that the icons of found printers are centered > between edge of the dialog and the description but they are not when I test > the patch. > > There is a visible edge (1px) around the space in which the SearchEntry is > embedded which doubles the edge in this part. Would it be possible to keep > just the upper edge? It would correspond with the HeaderBar better and > mock-up. > > ::: panels/printers/new-printer-dialog.ui > @@ +25,3 @@ > <object class="GtkDialog" id="dialog"> > + <property name="width_request">450</property> > + <property name="height_request">450</property> > > I see that you are requesting 450x450 but size on my machine is ~400x400 > (but I test it on 3.18 so maybe it is a local problem). > > @@ +29,2 @@ > <property name="can_focus">False</property> > + <property name="title" translatable="yes">Add Printer</property> > > Add a comment for translators here please. > > @@ +55,3 @@ > <child> > + <object class="GtkButton" id="new-printer-add-button"> > + <property name="label" translatable="yes">_Add</property> > > Add a comment for translators here please. > > ::: panels/printers/pp-new-printer-dialog.c > @@ +730,3 @@ > gtk_spinner_start (GTK_SPINNER (spinner)); > + gtk_stack_set_visible_child_name (GTK_STACK (stack), "loading-page"); > + gtk_header_bar_set_subtitle (GTK_HEADER_BAR (header_bar), > _("Searching for Printers")); > > Add a comment for translators here please. > > @@ +1702,3 @@ > if (pp_print_device_get_device_location (device) != NULL && > pp_print_device_get_device_location (device)[0] != '\0') > { > /* Translators: Location of found network printer (e.g. > Kitchen, Reception) */ > > You can remove this comment now. > > @@ +1707,3 @@ > else if (pp_print_device_get_host_name (device) != NULL && > pp_print_device_get_host_name (device)[0] != '\0') > { > /* Translators: Network address of found printer */ > > You can remove this comment now. Sure.
(In reply to Felipe Borges from comment #4) > Thanks for your review. > > The design should cover the situation when we already have some printers > > found and we are still searching for other ones like when you have a local > > USB printer and you enter a server name. In this situation we are still > > searching but nothing indicates that. > > According to the mockups at > https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines "Spinner is > displayed while search is taking place and nothing has been found". And > "Ongoing search is indicated by the sub-heading". These are notes at the > bottom of each dialog mockup. Although it is written at the bottom of each of dialog's mockups I don't see the indication text when I enter a text to the entry after the first search ended. And even if it would work the way as is described in the mockup the inconsistency between those two indicators would mean a lot of bugs in the future. From my point of view first thing I see is that there is a spinner so it is doing something. Few seconds later I see that the spinner has gone so I assume that the operation has finished and no little text from heading which is dimmed doesn't change this. Btw, the jumping of the title caused by disappearing of the indication text is something I don't like too.
Created attachment 334742 [details] [review] printers: Move "Add Printer" dialog buttons to header bar This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 334743 [details] [review] printers: Rename "Add a New Printer" dialog title to "Add Printer" This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 334744 [details] [review] printers: Drop "Add Printer" dialog internal borders This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 334745 [details] [review] printers: Update "No Printers Found" page in "Add Printers" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 334746 [details] [review] printers: Redesign loading page in the "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines The spinner is displayed while search is taking place AND nothing has been found. Ongoing search is indicated by the sub-heading.
Created attachment 334747 [details] [review] printers: Add grid lines between items in "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Comment on attachment 319269 [details] [review] printers: redesign the Add New Printer dialog I split the feature in the following patches. The authentication part (merging the pp-authentication-dialog into the pp-new-printer-dialog) comes next (patches to come). No need to rush to review these patches since we are in UI freeze. These patches are targeting the next cycle.
Created attachment 335457 [details] [review] printers: Redesign loading page in the "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines The spinner is displayed while search is taking place AND nothing has been found. Ongoing search is indicated by the sub-heading.
Created attachment 335458 [details] [review] printers: Add grid lines between items in "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 335459 [details] [review] printers: Merge authentication dialog into "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Comment on attachment 334742 [details] [review] printers: Move "Add Printer" dialog buttons to header bar This patch has been pushed already as cf99ceb.
Comment on attachment 334742 [details] [review] printers: Move "Add Printer" dialog buttons to header bar my mistake, this is a different dialog.
Review of attachment 334742 [details] [review]: You've also renamed "Authenticate" button to "Unlock" button, mention it in the description. You've forgot to rename "authenticate-button" to "unlock-button" in pp_new_printer_dialog_init().
Review of attachment 334743 [details] [review]: ::: panels/printers/new-printer-dialog.ui @@ +28,3 @@ <property name="can_focus">False</property> <property name="border_width">5</property> + <property name="title" translatable="yes">Add Printer</property> Add here comment for translators. ::: panels/printers/pp-new-printer-dialog.c @@ +416,3 @@ g_signal_connect (widget, "search-changed", G_CALLBACK (search_entry_changed_cb), dialog); + widget = WID ("unlock-button"); Move this change to the previous patch.
Review of attachment 334744 [details] [review]: Looks good.
Created attachment 335991 [details] [review] printers: Move "Add Printer" dialog buttons to header bar It also renames the "Authenticate" button to "Unlock". This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 335992 [details] [review] printers: Rename "Add a New Printer" dialog title to "Add Printer" This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
(In reply to Felipe Borges from comment #9) > Created attachment 334745 [details] [review] [review] > printers: Update "No Printers Found" page in "Add Printers" dialog > > This is a redesign based on the newest mockups available at > https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines I'm looking at this patch but the image is much darker than on the mock-up. Is it intentional? Maybe some "opacity" setting would solve this (+ dim-label on the label). Otherwise it looks good.
Review of attachment 335991 [details] [review]: Looks good now.
Review of attachment 335992 [details] [review]: Looks good.
Review of attachment 335458 [details] [review]: LGTM
(In reply to Felipe Borges from comment #10) > Created attachment 334746 [details] [review] [review] > printers: Redesign loading page in the "Add Printer" dialog > > This is a redesign based on the newest mockups available at > https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines > > The spinner is displayed while search is taking place AND nothing > has been found. Ongoing search is indicated by the sub-heading. The spinner is too dark, according to the mockup it should be light. Could you change it?
Attachment 334744 [details] pushed as a0e75d5 - printers: Drop "Add Printer" dialog internal borders Attachment 335458 [details] pushed as debb21d - printers: Add grid lines between items in "Add Printer" dialog Attachment 335991 [details] pushed as 8f57e4d - printers: Move "Add Printer" dialog buttons to header bar Attachment 335992 [details] pushed as bd25c8d - printers: Rename "Add a New Printer" dialog title to "Add Printer"
Created attachment 336071 [details] [review] printers: Redesign loading page in the "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines The spinner is displayed while search is taking place AND nothing has been found. Ongoing search is indicated by the sub-heading.
(In reply to Felipe Borges from comment #29) > Created attachment 336071 [details] [review] [review] > printers: Redesign loading page in the "Add Printer" dialog > > This is a redesign based on the newest mockups available at > https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines > > The spinner is displayed while search is taking place AND nothing > has been found. Ongoing search is indicated by the sub-heading. The brightness of the spinner is still lower than on the mockup. Is it intentional, or not?
Review of attachment 335459 [details] [review]: Thank you for the patch. I have several comments: "Unlock" is sensitive once the authentication content is shown and become insensitive while I start typing. The username and password entries should handle press of 'Enter'. The authentication dialog is quite wide which somehow disconnects the idea of single dialog with changing content once it jumps in size. The 'data->waiting' has to be set to FALSE once we are done or it will block any other attempts to get samba printers. There is a back button on the authentication dialog in the mockup instead of the Cancel button which I kind of miss when testing this. I see the 'Searching for Printers' text in the authentication dialog which should not be there. It seems that the spinner is shown when there is any search for printers, it should disappear and show actual list of printers when it finds any and let it grow (showing just the tiny "Searching for Printers"). The authentication dialog showed username provided by samba before. Unfortunately, this is provided by auth function which has some delay before it is executed. Maybe we could show system username at least. It would be great if the whole dialog could be taller to get closer to the mockup's ratio 1:1. I get good result with size set to 500x500. Could you create a patch for this (maybe the other dialogs needs this changes too)? ::: panels/printers/pp-new-printer-dialog.c @@ +390,3 @@ { + /* Translators: %s is the server name about to be authenticated by the dialog. */ + label = g_strdup_printf (_("Unlock %s"), server_name); You are leaking this pointer. @@ +442,3 @@ + gtk_widget_set_sensitive (WID ("unlock-button"), + strlen (get_entry_text ("username-entry", dialog)) > 0 && + strlen (get_entry_text ("password-entry", dialog)) > 0); You are leaking those 2 pointers here. Also, you should check the results for NULLs. @@ +508,3 @@ + g_signal_connect (widget, "changed", G_CALLBACK (auth_entries_changed), dialog); + widget = WID ("password-entry"); + g_signal_connect (widget, "changed", G_CALLBACK (auth_entries_changed), dialog); You don't need to use the 'widget' here since you use it just once. The WID ("...") seems short enough. @@ +1527,3 @@ } + priv->samba_host = pp_samba_new (PP_NEW_PRINTER_DIALOG (data->dialog), The 'data->dialog' is already 'PpNewPrinterDialog *'. ::: panels/printers/pp-samba.c @@ +243,1 @@ + g_signal_connect (data->parent, "unlock", G_CALLBACK (auth_cb), user_data); I really don't like this way. I would prefer to have new signal in PpHost class which would ask for username and password. The new printer dialog would connect to it. This would handle scenario when user enters address of an authenticated samba server. The dialog would get signal that username and password is needed and would provide it via a "pp_host_set_auth_info()". We could use the "pp_host_set_auth_info()" with the authentication of selected server in the way that we would ask for username and password and after that we would create the PpSamba and called the "pp_host_set_auth_info()" on it with consequent pp_samba_get_devices_async().
(In reply to Marek Kašík from comment #31) > Review of attachment 335459 [details] [review] [review]: > > It seems that the spinner is shown when there is any search for printers, it > should disappear and show actual list of printers when it finds any and let > it grow (showing just the tiny "Searching for Printers"). Have you applied this patch after the one at Attachment 336071 [details]?
(In reply to Felipe Borges from comment #32) > (In reply to Marek Kašík from comment #31) > > Review of attachment 335459 [details] [review] [review] [review]: > > > > It seems that the spinner is shown when there is any search for printers, it > > should disappear and show actual list of printers when it finds any and let > > it grow (showing just the tiny "Searching for Printers"). > > Have you applied this patch after the one at Attachment 336071 [details]? Yes, I see it applied there.
Created attachment 337426 [details] [review] printers: Redesign loading page in the "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines The spinner is displayed while search is taking place AND nothing has been found. Ongoing search is indicated by the sub-heading.
Created attachment 337427 [details] [review] printers: Redesign loading page in the "Add Printer" dialog This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines The spinner is displayed while search is taking place AND nothing has been found. Ongoing search is indicated by the sub-heading.
Created attachment 337428 [details] [review] printers: Set "No Printers Found" icon opacity equal to the spinner's In the "Add New Printer" dialog.
Review of attachment 334745 [details] [review]: Thank you for this patch. It looks good once I apply also the "printers: Set "No Printers Found" icon opacity equal to the spinner's" one. Merge them together and push it to master please.
Review of attachment 337428 [details] [review]: Merge this with the "printers: Update "No Printers Found" page in "Add Printers" dialog" please.
Review of attachment 337427 [details] [review]: This patch looks good to me. Thank you for working on this.
Attachment 334745 [details] pushed as 9eedd9f - printers: Update "No Printers Found" page in "Add Printers" dialog Attachment 337427 [details] pushed as 116f763 - printers: Redesign loading page in the "Add Printer" dialog
Created attachment 339706 [details] [review] printers: Make PpSamba a derivated class of PpHost
Created attachment 339707 [details] [review] printers: Introduce the "authentication-required" signal in PpHost This signal should be emitted by hosts and derivated classes when it is required to authenticate hosts (ask for credentials).
Created attachment 339708 [details] [review] printers: Merge authentication dialog into "Add Printer" This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 339709 [details] [review] printers: Drop PpAuthenticationDialog This dialog is no longer necessary since the authentication form is now handled in the PpNewPrinterDialog itself.
Created attachment 339710 [details] [review] printers: Decouple PpSamba and the PpNewPrinterDialog PpSamba no longer needs to hold a reference to the dialog window.
Created attachment 339711 [details] [review] printers: Allow dismissing authentication in "Add New Printer" dialog Introduce a "go-back" button allowing to get back to the list of printers when the user is exposed to the authentication form in the "Add New Printer" dialog. This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 339712 [details] [review] printers: Use "hostname" in the "Add Printer" dialog
Review of attachment 339706 [details] [review]: Thank you for the patch. It looks good except the g_object_get vs. g_object_get_data issue. ::: panels/printers/pp-samba.c @@ +459,3 @@ smbc_setOptionUserData (smb_context, data); + hostname = g_object_get_data (object, "hostname"); g_object_get_data() will not give you value of a property, you have to use g_object_get() here (and free the string later). @@ +496,3 @@ + gchar *hostname; + + hostname = g_object_get_data (G_OBJECT (samba), "hostname"); Ditto.
Review of attachment 339707 [details] [review]: This patch looks good to me.
Comment on attachment 339707 [details] [review] printers: Introduce the "authentication-required" signal in PpHost Attachment 339707 [details] pushed as af9fd3e - printers: Introduce the "authentication-required" signal in PpHost
Created attachment 339956 [details] [review] printers: Make PpSamba a derivated class of PpHost
Review of attachment 339956 [details] [review]: Thank you for the changes. Push it to master please.
Comment on attachment 339956 [details] [review] printers: Make PpSamba a derivated class of PpHost Attachment 339956 [details] pushed as e980b19 - printers: Make PpSamba a derivated class of PpHost
Review of attachment 339708 [details] [review]: Thank you for the patch. It still need some work. Also the username field should have focus once it shows up (if it is empty, if not then focus the password field). ::: panels/printers/pp-new-printer-dialog.c @@ +335,3 @@ static void +go_to_page (PpNewPrinterDialog *dialog, + gchar *page) Make it const please. @@ +379,3 @@ + { + g_clear_pointer (&password, g_free); + return; Shouldn't we also return when username is NULL? @@ +386,3 @@ + go_to_page (data->dialog, ADDPRINTER_PAGE); + g_signal_handlers_disconnect_by_func (WID ("unlock-button"), + on_authenticate, NULL); Why do you need this? @@ +406,3 @@ + go_to_page (dialog, AUTHENTICATION_PAGE); + + g_signal_connect (WID ("unlock-button"), "clicked", G_CALLBACK (on_authenticate), data); I don't like the recycling of the button here. Add please another button (probably with the same text) which will be dedicated to setting of the authentication info. @@ +408,3 @@ + g_signal_connect (WID ("unlock-button"), "clicked", G_CALLBACK (on_authenticate), data); + g_signal_connect (WID ("username-entry"), "activate", G_CALLBACK (on_authenticate), data); + g_signal_connect (WID ("password-entry"), "activate", G_CALLBACK (on_authenticate), data); I think that you don't need all the connections. Just set 'can-default' and 'has-default' for the 'Unlock' button and it will receive the default. @@ +409,3 @@ + g_signal_connect (WID ("username-entry"), "activate", G_CALLBACK (on_authenticate), data); + g_signal_connect (WID ("password-entry"), "activate", G_CALLBACK (on_authenticate), data); +} Add new line here. @@ +426,3 @@ + + if (username != NULL && password != NULL) + can_authenticate = (strlen (username) > 0 && strlen (password) > 0); username[0] != '\0' is optimal here (also for the password). ::: panels/printers/pp-samba.c @@ +138,1 @@ + priv->waiting = FALSE; This has to be done also when the authentication dialog is cancelled so maybe call this function with NULL as username and password from the PpNewPrinterDialog. Otherwise it will wait for 'waiting == FALSE' forever. ::: panels/printers/pp-samba.h @@ +66,3 @@ +void pp_samba_set_auth_info (PpSamba *samba, + gchar *username, + gchar *password); Make it const please.
Review of attachment 339709 [details] [review]: This looks good to me. Push it once the merge of authentication dialog is pushed.
Review of attachment 339710 [details] [review]: Please push this once the previous patches are pushed.
Review of attachment 339711 [details] [review]: You have to cancel the search when you are returning from the authentication page otherwise you won't be able to authenticate again (the waiting member of PpSambaPrivate). I see that when I have fonts scaled 1.2x on my machine the back button of the whole gnome-control-center has different proportions, unify this please.
Review of attachment 339712 [details] [review]: I see that title of the authentication dialog still says "Add Printer". Do you plan another patch for that? Maybe this patch would be good one for this change too. ::: panels/printers/pp-new-printer-dialog.c @@ +411,3 @@ + g_object_get (G_OBJECT (host), "hostname", &hostname, NULL); + /* Translators: Samba server needs authentication of the user to show list of its printers. */ + text = g_strdup_printf (_("Unlock %s."), hostname); You are leaking this pointer.
Created attachment 340560 [details] [review] printers: Merge authentication dialog into "Add Printer" This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
(In reply to Marek Kašík from comment #57) > Review of attachment 339711 [details] [review] [review]: > > You have to cancel the search when you are returning from the authentication > page otherwise you won't be able to authenticate again (the waiting member > of PpSambaPrivate). Now handled in attachment 340560 [details] [review]. > I see that when I have fonts scaled 1.2x on my machine the back button of > the whole gnome-control-center has different proportions, unify this please. I'm sorry, I don't understand what you mean here.
Created attachment 340562 [details] [review] printers: Use "hostname" in the "Add Printer" dialog
Created attachment 340563 [details] [review] printers: Merge authentication dialog into "Add Printer" This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Review of attachment 340563 [details] [review]: ::: panels/printers/pp-new-printer-dialog.c @@ +417,3 @@ + go_to_page (dialog, AUTHENTICATION_PAGE); + + Since the samba_host object might be destroyed after the return of the authentication process. @@ +477,3 @@ + "authentication-required", + G_CALLBACK (on_authentication_required), + dialog, 0); Make sure that the samba_host is going to exist for all the possible signal emissions. ::: panels/printers/pp-samba.c @@ +75,3 @@ PpSambaPrivate); + + samba->priv->waiting = TRUE; Make sure that the waiting property is set TRUE only once (when the object is initialized).
Created attachment 340586 [details] Back buttons (In reply to Felipe Borges from comment #60) > (In reply to Marek Kašík from comment #57) > > Review of attachment 339711 [details] [review] [review] [review]: > > > > You have to cancel the search when you are returning from the authentication > > page otherwise you won't be able to authenticate again (the waiting member > > of PpSambaPrivate). > > Now handled in attachment 340560 [details] [review] [review]. > > > I see that when I have fonts scaled 1.2x on my machine the back button of > > the whole gnome-control-center has different proportions, unify this please. > > I'm sorry, I don't understand what you mean here. I have 'org.gnome.desktop.interface.text-scaling-factor' set to 1.2 and the back buttons on gnome-control-center looks a little different. See the screenshot.
(In reply to Marek Kašík from comment #64) > Created attachment 340586 [details] > Back buttons > > (In reply to Felipe Borges from comment #60) > > (In reply to Marek Kašík from comment #57) > > > Review of attachment 339711 [details] [review] [review] [review] [review]: > > > > > > You have to cancel the search when you are returning from the authentication > > > page otherwise you won't be able to authenticate again (the waiting member > > > of PpSambaPrivate). > > > > Now handled in attachment 340560 [details] [review] [review] [review]. > > > > > I see that when I have fonts scaled 1.2x on my machine the back button of > > > the whole gnome-control-center has different proportions, unify this please. > > > > I'm sorry, I don't understand what you mean here. > > I have 'org.gnome.desktop.interface.text-scaling-factor' set to 1.2 and the > back buttons on gnome-control-center looks a little different. See the > screenshot. Oh, right. The buttons are expanding vertically.
Created attachment 340589 [details] [review] printers: Allow dismissing authentication in "Add New Printer" dialog Introduce a "go-back" button allowing to get back to the list of printers when the user is exposed to the authentication form in the "Add New Printer" dialog. This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Review of attachment 340589 [details] [review]: Thank you for the change. It looks good to me now.
Review of attachment 340562 [details] [review]: This looks good to me know. Thank you.
Review of attachment 340563 [details] [review]: This still needs some work. ::: panels/printers/pp-new-printer-dialog.c @@ +396,3 @@ + PpSamba *samba = PP_SAMBA (user_data); + + pp_samba_set_auth_info (samba, NULL, NULL); You need to call this when user clicks on the back button, otherwise I am not able to try the authentication again without leaving the add new printer dialog. @@ +440,3 @@ + if (username != NULL && username[0] != '\0' && + password != NULL && password[0] != '\0') + can_authenticate = (strlen (username) > 0 && strlen (password) > 0); I meant to use the 'username[0] != '\0'' instead of the 'strlen > 0'. This way you are checking it twice. ::: panels/printers/pp-samba.c @@ +75,3 @@ PpSambaPrivate); + + samba->priv->waiting = TRUE; The 'waiting' member has to be set only when there is need for authentication. It is used for situation when you need to get password from user during a sync callback from samba context. You have to clear it once the authentication is over.
Created attachment 340768 [details] [review] printers: List new printers ASAP in the "Add Printer" dialog List a printer in the "Add Printer" dialog as soon as it is discovered. The header subtitle "Searching for Printers" denotes that the Search is not done yet.
Created attachment 340772 [details] [review] printers: Merge authentication dialog into "Add Printer" This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Created attachment 340773 [details] [review] printers: Allow dismissing authentication in "Add New Printer" dialog Introduce a "go-back" button allowing to get back to the list of printers when the user is exposed to the authentication form in the "Add New Printer" dialog. This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
(In reply to Marek Kašík from comment #69) > Review of attachment 340563 [details] [review] [review]: > > This still needs some work. > > ::: panels/printers/pp-new-printer-dialog.c > @@ +396,3 @@ > + PpSamba *samba = PP_SAMBA (user_data); > + > + pp_samba_set_auth_info (samba, NULL, NULL); > > You need to call this when user clicks on the back button, otherwise I am > not able to try the authentication again without leaving the add new printer > dialog. Got it. I'm introducing this in the next patch: attachment 340773 [details] [review].
Review of attachment 340768 [details] [review]: This patch looks good to me. Thanks.
Comment on attachment 340768 [details] [review] printers: List new printers ASAP in the "Add Printer" dialog Attachment 340768 [details] pushed as 66c7f45 - printers: List new printers ASAP in the "Add Printer" dialog
Created attachment 341241 [details] [review] 0001-printers-Merge-authentication-dialog-into-Add-Printe.patch
Created attachment 341242 [details] [review] 0002-printers-Allow-dismissing-authentication-in-Add-New-.patch
Created attachment 341243 [details] [review] 0003-printers-Use-hostname-in-the-Add-Printer-dialog.patch
Created attachment 341244 [details] [review] 0004-printers-Decouple-PpSamba-and-the-PpNewPrinterDialog.patch
Created attachment 341245 [details] [review] 0005-printers-Drop-PpAuthenticationDialog.patch
Review of attachment 341241 [details] [review]: Thank you for this patch. Lets get it in.
Review of attachment 341242 [details] [review]: When I press the back button there are two scenarios which happens. First one is that it returns to the list of available printers but the SMB server which we've previously tried to authenticate is not on the list and the text in the headerbar indicates that there is some search for printers. Second scenario is that few seconds after the return to the list of available printers the authentication dialog is shown again.
Created attachment 341753 [details] [review] 0001-printers-Allow-dismissing-authentication-in-Add-New-.patch
Created attachment 341754 [details] [review] 0002-printers-Prevent-PpSamba-from-emiting-authentication.patch
Created attachment 341755 [details] [review] 0003-printers-Do-not-remove-devices-from-Add-Printer-list.patch
Created attachment 341756 [details] [review] 0004-printers-Unselect-printers-when-dismissing-authentic.patch
Created attachment 341757 [details] [review] 0005-printers-Use-hostname-in-the-Add-Printer-dialog.patch
Created attachment 341758 [details] [review] 0006-printers-Decouple-PpSamba-and-the-PpNewPrinterDialog.patch
Created attachment 341759 [details] [review] 0007-printers-Drop-PpAuthenticationDialog.patch
Review of attachment 341755 [details] [review]: You have to remove the SMB server if the authentication is successful, otherwise you would end up with the server in the list and also with its printers (if you clean the entry).
Review of attachment 341754 [details] [review]: If this is the patch which should fix the automatic showing of authentication dialog once we go back from it then it doesn't fix this issue for me. (tell me if not please)
Review of attachment 341756 [details] [review]: I see that the server is unselected and the button is the "Add" one but it is still sensitive which it shouldn't be.
Review of attachment 341757 [details] [review]: Since this looks almost same as the last accepted version I'm accepting it. Thank you.
Review of attachment 341758 [details] [review]: This looks good to me. Thanks.
Review of attachment 341759 [details] [review]: This looks good to me. Thanks
Comment on attachment 341241 [details] [review] 0001-printers-Merge-authentication-dialog-into-Add-Printe.patch Attachment 341241 [details] pushed as afac6d5 - printers: Merge authentication dialog into "Add Printer"
Attachment 341757 [details] pushed as ebe21b6 - printers: Use "hostname" in the "Add Printer" dialog Attachment 341758 [details] pushed as e8cdaea - printers: Decouple PpSamba and the PpNewPrinterDialog Attachment 341759 [details] pushed as bd10fb5 - printers: Drop PpAuthenticationDialog
Created attachment 343727 [details] [review] printers: Allow dismissing authentication in "Add New Printer" dialog Introduce a "go-back" button allowing to get back to the list of printers when the user is exposed to the authentication form in the "Add New Printer" dialog. This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
(In reply to Felipe Borges from comment #98) > Created attachment 343727 [details] [review] [review] > printers: Allow dismissing authentication in "Add New Printer" dialog > > Introduce a "go-back" button allowing to get back to the list of > printers when the user is exposed to the authentication form in > the "Add New Printer" dialog. > > This is a redesign based on the newest mockups available at > https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines This patch should already solve the authentication dialog being shown after dismissed, and the topright-buttons sensitivity.
Review of attachment 343727 [details] [review]: Thank you for the changes. It still needs some tweaking. One problem is that once you return back from the authentication dialog the 'Searching...' text is shown even if it is not already searching. This is caused by non-NULL pointer to samba_host. ::: panels/printers/pp-new-printer-dialog.c @@ +445,2 @@ static void +on_go_back_button_clicked (GtkButton *button, You should also clean the priv->samba_host pointer in this function (otherwise there is the 'Searching...' text in the title). @@ +450,3 @@ + PpNewPrinterDialogPrivate *priv = dialog->priv; + + pp_samba_set_auth_info (priv->samba_host, NULL, NULL); We should handle better the setting of the NULL in the function and also set username and password to '\0' wight after the while() loop to signalize that we want to cancel the authentication. ::: panels/printers/pp-samba.c @@ +112,3 @@ PpSamba *samba = PP_SAMBA (data->samba); + if (samba->priv->waiting) Lets use the cancelled member of SMBData to control whether it is running for the second time. @@ +117,1 @@ + samba->priv->waiting = TRUE; Lets move this right before the "while (samba->priv->waiting)" in auth_fn().
Created attachment 344414 [details] [review] printers: Allow dismissing authentication in "Add New Printer" dialog Introduce a "go-back" button allowing to get back to the list of printers when the user is exposed to the authentication form in the "Add New Printer" dialog. This is a redesign based on the newest mockups available at https://wiki.gnome.org/Design/SystemSettings/Printers#Guidelines
Review of attachment 344414 [details] [review]: Thank you for the patch. Please push it to master.
I'll ask you for one more change. The aspect ratio of the new printer dialog should be closer to 1:1 according to the mockup. Could you prepare a patch for that?
Comment on attachment 344414 [details] [review] printers: Allow dismissing authentication in "Add New Printer" dialog (In reply to Marek Kašík from comment #103) > I'll ask you for one more change. The aspect ratio of the new printer dialog > should be closer to 1:1 according to the mockup. Could you prepare a patch > for that? sure. Thanks for your reviews! Attachment 344414 [details] pushed as 6f3ed90 - printers: Allow dismissing authentication in "Add New Printer" dialog