After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699849 - [review] dcbw/onex-hide-security-combo
[review] dcbw/onex-hide-security-combo
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: nm-applet
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-05-07 16:12 UTC by Dan Williams
Modified: 2013-05-25 00:19 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Dan Williams 2013-05-07 16:12:54 UTC
When connecting to a WPA Enterprise network for the first time, the "Security" combo box is show, but it should be hidden because the security is already known for most common configurations.

It only needs to be shown when the AP only supports WEP, because the AP's details are not enough to distinguish WEP from Dynamic WEP.
Comment 1 Dan Winship 2013-05-08 13:14:37 UTC
>+	s_wifi = nm_connection_get_setting_wireless (specific_connection);
>+	s_wsec = nm_connection_get_setting_wireless_security (specific_connection);
>+	if (nm_setting_wireless_get_security (s_wifi) || s_wsec) {
>+		const char *key_mgmt = nm_setting_wireless_security_get_key_mgmt (s_wsec);

Taken at face value, it looks like you could end up calling nm_setting_wireless_security_get_key_mgmt() on NULL here. Maybe you meant "&&" rather than "||"? But if :security is set and there's no s_wsec, then the connection wouldn't have verified anyway...

I want to say you can ignore s_wifi entirely and just check if s_wsec is present, but I guess NMSettingWireless doesn't verify that there *isn't* an NMSettingWirelessSecurity if :security *isn't* set... (But probably it should?)

Anyway, I'd be fine with just changing || to && there. And everything else looks right.
Comment 2 Dan Williams 2013-05-14 14:55:24 UTC
Changed that section to:

const char *key_mgmt = NULL;
...
...
/* We need to show the security combo for WEP/Dynamic-WEP cases because
 * that can't be autodetected.  Open/WPA-PSK/WPA-Enterprise can
 * be autodetected and so we shouldn't show the user stuff they
 * don't need to change.
 */
s_wsec = nm_connection_get_setting_wireless_security (specific_connection);
if (s_wsec)
	key_mgmt = nm_setting_wireless_security_get_key_mgmt (s_wsec);
if (g_strcmp0 (key_mgmt, "none") != 0 && g_strcmp0 (key_mgmt, "ieee8021x") != 0)
	hide_security_combo = TRUE;
Comment 3 Dan Winship 2013-05-14 15:06:42 UTC
sure, that works
Comment 4 Dan Williams 2013-05-25 00:19:09 UTC
Merged.