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 754832 - [review] [nm-applet] propagate and print validation error [th/validation-error-bgo754832]
[review] [nm-applet] propagate and print validation error [th/validation-erro...
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: 2015-09-10 13:14 UTC by Thomas Haller
Modified: 2015-09-12 11:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2015-09-10 13:14:57 UTC 
QUOTE:


    editor: forward the validation error to print more sensible error message to stdout
    
    When a connection doesn't verify, the "Save" dialog stays desensitized.
    Even for an advanced user it's not immediately clear what causes the
    valdation failure.
    
    We already print a message like "Invalid setting Ethernet" to stdout.
    Extend the message to also print an more detailed error that gets propagated
    from the validation.
    
    This later should be improved further to indicating the failure reason
    in the UI too.
Comment 1 Thomas Haller 2015-09-10 16:39:29 UTC 
did more changes there...

now the error message is shown as tooltip for the disabled "Save" button
Comment 2 Dan Williams 2015-09-10 17:02:58 UTC 
First, I like this solution to the problem.

> c-e: fix icon and tooltip of "Save" button regarding authorization

 	priv->authorized = (result == NM_CLIENT_PERMISSION_RESULT_YES || result == NM_CLIENT_PERMISSION_RESULT_AUTH);
+	priv->authorized_asks = priv->authorized_asks && (result == NM_CLIENT_PERMISSION_RESULT_AUTH);

Why does this check the existing value?  Since this is the only place it gets set, it will never be TRUE.


> editor: forward the validation error to print more sensible error message to stdout

This has a different prefix than the others; in the past I think we've just used 'editor', but the other commits use 'c-e:'.  Should pick one.

+	if (CE_PAGE_GET_CLASS (self)->ce_page_validate_v) {
+		if (!CE_PAGE_GET_CLASS (self)->ce_page_validate_v (self, connection, error)) {
+			if (error && !error)
+				g_set_error_literal (error, NMA_ERROR, NMA_ERROR_GENERIC, _("unspecified error"));
+			return FALSE;
+		}
+	}

That g_set_error_literal() will never be run.  I suspect you want 'error && !*error'?

Should ce_page_mac_entry_valid() take a 'detail' and set the error?  Seems like that would be simpler.  Same for nm_utils_iface_valid_name(); we should have a common function that calls that, and returns a generic error.  And ce_page_device_entry_get().

EAP methods: don't use "wpa-eap" here, since this is also used for wired 802.1x.  I'd just use the name of the EAP method instead, and it should be capitalized eg EAP-FAST or EAP-TTLS.  Since the errors are translated now, nobody should be parsing them, and I think we should use more human-readable names instead of internal property names.  These are UI now.

eap-method-fast.c: should be "missing EAP-FAST PAC file"

eap-method-leap.c: "missing EAP-LEAP [username|password]"

eap-method-peap.c: "invalid EAP-PEAP CA certificate: %s"

eap-method-simple.c: "missing EAP [username|password]" - because this method is used for all the simple internal methods (CHAP, PAP, MSCHAPv2, etc) and "simple" is an internal-only name, I don't think we should let that leak out.

anyway, you get the idea...
Comment 3 Thomas Haller 2015-09-11 08:21:30 UTC 
(In reply to Dan Williams from comment #2)
> First, I like this solution to the problem.
> 
> > c-e: fix icon and tooltip of "Save" button regarding authorization
> 

done all. See fixup commits.
Comment 4 Dan Williams 2015-09-11 14:33:32 UTC 
Looks good except for the fixup that changes the EAP tooltips; in eap-method-peap.c there is "invalid EAP-PEAL CA ertificate: no certificate specified" and that should be PEAP not PEAL.

Operation looks good to me...