GNOME Bugzilla – Bug 698532
Do not prompt for remote username / password when adding printer
Last modified: 2021-06-09 16:11:47 UTC
When connected to a large network, adding a printer is near impossible because of the username / password dialogue which appears to present itself for every device with network shares on the network. I should not have to click cancel several hundred times simply to add a printer. Either add the ability to disable this "feature" or remove it completely.
Created attachment 242205 [details] Screenshot if dialogue shown
Created attachment 245039 [details] authentication of found samba server Hi, thank you for the report. Attached is a screenshot of how this could be solved. We wouldn't ask user for authentication when a samba server which needs authentication is found but we would just show the server in the list. User would be asked for auth info only when he clicks on "Authenticate" button (which shows instead of "Add" button if a samba server is selected). We would also ask user for the auth info if the samba server is on the address he enters in the entry (without clicking on the "Authenticate" button). The dialog would show printers available on the server after authentication and the "server" item would disappear. What designers think about this? Regards Marek
Created attachment 246818 [details] [review] Temporary solution which doesn't need an UI change. This patch turns off asking for auth info for automatically found samba servers which needs authentication. You'll still be asked for password if you type the address of the server into the search entry manually. It doesn't affect listing of printers on samba servers which doesn't need authentication. Regards Marek
Comment on attachment 246818 [details] [review] Temporary solution which doesn't need an UI change. I've committed this to 3.8. It is not an ideal solution but I think that it is better one then annoying users with authentication users against all local samba servers. I will improve it for 3.10. Regards Marek
Created attachment 253516 [details] proposed workflow Attached is proposed workflow. It goes down from the top. Parts relevant to this bug are 2, 3 and 4.
(In reply to comment #5) > Created an attachment (id=253516) [details] > proposed workflow > > Attached is proposed workflow. It goes down from the top. Parts relevant to > this bug are 2, 3 and 4. Thanks for the screenshots. This looks reasonable, although I'm not a fan of opening modal dialogs on top of other modal dialogs. That should be OK for 3.10, but I want to revisit it next cycle. Two changes it would be nice to get in right now: 1. Change the icon for servers to network-server. Also add an emblem to indicate that it is locked if that is the case (you can use changes-prevent for that). 2. After a server has been unlocked, it would be helpful to show the printers that are available on that server only. One way to do that would be to automatically fill the search field with the name/address of the server.
(In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=253516) [details] [details] > > proposed workflow > > > > Attached is proposed workflow. It goes down from the top. Parts relevant to > > this bug are 2, 3 and 4. > > Thanks for the screenshots. This looks reasonable, although I'm not a fan of > opening modal dialogs on top of other modal dialogs. That should be OK for > 3.10, but I want to revisit it next cycle. > > Two changes it would be nice to get in right now: > > 1. Change the icon for servers to network-server. Also add an emblem to > indicate that it is locked if that is the case (you can use changes-prevent for > that). > > 2. After a server has been unlocked, it would be helpful to show the printers > that are available on that server only. One way to do that would be to > automatically fill the search field with the name/address of the server. Thank you very much for the design review. I'm going to finish the patch then. Btw, we already have the modal dialog there.
Created attachment 253860 [details] [review] Propagate cancellation of authentication dialog
Created attachment 253861 [details] [review] Enable use of emblems for printer icons
Created attachment 253862 [details] [review] Authenticate Samba servers only when enabled
Created attachment 253863 [details] [review] Handle cancelling of authentication dialog
Created attachment 253864 [details] [review] Show authenticated Samba servers in the list of new devices The attached patches implements the proposal with Allan's comments incorporated.
Created attachment 253939 [details] implemented workflow Attached are screenshots of workflow implemented by above patches (only the first screenshot and the third one are new to current workflow).
Review of attachment 253861 [details] [review]: ok
The flow looks fine from here. The modal dialog "Inception" can be addressed for 3.12 with a revealer + back button.
Review of attachment 253860 [details] [review]: ok
Review of attachment 253862 [details] [review]: OK
Review of attachment 253863 [details] [review]: Looks fine otherwise ::: panels/printers/pp-samba.c @@ +280,3 @@ data = (SMBData *) smbc_getOptionUserData (smb_context); + if (!data->cancelled) if (data->cancelled) return; would prevent the whole indentation increment below. @@ +371,3 @@ + device->is_authenticated_server = TRUE; + + data->devices->devices = g_list_append (data->devices->devices, device); I'd prefer if these 4 lines were consolidated in a small helper function since they are just copy&pasted from below. @@ +373,3 @@ + data->devices->devices = g_list_append (data->devices->devices, device); + + if (dir) This can't ever be true since we're inside an if (!dir && errno == EACCES) block.
Review of attachment 253864 [details] [review]: ::: panels/printers/new-printer-dialog.ui @@ +49,3 @@ </child> <child> + <object class="GtkNotebook" id="notebook"> Could use a GtkStack instead. But IMO we shouldn't be changing the button at all and instead the auth dialog should be triggered by the activate or even selected signal on the row. Anyway, this works and designers seem ok with it so let's just get it in as is. @@ +67,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">page 1</property> This isn't visible so we shouldn't expose it to translators. @@ +89,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">page 2</property> idem ::: panels/printers/pp-new-printer-dialog.c @@ +296,3 @@ +{ + gchar *server_name; + gpointer user_data; s/user_data/dialog @@ +331,3 @@ + device = (PpPrintDevice *) iter->data; + if (device->is_authenticated_server && + g_strcmp0 (data->server_name, device->host_name) == 0) Why do you check the host/server name? IIUC this is always true. @@ +332,3 @@ + if (device->is_authenticated_server && + g_strcmp0 (data->server_name, device->host_name) == 0) + cancelled = TRUE; You can break immediately. @@ +362,3 @@ + pp_print_device_free ((PpPrintDevice *) iter->data); + g_list_free (result->devices); + g_free (result); These 4 lines are repeated in this file quite a few times. I recommend creating a function to free these results variables and get rid of the pointless typedef PpDevicesList in pp-utils.h @@ +368,3 @@ + } + else + { Aren't we leaking user_data in this else branch? @@ +371,3 @@ + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + { + dialog = PP_NEW_PRINTER_DIALOG (user_data); ... and this should be user_data->dialog? @@ +737,3 @@ + else + { + if (device->is_authenticated_server && else if (...) ? @@ +1669,3 @@ + device->host_name, + /* Translators: This item is a samba server which needs authentication to show its printers */ + _("Authenticated Samba Server")); Not sure about this string. Do we generally mention "Samba" in the UI elsewhere or do we use a generic term? Perhaps "Print server needs authentication" ?
Oh, and this needs a UI freeze break exception of course.
Created attachment 253989 [details] [review] Simplify freeing of lists of found printers
Created attachment 253991 [details] [review] Show authenticated Samba servers in the list of new devices Thank you very much for the reviews. (In reply to comment #19) > Review of attachment 253864 [details] [review]: > > ::: panels/printers/new-printer-dialog.ui > @@ +49,3 @@ > </child> > <child> > + <object class="GtkNotebook" id="notebook"> > > Could use a GtkStack instead. But IMO we shouldn't be changing the button at > all and instead the auth dialog should be triggered by the activate or even > selected signal on the row. > > Anyway, this works and designers seem ok with it so let's just get it in as is. I left it as it is. > @@ +67,3 @@ > + <property name="visible">True</property> > + <property name="can_focus">False</property> > + <property name="label" translatable="yes">page > 1</property> > > This isn't visible so we shouldn't expose it to translators. Fixed. > @@ +89,3 @@ > + <property name="visible">True</property> > + <property name="can_focus">False</property> > + <property name="label" translatable="yes">page > 2</property> > > idem Fixed. > ::: panels/printers/pp-new-printer-dialog.c > @@ +296,3 @@ > +{ > + gchar *server_name; > + gpointer user_data; > > s/user_data/dialog Done. > @@ +331,3 @@ > + device = (PpPrintDevice *) iter->data; > + if (device->is_authenticated_server && > + g_strcmp0 (data->server_name, device->host_name) == 0) > > Why do you check the host/server name? IIUC this is always true. You are right in this case, I was too general here. > @@ +332,3 @@ > + if (device->is_authenticated_server && > + g_strcmp0 (data->server_name, device->host_name) == 0) > + cancelled = TRUE; > > You can break immediately. Done. > @@ +362,3 @@ > + pp_print_device_free ((PpPrintDevice *) iter->data); > + g_list_free (result->devices); > + g_free (result); > > These 4 lines are repeated in this file quite a few times. I recommend creating > a function to free these results variables and get rid of the pointless typedef > PpDevicesList in pp-utils.h I've implemented this in separate patch (see https://bugzilla.gnome.org/attachment.cgi?id=253989). > @@ +368,3 @@ > + } > + else > + { > > Aren't we leaking user_data in this else branch? Yes, we are. Fixed. > @@ +371,3 @@ > + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) > + { > + dialog = PP_NEW_PRINTER_DIALOG (user_data); > > ... and this should be user_data->dialog? Fixed. > @@ +737,3 @@ > + else > + { > + if (device->is_authenticated_server && > > else if (...) ? Done. > @@ +1669,3 @@ > + device->host_name, > + /* Translators: This item > is a samba server which needs authentication to show its printers */ > + _("Authenticated Samba > Server")); > > Not sure about this string. Do we generally mention "Samba" in the UI elsewhere > or do we use a generic term? Perhaps "Print server needs authentication" ? I've removed the "Samba" word from the string (We are not sure whether this is a print server, it can be just a file server). Thanks Marek
Review of attachment 253989 [details] [review]: Ok. Though I still don't get the point of the typedef and having to access the list indirectly instead of just making the result be the list. That could be a further clean up though.
Review of attachment 253991 [details] [review]: The code looks fine so I'm going to mark this a-c-n but let's not push this before having that string reviewed by someone else. ::: panels/printers/pp-new-printer-dialog.c @@ +1665,3 @@ + device->host_name, + /* Translators: This item is a server which needs authentication to show its printers */ + _("Authenticated Server")); It's not the server that's authenticated, it's the user that has to authenticate with the server, right? I'd like a comment from a native English speaker here.
Comment on attachment 253991 [details] [review] Show authenticated Samba servers in the list of new devices (In reply to comment #24) > Review of attachment 253991 [details] [review]: > > The code looks fine so I'm going to mark this a-c-n but let's not push this > before having that string reviewed by someone else. > > ::: panels/printers/pp-new-printer-dialog.c > @@ +1665,3 @@ > + device->host_name, > + /* Translators: This item > is a server which needs authentication to show its printers */ > + _("Authenticated > Server")); > > It's not the server that's authenticated, it's the user that has to > authenticate with the server, right? > > I'd like a comment from a native English speaker here. I've changed the string to "Server requires authentication" after discussion with Rui and Shaun. Thank you all for your help. Regards Marek
Created attachment 267417 [details] authentication of samba server (In reply to comment #6) > Thanks for the screenshots. This looks reasonable, although I'm not a fan of > opening modal dialogs on top of other modal dialogs. That should be OK for > 3.10, but I want to revisit it next cycle. Regarding the not opening modal dialogs on top of other modal dialogs. What do you think about getting auth info the way it is shown on the attached screenshot?
(It shows the auth info entries if user clicks on "Authenticate" when a server which needs authentication is selected)
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new bug report at https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/ Thank you for your understanding and your help.