GNOME Bugzilla – Bug 650244
Network Manager system dialogs
Last modified: 2011-10-18 21:29:49 UTC
(I'm opening a bug for this because I'm not always available at good times in #gnome-design, and gnome-shell-list is not really followed) Currently, gnome-shell's integrated NetworkManager indicator relies on nm-applet to present dialogs for configuring certain connections (mainly for passwords). As nm-applet is deprecated, for 3.2 we want to remove this dependency, allowing for system modal dialogs as explained in https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemDialogs Part of the reason we didn't switch in 3.0 was a technical problem (implementing NMSecretAgent, talking to gnome-keying, having a memlocked StPasswordEntry, etc.), part is that designs for the dialogs are missing at the moment. First of all, is this the right approach? Are we still convinced that network dialogs should be system modal? Or it is better to use a notification like bluetooth does? (Of course, a notification would only work for the simpler dialogs, asking only a password or a pin code). Before asking, consider that NetworkManager will sometime connect "spontaneously", for example if a previously connected access point appears and the device is not otherwise used. In that case it might be disrupting to show a system modal dialog. (And for technical reasons I would like to avoid having different code paths based on whether the user interacted recently with the network menu or not). Second question: should all network dialogs be handled by the shell? I'm particular I'm referring to the "Dialog Of Doom", http://git.gnome.org/browse/network-manager-applet/tree/src/wifi.ui plus all of http://git.gnome.org/browse/network-manager-applet/tree/src/wireless-security (one at a time). With most complex security modes (WPA2 Enterprise with PEAP for example) this becomes huge and very complex. So would it make sense to say that these dialogs are not just asking for a password and therefore should go to the network panel in the control-center? The problem with this would be how to summon these dialogs if one decides to connect to WPA2 Enterprise, given that there would be no daemon to talk on dbus (a simple "gnome-control-center network --wifi-config <device> <ap>" would be enough, if possible, or maybe a well-known dbus name). Another problematic dialog is the mobile wizard, which is required the first time you use mobile broadband. I definitely don't want to see a system-modal shell-themed GtkAssistant, but I'm not sure where it should go.
nm-applet recently got changed so it only shows a less frightening dialog for wpa2 enterprise when just attempting to connect. Most of the other settings can not be changed at that time, anyway. These simpler dialogs are the only thing that we should have as shell-style system dialogs, imo. Full-on configuration of wpa2 enterprise is not in scope.
(In reply to comment #1) > nm-applet recently got changed so it only shows a less frightening dialog for > wpa2 enterprise when just attempting to connect. Most of the other settings can > not be changed at that time, anyway. > > These simpler dialogs are the only thing that we should have as shell-style > system dialogs, imo. Full-on configuration of wpa2 enterprise is not in scope. Except that connecting to wpa2 enterprise for the first time requires all those settings. 3.0.0 did not provide them, so wpa2ent did not work, and we had to introduce another dbus method to nmapplet in 3.0.1. So the answer is: what should we do when the user tries to autoconfigure a connection we know requires more input (wpa2ent, mobile broadband, ieee8021x, so far)? Do we have a good way to summon the appropriate control center panel? (and will it provide the necessary features?)
After reading through nm-applet sources, and not seeing any input from designers, I implemented the NMSecretAgent part, thus handling only passwords. This means that gnome-shell still calls out to nm-applet, except that now it also conflicts with it, as you cannot have more than one secret agent per session. I think complex dialogs should be handled by nm-connection-editor and/or gnome-control-center, so I'll look at splitting them in the network-manager-applet module. In any case, posting here what I have, since UI freeze is behind the corner.
Created attachment 194258 [details] [review] Add a new network agent for the Shell A network agent is a component that stores network secrets (like wifi passwords) in the session keyring. This commit adds an implementation of it to be used by the shell network dialogs. It handles most of the keyring stuff, delegating the UI to upper layers.
Created attachment 194259 [details] [review] Add a system modal dialog for network secrets Using the new ShellNetworkAgent, show a system modal dialog (similar to the PolicyKit one) when NetworkManager needs secrets for connecting to wireless.
So as i understand it, this requires an nm-applet patch which does not yet exist? Probably too late for 3.2 if so.
Created attachment 194393 [details] [review] Complete transitioning away from nm-applet Wireless and 3g dialog code has moved to gnome-control-center, so we can stop calling out to nm-applet. Also, we can now enable the notifications provided by the shell and kill a bit of code about auth that is not actually needed.
(In reply to comment #6) > So as i understand it, this requires an nm-applet patch which does not yet > exist? Probably too late for 3.2 if so. Now it exists. Is it still too late?
no, it's not too late, assuming the NM parts can get in too
Comment on attachment 194258 [details] [review] Add a new network agent for the Shell looks good. I'm assuming a lot of this is just copied from nm-applet, so I've made no effort to figure out if, eg, the gnome-keyring calls are correct. >- polkit-agent-1 >= $POLKIT_MIN_VERSION xfixes) >+ polkit-agent-1 >= $POLKIT_MIN_VERSION xfixes >+ libnm-glib libnm-util gnome-keyring-1) tabs vs spaces also, shell-network-agent.c should be reindented to shell coding style. >+/* If these are kept in sync with nm-applet, secrets will be shared */ >+#define KEYRING_UUID_TAG "connection-uuid" maybe they should be in a header file then? >+ /* <gchar* setting_key, gchar *secret> */ >+ /* <char* request_id, ShellAgentRequest *> */ trivial, but can you be consistent about char vs gchar and "char *foo" vs "char* foo" >+ g_hash_table_foreach (priv->requests, iterate_cancel_cb, object); if you use a GHashTableIterator instead you can avoid having to create and destroy a GError each time >+ "The secret request was cancelled by the user"); should this (and other GErrors) be translated? >+ /* Don't system-owned or always-ask secrets */ missing the word "save". (also, it's not clear to me how/if the code after this skips always-ask secrets)
Comment on attachment 194259 [details] [review] Add a system modal dialog for network secrets >+ this._buttonInfos = buttons; this doesn't seem to be used? >+ let descriptionLabel = new St.Label({ style_class: 'polkit-dialog-description', >+ text: this._content.message, >+ // HACK: for reasons unknown to me, the label >+ // is not asked the correct heigth for width, >+ // and thus is underallocated >+ // place a fixed heigth to avoid overflowing >+ style: 'height: 3em' >+ }); This may be because .polkit-dialog has a fixed width, and something in St isn't dealing with that correctly. (Having polkit-dialog have a fixed width in px is also completely broken, since it means the text reliably gets truncated if you have text-zoom turned on to compensate for your monitor dpi. But that's an existing bug.) Although this adds to the bug, since with text-zoom set to "Larger", the message will generally end up too long to fit in 3em height. Putting it in an St.Bin might help? (If you leave the hack, fix the spelling of "height" in both places.) >+ let outer = new St.Button({ button_mask: St.ButtonMask.ONE, ok, I should have commented on this in the last patch, but what does "outer" mean? Also, there needs to be more spacing between the entry and the label/checkbox. It's too cramped right now. >+ _updateOkButton: function() { I don't know exactly where's it's failing, but the OK button is always enabled for me; I can click it even when the password field is empty. Also, it should be grayed out when it's not reactive. I wasn't able to test anything other than WPA (so, eg, I don't know if the multi-field stuff like EAP works)
Comment on attachment 194393 [details] [review] Complete transitioning away from nm-applet >Also, we can now enable the >notifications provided by the shell Yeah... but do we want to? The "now connected" ones at least seem unnecessary and annoying. The code looks fine though
Created attachment 194756 [details] [review] Add a new network agent for the Shell A network agent is a component that stores network secrets (like wifi passwords) in the session keyring. This commit adds an implementation of it to be used by the shell network dialogs. It handles most of the keyring stuff, delegating the UI to upper layers.
Created attachment 194757 [details] [review] Add a system modal dialog for network secrets Using the new ShellNetworkAgent, show a system modal dialog (similar to the PolicyKit one) when NetworkManager needs secrets for connecting to wireless.
Created attachment 194758 [details] [review] Complete transitioning away from nm-applet Wireless and 3g dialog code has moved to gnome-control-center, so we can stop calling out to nm-applet. Also, we can now enable the notifications provided by the shell and kill a bit of code about auth that is not actually needed.
(In reply to comment #10) > (From update of attachment 194258 [details] [review]) > > >- polkit-agent-1 >= $POLKIT_MIN_VERSION xfixes) > >+ polkit-agent-1 >= $POLKIT_MIN_VERSION xfixes > >+ libnm-glib libnm-util gnome-keyring-1) > > tabs vs spaces > > also, shell-network-agent.c should be reindented to shell coding style. Fixed. > >+/* If these are kept in sync with nm-applet, secrets will be shared */ > >+#define KEYRING_UUID_TAG "connection-uuid" > > maybe they should be in a header file then? Moved, and correctly namespaced (otherwise gobject-introspection complains). > >+ /* <gchar* setting_key, gchar *secret> */ > > >+ /* <char* request_id, ShellAgentRequest *> */ > > trivial, but can you be consistent about char vs gchar and "char *foo" vs > "char* foo" I went with gchar everywhere. > >+ g_hash_table_foreach (priv->requests, iterate_cancel_cb, object); > > if you use a GHashTableIterator instead you can avoid having to create and > destroy a GError each time Done. > >+ "The secret request was cancelled by the user"); > > should this (and other GErrors) be translated? I don't think so, as NetworkManager ignores the error message, even in logs. > >+ /* Don't system-owned or always-ask secrets */ > > missing the word "save". (also, it's not clear to me how/if the code after this > skips always-ask secrets) Code is right. I reworded the comment to make it clearer. (In reply to comment #11) > (From update of attachment 194259 [details] [review]) > >+ this._buttonInfos = buttons; > > this doesn't seem to be used? Killed. > >+ let descriptionLabel = new St.Label({ style_class: 'polkit-dialog-description', > >+ text: this._content.message, > >+ // HACK: for reasons unknown to me, the label > >+ // is not asked the correct heigth for width, > >+ // and thus is underallocated > >+ // place a fixed heigth to avoid overflowing > >+ style: 'height: 3em' > >+ }); > > This may be because .polkit-dialog has a fixed width, and something in St isn't > dealing with that correctly. (Having polkit-dialog have a fixed width in px is > also completely broken, since it means the text reliably gets truncated if you > have text-zoom turned on to compensate for your monitor dpi. But that's an > existing bug.) Although this adds to the bug, since with text-zoom set to > "Larger", the message will generally end up too long to fit in 3em height. > > Putting it in an St.Bin might help? > > (If you leave the hack, fix the spelling of "height" in both places.) The only effect of using an St.Bin was that the label was no longer multiline, so I kept the hack. > >+ let outer = new St.Button({ button_mask: St.ButtonMask.ONE, > > ok, I should have commented on this in the last patch, but what does "outer" > mean? Uhm... the opposite of inner? Anyway, replaced with button. > Also, there needs to be more spacing between the entry and the label/checkbox. > It's too cramped right now. I hope 15px is enough. > >+ _updateOkButton: function() { > > I don't know exactly where's it's failing, but the OK button is always enabled > for me; I can click it even when the password field is empty. > > Also, it should be grayed out when it's not reactive. Fixed. (In reply to comment #12) > (From update of attachment 194393 [details] [review]) > >Also, we can now enable the > >notifications provided by the shell > > Yeah... but do we want to? The "now connected" ones at least seem unnecessary > and annoying. > > The code looks fine though I removed the "You're now connected" notification, but kept the other. I think that we do want notifications when connection goes down. I made them transient, though, as the persistent indicator lives at the top.
Attachment 194756 [details] pushed as 2af5e85 - Add a new network agent for the Shell Attachment 194757 [details] pushed as 2ebdc81 - Add a system modal dialog for network secrets Attachment 194758 [details] pushed as d896248 - Complete transitioning away from nm-applet
Could this please be made optional? I don't use NetworkManager (and probably will never), but these patches demand that I install a (for me) completely useless daemon with a bunch of libraries. Additionally, at runtime gnome-shell doesn't depend on this, it runs just fine without a nm-daemon running.
Thanks for the functionality. Still I was a little frustrated to find the dialog being modal. Especially since it pops up only after a while when it cannot make a connection. Would it be an idea to make this more like a notification?