> On Říj. 21, 2015, 7:41 dop., Heiko Tietze wrote: > > Looks very nice. But the question is whether or not users understand the > > status quickly and if the dropdown menu is clear enough (the eye is a > > toggle button). The alternative solution is just a radio button group below > > the input, which is furthermore easily accessible by disabled people. > > > > I'd place the "ask everytime" option as the first item since it is the > > default. > > Jan Grulich wrote: > Having a radio button group below each password field would make it heavy > I would say. I'm trying to remove as much visible components I can to make it > more simple. > > The "ask everytime" option is not the default option, the default option > should be to store secrets for the current user which should store secrets to > KWallet. > > Lamarque Souza wrote: > I agree with Jan that radio button group would pollute the ui. Talking > about the default option, wouldn't be better make "Always Ask" the default > for VPN connections? I know that is not consistent with the current default, > but VPN is kind of a special case since usually whoever is using it is more > concerned about security than convenience. By the way, good job at improving > Plasma NM. > > Jan Grulich wrote: > I agree, using "always ask" as default for VPN connections is something > we should consider. > > And thanks, plasma-nm is my passion so I don't have to force myself to > make it better :). > > Thomas Pfeiffer wrote: > I guess the source of the miscommunication between Heiko and you is that > the screenshot you chose is the simples scenario possible. In the case WPA & > WPA 2 Personal (PSK) there is indeed just that one field, and plenty of space > to put a radio button group right in the dialog. > I assume what you are worried about are the more complex security setting > scenarios, where the dialog is already full of fields and controls anyway, > and I can understand why you don't want to put even more stuff in those > dialogs. > > While I agree with Heiko that many users may not even notice the option > at all, I don't think that's a big problem as long as the default option is a > safe one. > > I have a few other concerns, though. > 1. Are the passwords stored in NM encrypted? I guess not, in which case > this has to be made clear to the user, because it increases the security > risk. Although this makes the label quite long, I'd put "(not encrypted)" at > the end, and "encrypted" behind the single-user option. Security-relevant > information has to be made available to the user. > 2. With the option "ask every time", the password is not stored at all, > right? So why would I enter a password in the dialog, only to then tell the > software to not store it? That does not really make sense, since this is not > the place where you connect for the first time. For consistency, I'd vote for > having a "store password" checkbox before the password field, like in most > other places where one has the option to store a password, and to only enable > the field when that checkbox is clicked > 3. "everytime" is not an English word, it has to be "every time" > 4. It has to be "for all users" > > Jan Grulich wrote: > 1) Yes, when they are stored in NM then they are not encrypted. I'll add > the info there. > 2) In case you select "ask every time" you don't need to put there your > password because the password field will be disabled. This gets even more > complicated in case where you have to specify that the password is not > required at all (e.g. necessary for GSM or CDMA) so checkbox for enabling the > password field probably won't work and again adds an element which doesn't > need to be there, at least I would like to keep it that way. > 3-4) I'l fix this. > > Jan Grulich wrote: > Updated screenshot: >  > > Heiko Tietze wrote: > So I can choose that the password is irrelevant? Weird :-) (just disable > dropdown) > Since none of you responded about accessibility I'd like to bring this > issue once again on the table. People who cannot use the mouse will not be > able to change the type of storage. Of course, you may consider it as > acceptable drawback of a nice solution. > > Thomas Pfeiffer wrote: > The "This password is not required" option is a problem. I said that the > bad discoverability of the option is not a big problem as long as nobody > actually _has_ to change the option. In that case, however, it would be > required to change it in order to keep the dialog from complaining that the > field is empty, right? So we need to find a different solution, or we'll end > up with frustrated users who cannot save their settings for a connection that > does not need a password because they don't know where they can set it. > > Actually, this is a problem with both "This password is not required" and > "Ask for this password every time" (which I'd rename to "Do not store > password", now that I'm reading it again): You have an option which disables > a field, but it's positioned _after_ the field. Since people will go through > the form from left to right, they will notice the option (if they notice it > at all) only _after_ seeing the password field and thinking that they will > have to fill it in. This breaks the workflow. > > Heiko is right about the potential accessibility problem. However, this > is relevant not only for this option but also for the visible password option > (that's relevant for a screenreader as well, isn't it?). So is there a way to > access those inline buttons via keyboard? > > Jan Grulich wrote: > We and even NM will let you to save the connection even when the password > is missing but if you do not mark the password as not required, NM will > probably ask for it during activation. This can be workarounded by using "not > required" option as default for certain connection types (e.g. in case of > gsm/cdma it would make sense). Btw. this already works in plasma-nm that way, > I didn't change functionality, I just removed comboboxes and moved this > options directly to the password field. > > What about putting this option to the left side of the password field? > Note that comboboxes were previously also on the right side so the workflow > is basically still the same. > > Heiko is unfortunately right, none of the options is accessible via > keyboard. I'm not sure what we can do about this right now. A password field > with an icon to show/hide the password was added officially as a new widget > to KDE Frameworks so we can try to complain there and then steal the solution > :D. I don't use directly the same widget because that would require > dependency on latest KF5 but I implemented it the same way. > > Jan Grulich wrote: > Ping? Can we move forward with this? I would ignore problems regarding > "This password is not required" option because this was not introduced in > this patch, the only change are removed comboboxes and added action icon in > the password field instead. Would be adding keyboard shortcuts enough to > solve the accessibility problem or does it need to be accessible using "tab" > key? > > Heiko Tietze wrote: > The inline controls have the conceptual flaw of reduced accessibility. > First option: You accept it. Disabled people just have to use different > methods. Or let's rather say the remaining issue goes into a new bug. Second > option: Inline controls are accessible per tab (just like being separated in > an extra button) and/or shortcut. This solution lacks on discoverability > although. Third option: Use conservative controls such as buttons. Would be > not that attractive as the inline controls. It's up to you resp. the > maintainer to decide how to proceed. > > BTW: The issue is also valid and has to be solved for the visibility icon.
I don't want to of course ignore the accessibility issue, I'll try to find a solution for that. Let's forget the accessibility issue now, do you agree with this change despite it lacks on discoverability? I'm asking because I would like to push this ASAP because it's going to be message freeze soon for Plasma 5.5 and I don't know whether I'll be able or have enough time to find solution for the accessibility issue till the message freeze. - Jan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125723/#review87177 ----------------------------------------------------------- On Říj. 21, 2015, 6:12 dop., Jan Grulich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125723/ > ----------------------------------------------------------- > > (Updated Říj. 21, 2015, 6:12 dop.) > > > Review request for Network Management, KDE Usability and Lamarque Souza. > > > Bugs: 340707 > http://bugs.kde.org/show_bug.cgi?id=340707 > > > Repository: plasma-nm > > > Description > ------- > > I took again inspiration from nm-connection-editor and extended our editor to > allow to users select password storage. Now each password field has an option > to either store password to all users (store it to NM) or to current user > (store it to KWallet) or do not store it at all (user will be prompted) or > tell NM that the password is not required at all (available only for some > connection types). Also during transformation of strongswan vpn plugin I > found out that strongswan shouldn't support password storing at all > (according to NM plugin) so I removed it accordingly. > > + this patch also tries to fix couple of coding style issues as usual > > Screenshot: >  > > > Diffs > ----- > > libs/editor/connectiondetaileditor.cpp aa09d1f > libs/editor/settings/bondwidget.h 2c64c31 > libs/editor/settings/bondwidget.cpp dbd2cf2 > libs/editor/settings/bridgewidget.h a322083 > libs/editor/settings/bridgewidget.cpp 3dc1889 > libs/editor/settings/btwidget.h 84a8f71 > libs/editor/settings/btwidget.cpp e2e9972 > libs/editor/settings/cdmawidget.h bbffe0d > libs/editor/settings/cdmawidget.cpp f361d9e > libs/editor/settings/gsmwidget.h 69d4cfd > libs/editor/settings/gsmwidget.cpp 941f411 > libs/editor/settings/infinibandwidget.h 2c90b7f > libs/editor/settings/infinibandwidget.cpp 55dfb97 > libs/editor/settings/ipv4widget.h a993211 > libs/editor/settings/ipv4widget.cpp d7e893f > libs/editor/settings/ipv6widget.h 4a1f472 > libs/editor/settings/ipv6widget.cpp 62e96ff > libs/editor/settings/pppoewidget.h 4570bba > libs/editor/settings/pppoewidget.cpp 0fa4ea2 > libs/editor/settings/pppwidget.h 0c088ce > libs/editor/settings/pppwidget.cpp 7185b7d > libs/editor/settings/security802-1x.h cb5dbec > libs/editor/settings/security802-1x.cpp 21929b8 > libs/editor/settings/teamwidget.h 39821c2 > libs/editor/settings/teamwidget.cpp 3db2b7a > libs/editor/settings/ui/802-1x.ui f1bc058 > libs/editor/settings/ui/cdma.ui 9805021 > libs/editor/settings/ui/gsm.ui 87891cc > libs/editor/settings/vlanwidget.h fabeaca > libs/editor/settings/vlanwidget.cpp 5e083fa > libs/editor/settings/wificonnectionwidget.h 35a59d3 > libs/editor/settings/wificonnectionwidget.cpp d35ec45 > libs/editor/settings/wifisecurity.h a263c32 > libs/editor/settings/wifisecurity.cpp 4d9c811 > libs/editor/settings/wimaxwidget.h c8f850d > libs/editor/settings/wimaxwidget.cpp dcd212c > libs/editor/settings/wiredconnectionwidget.h ed8dc88 > libs/editor/settings/wiredconnectionwidget.cpp d8fcd90 > libs/editor/settings/wiredsecurity.h a34731e > libs/editor/settings/wiredsecurity.cpp 8fdb1cc > libs/editor/widgets/passwordfield.h 1bd21a2 > libs/editor/widgets/passwordfield.cpp 05fe6dc > libs/editor/widgets/settingwidget.h fdae197 > vpn/l2tp/l2tp.ui b04ebce > vpn/l2tp/l2tpauth.h 54b6462 > vpn/l2tp/l2tpauth.cpp 7369458 > vpn/l2tp/l2tpwidget.h 1e4c383 > vpn/l2tp/l2tpwidget.cpp b278228 > vpn/openconnect/openconnectauth.h 9a77421 > vpn/openconnect/openconnectauth.cpp fecb16e > vpn/openconnect/openconnectwidget.h ae0e9d2 > vpn/openconnect/openconnectwidget.cpp c293e52 > vpn/openswan/openswan.ui a6d61fa > vpn/openswan/openswanauth.h aa78f88 > vpn/openswan/openswanauth.cpp 1fd79fb > vpn/openswan/openswanwidget.h 55a54dd > vpn/openswan/openswanwidget.cpp b94f752 > vpn/openvpn/openvpn.ui 1c49cad > vpn/openvpn/openvpnadvanced.ui ce89ce1 > vpn/openvpn/openvpnadvancedwidget.h c0346ee > vpn/openvpn/openvpnadvancedwidget.cpp 0864e35 > vpn/openvpn/openvpnauth.h dd3b564 > vpn/openvpn/openvpnauth.cpp 5250bc1 > vpn/openvpn/openvpnwidget.h 51d7aab > vpn/openvpn/openvpnwidget.cpp e27e216 > vpn/pptp/pptpauth.h 1772c81 > vpn/pptp/pptpauth.cpp 8ffec08 > vpn/pptp/pptpprop.ui c713f24 > vpn/pptp/pptpwidget.h 2b25dd7 > vpn/pptp/pptpwidget.cpp 880d6c5 > vpn/ssh/sshauth.h 88dcebc > vpn/ssh/sshauth.cpp 6e8ffa9 > vpn/ssh/sshwidget.h fa3f852 > vpn/ssh/sshwidget.cpp 7b3ad28 > vpn/ssh/sshwidget.ui 1d0300b > vpn/sstp/sstpauth.h 08f5025 > vpn/sstp/sstpauth.cpp b452a73 > vpn/sstp/sstpwidget.h 0156b86 > vpn/sstp/sstpwidget.cpp 7ff68aa > vpn/sstp/sstpwidget.ui 51dfc96 > vpn/strongswan/strongswanauth.h d23947d > vpn/strongswan/strongswanauth.cpp f034abe > vpn/strongswan/strongswanprop.ui 06fb254 > vpn/strongswan/strongswanwidget.h 7204ff5 > vpn/strongswan/strongswanwidget.cpp 094e81e > vpn/vpnc/vpnc.ui fc83bf8 > vpn/vpnc/vpncauth.h eb24744 > vpn/vpnc/vpncauth.cpp 9142682 > vpn/vpnc/vpncwidget.h 9aa4ac8 > vpn/vpnc/vpncwidget.cpp 6aaffda > > Diff: https://git.reviewboard.kde.org/r/125723/diff/ > > > Testing > ------- > > > Thanks, > > Jan Grulich > >
_______________________________________________ kde-networkmanager mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-networkmanager
