Created attachment 329078 [details] [review]
wireless-security: Make broken configuration entries red

When a configuration setting is wrong, set the entry or file chooser
that contains the incorrect information to be surrounded by red.

This makes it easier for users to find where the error was made that
disallows them to click the "Apply" button.

See https://bugzilla.gnome.org/show_bug.cgi?id=734446
and https://bugzilla.gnome.org/show_bug.cgi?id=734472

Review of attachment 329078 [details] [review]:

Is it expected that the red border is visible only when using GNOME3
and not for example with MATE?

::: src/wireless-security/eap-method-simple.c
@@ +80,3 @@
 	/* Check if the password should always be requested */
+	if (always_ask_selected (method->password_entry)) {
+		widget_unset_error (method->password_entry);

widget_unset_error (GTK_WIDGET (method->password_entry)); ?

::: src/wireless-security/ws-wpa-psk.c
@@ +67,3 @@
 	len = key ? strlen (key) : 0;
 	if ((len < 8) || (len > 64)) {
+		widget_set_error (entry);

I'm not sure why, but it seems this makes the psk disappear when invalid on MATE.

(In reply to Beniamino Galvani from comment #2)
> Review of attachment 329078 [details] [review] [review]:
> 
> Is it expected that the red border is visible only when using GNOME3
> and not for example with MATE?

Because its theme doesn't implement the feature?

> ::: src/wireless-security/eap-method-simple.c
> @@ +80,3 @@
>  	/* Check if the password should always be requested */
> +	if (always_ask_selected (method->password_entry)) {
> +		widget_unset_error (method->password_entry);
> 
> widget_unset_error (GTK_WIDGET (method->password_entry)); ?
> 
> ::: src/wireless-security/ws-wpa-psk.c
> @@ +67,3 @@
>  	len = key ? strlen (key) : 0;
>  	if ((len < 8) || (len > 64)) {
> +		widget_set_error (entry);
> 
> I'm not sure why, but it seems this makes the psk disappear when invalid on
> MATE.

I don't see how that's possible, unless there's a bug in the theme MATE uses.

Created attachment 329179 [details] [review]
wireless-security: Make broken configuration entries red

When a configuration setting is wrong, set the entry or file chooser
that contains the incorrect information to be surrounded by red.

This makes it easier for users to find where the error was made that
disallows them to click the "Apply" button.

See https://bugzilla.gnome.org/show_bug.cgi?id=734446
See https://bugzilla.gnome.org/show_bug.cgi?id=734472

Fixed a warning in the filepicker validation, and made the filepicker validation better. I've also made sure to clear errors on widgets when disabling the 8021x security.

I'm not sure that I caught all the possible error cases, but I'd really like to land this, and fix specific bugs later on. I also haven't added any support for connection-editor/ validation, and I don't plan to in the short term either.

Patch lgtm and tests good.


I pushed the patch + 2 fixup commits to ce-highlight-invalid-bgo767210 branch.

https://git.gnome.org/browse/network-manager-applet/log/?h=ce-highlight-invalid-bgo767210



ACK?

Not sure about the changes in eap-method.c but if you've tested them, looks fine to me.

I don't see Beniaminos issue with the psk field on KDE (comment 2).


Merged patch+fixups to master:
https://git.gnome.org/browse/network-manager-applet/commit/?id=950c14dd71e9e9d6b9644cdbd7a148a34c2bea79