Discussion:
Review Request 125723: Add options to select password storage to all password fields
Jan Grulich
2015-10-20 11:29:43 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------

Review request for Network Management 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:
![Screenshot of new options](Loading Image...)


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
Lamarque Souza
2015-10-20 17:27:23 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87134
-----------------------------------------------------------



libs/editor/widgets/passwordfield.h (line 32)
<https://git.reviewboard.kde.org/r/125723/#comment59873>

In Plasma NM 0.9.0.x we used to have an option to store secrets for a list of users. Has not that option been ported to Plasma NM >= 0.9.3.x?



libs/editor/widgets/passwordfield.h (line 40)
<https://git.reviewboard.kde.org/r/125723/#comment59881>

I think this should be renamed to setPasswordOptionsEnabled, like what is done in Qt.



libs/editor/widgets/passwordfield.h (line 41)
<https://git.reviewboard.kde.org/r/125723/#comment59883>

This sounds really wierd. Maybe you can join this and the method above into one:

setOptionEnabled(PasswordOption option, bool enable);

That would make the API cleaner.



libs/editor/widgets/passwordfield.h (line 54)
<https://git.reviewboard.kde.org/r/125723/#comment59887>

We usually use a m_ prefix before private variables.



libs/editor/widgets/passwordfield.cpp (line 40)
<https://git.reviewboard.kde.org/r/125723/#comment59884>

"Store password for this user only"



libs/editor/widgets/passwordfield.cpp (line 43)
<https://git.reviewboard.kde.org/r/125723/#comment59885>

"Store password for this user only"



libs/editor/widgets/passwordfield.cpp (line 49)
<https://git.reviewboard.kde.org/r/125723/#comment59886>

Well, the password is not stored for all users, it is stored once and is available to all users. I am not able to find a short and accurate description for that. Is "Store password and make it available to all users" ok here?



libs/editor/widgets/passwordfield.cpp (line 77)
<https://git.reviewboard.kde.org/r/125723/#comment59888>

What is "the position we want"? I mean, why remove and then readd the action?


- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 11:29 a.m.)
Review request for Network Management 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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 06:08:09 UTC
Permalink
Post by Jan Grulich
libs/editor/widgets/passwordfield.h, line 32
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line32>
In Plasma NM 0.9.0.x we used to have an option to store secrets for a list of users. Has not that option been ported to Plasma NM >= 0.9.3.x?
Nope and I don't even know that there was such option. I also don't know how to store secrets for a list of users because you don't have access to their KWallet and if you store secrets to NM then they will be available for everyone.
Post by Jan Grulich
libs/editor/widgets/passwordfield.h, line 40
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line40>
I think this should be renamed to setPasswordOptionsEnabled, like what is done in Qt.
Right, I had before enablePasswordOptions() and then I decided to make it as setter and just blindly add "set" at the beginning without thinking :).
Post by Jan Grulich
libs/editor/widgets/passwordfield.h, line 41
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line41>
setOptionEnabled(PasswordOption option, bool enable);
That would make the API cleaner.
The first one enables all options, the second enables just certain option because not requiring password doesn't make sense in some cases.
Post by Jan Grulich
libs/editor/widgets/passwordfield.cpp, line 49
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411858#file411858line49>
Well, the password is not stored for all users, it is stored once and is available to all users. I am not able to find a short and accurate description for that. Is "Store password and make it available to all users" ok here?
Yes, seems to be ok.
Post by Jan Grulich
libs/editor/widgets/passwordfield.cpp, line 77
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411858#file411858line77>
What is "the position we want"? I mean, why remove and then readd the action?
We want it (the action with password options represented by icon) to be on the right side right behind the "show password action" which is not if it's added additionally so we have to remove the existing action first. I tried to just make the action with password options hidden or disabled and be there by default but it's visible all the time.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87134
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Říj. 20, 2015, 11:29 dop.)
Review request for Network Management 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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Lamarque Souza
2015-10-21 09:58:12 UTC
Permalink
Post by Jan Grulich
libs/editor/widgets/passwordfield.h, line 32
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line32>
In Plasma NM 0.9.0.x we used to have an option to store secrets for a list of users. Has not that option been ported to Plasma NM >= 0.9.3.x?
Nope and I don't even know that there was such option. I also don't know how to store secrets for a list of users because you don't have access to their KWallet and if you store secrets to NM then they will be available for everyone.
NM_SETTING_CONNECTION_PERMISSIONS is a list of the pair (user, permissions). The "Store the password only for this user" is the case where the list contains just one user, but it can contains as much as we want (or NM can handle). That was implemented in 0.9.0.x, take a look at networkmanagement/libs/ui/advancedpermissionswidget.cpp. NMQt already handles NM_SETTING_CONNECTION_PERMISSIONS as a list of the pair (user, permission).
Post by Jan Grulich
libs/editor/widgets/passwordfield.cpp, line 77
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411858#file411858line77>
What is "the position we want"? I mean, why remove and then readd the action?
We want it (the action with password options represented by icon) to be on the right side right behind the "show password action" which is not if it's added additionally so we have to remove the existing action first. I tried to just make the action with password options hidden or disabled and be there by default but it's visible all the time.
I got it. You could change this comment to "Remove the "show password action" first so we can add the following actions in the correct order".


- Lamarque


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87134
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 21, 2015, 6:12 a.m.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 10:39:46 UTC
Permalink
Post by Jan Grulich
libs/editor/widgets/passwordfield.h, line 32
<https://git.reviewboard.kde.org/r/125723/diff/1/?file=411857#file411857line32>
In Plasma NM 0.9.0.x we used to have an option to store secrets for a list of users. Has not that option been ported to Plasma NM >= 0.9.3.x?
Nope and I don't even know that there was such option. I also don't know how to store secrets for a list of users because you don't have access to their KWallet and if you store secrets to NM then they will be available for everyone.
NM_SETTING_CONNECTION_PERMISSIONS is a list of the pair (user, permissions). The "Store the password only for this user" is the case where the list contains just one user, but it can contains as much as we want (or NM can handle). That was implemented in 0.9.0.x, take a look at networkmanagement/libs/ui/advancedpermissionswidget.cpp. NMQt already handles NM_SETTING_CONNECTION_PERMISSIONS as a list of the pair (user, permission).
Well, we have this implemented even in plasma-nm, but this doesn't affect only secrets. If a certain user is not present in that list then he won't see that connection available at all and I don't think this is what we want.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87134
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Lamarque Souza
2015-10-20 17:30:07 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87150
-----------------------------------------------------------


Another thought. The combox was well visible, the icon in password field is not that visible. Wouldn't that impact people with bad eyesight?

- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 11:29 a.m.)
Review request for Network Management 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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 06:12:21 UTC
Permalink
Post by Jan Grulich
Post by Lamarque Souza
Another thought. The combox was well visible, the icon in password field is not that visible. Wouldn't that impact people with bad eyesight?
Maybe, but the combobox wasn't there for all password fields, it was used mainly for VPN connections. I'll add usability group to be sure it's okay. If not, we can just add the combobox instead of the action and also make it as part of the password field.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87150
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Říj. 21, 2015, 6:08 dop.)
Review request for Network Management 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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 06:08:25 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------

(Updated Říj. 21, 2015, 6:08 dop.)


Review request for Network Management and Lamarque Souza.


Changes
-------

Fixed mentioned issues.


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:
![Screenshot of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)


Diffs (updated)
-----

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
Heiko Tietze
2015-10-21 07:41:31 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------


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.

- Heiko Tietze
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Okt. 21, 2015, 6:12 vorm.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 08:06:35 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Lamarque Souza
2015-10-21 09:58:16 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.


- Lamarque


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 21, 2015, 6:12 a.m.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 10:55:36 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Thomas Pfeiffer
2015-10-21 11:24:29 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 21, 2015, 6:12 a.m.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 11:34:43 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 11:40:01 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
Updated screenshot:
![Image description](Loading Image...)


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Heiko Tietze
2015-10-21 11:50:16 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
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.


- Heiko


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Okt. 21, 2015, 6:12 vorm.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Thomas Pfeiffer
2015-10-21 13:34:46 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
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.
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?


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 21, 2015, 6:12 a.m.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-21 17:44:47 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
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.
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?
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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-10-29 12:09:44 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
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.
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?
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.
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?


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Heiko Tietze
2015-10-29 12:57:53 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
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.
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?
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.
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?
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.


- Heiko


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87177
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Oct. 21, 2015, 6:12 a.m.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-11-03 09:17:50 UTC
Permalink
Post by Jan Grulich
Post by Heiko Tietze
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.
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.
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.
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 :).
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"
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.
![Image description](https://jgrulich.fedorapeople.org/nm-password-options2.png)
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.
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?
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.
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?
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
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Heiko Tietze
2015-11-03 09:46:08 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/#review87909
-----------------------------------------------------------

Ship it!


Ship It!

- Heiko Tietze
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------
(Updated Okt. 21, 2015, 6:12 vorm.)
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 of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)
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
Jan Grulich
2015-11-03 12:41:19 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125723/
-----------------------------------------------------------

(Updated Nov. 3, 2015, 12:41 p.m.)


Status
------

This change has been marked as submitted.


Review request for Network Management, KDE Usability and Lamarque Souza.


Changes
-------

Submitted with commit 5b8db6cf2363906e25b4e033100b8da7ac3a9aa4 by Jan Grulich to branch master.


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:
![Screenshot of new options](https://jgrulich.fedorapeople.org/nm-password-options.png)


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

Loading...