GNOME Bugzilla – Bug 683103
schema listing is incorrect
Last modified: 2013-07-03 17:40:49 UTC
Created attachment 223061 [details] [review] proposed patch authentication-methods is listed as type "as", and but the value is stored as string. I have attached the patch to fix this issue. On a side note, I did notice we use two methods - none, and vnc. I do not see a purpose for using "as" instead of "s", until unless we plan to extend this to say spice/rdp/... -- ritz
This patch is obviously wrong. <choices> lists the strings that can occur in an "s" / "as" / "aas" / ... value; a <choice> it's not supposed to store a list itself. If the schema compiler accepts this input, I think that should be filed as a bug.
from http://developer.gnome.org/gio/2.32/GSettings.html <!-- choices is only allowed for keys with string or string array type --> <!ELEMENT choices (choice+) > <!-- each choice element specifies one possible value --> <!ELEMENT choice EMPTY > <!ATTLIST choice value CDATA #REQUIRED > my xml foo is weak, but as per my understanding this does not stop us from adding string array. and the below $ glib-compile-schemas --dry-run . warning: Schema 'org.gnome.Vino' has path '/desktop/gnome/remote-access/'. Paths starting with '/apps/', '/desktop/' or '/system/' are deprecated. ./org.gnome.Vino.gschema.xml: Error on line 126 char 1: <enum id='org.gnome.Vino.VinoIconVisibility'> not (yet) defined.. This entire file has been ignored.
It seems a bit silly that 'none' is an option at all here. If the intention is that enabling multiple authentication mechanisms should be supported (but also having none enabled) then the way to express none enabled should be [] (the empty list), not ['none'].
* The way to set this up would be to use the command below $ gsettings set org.gnome.Vino authentication-methods "['vnc']" * Given that "choices" is designed to for strings, and we use single auth ( I do not forsee vnc+rdp , or is this open for implementation ? ) from capplet/vino-preferences.c 93 static GVariant * 94 set_authtype (const GValue *value, 95 const GVariantType *type, 96 gpointer user_data) 97 { 98 const gchar *authtype; 99 100 if (g_value_get_boolean (value)) 101 authtype = "vnc"; 102 else 103 authtype = "none"; 104 105 return g_variant_new_strv (&authtype, 1); 106 } * I would assume, it would be a better choice to use "s" rather than "as" to store authtype. from common/org.gnome.Vino.gschema.xml 75 <key name='authentication-methods' type='as'> ---------------------------------------------------------- * Alternatively, we could remove the choices to allow manual input or implement the currently attached patch which works with the current dconf-editor/client. any thoughts ? Thank you.
(In reply to comment #4) > * I would assume, it would be a better choice to use "s" rather than "as" to > store authtype. > > any thoughts ? Thank you. As VNC is likely to be the only supported authentication method, just adding a new boolean key "vnc-auth-enable" should be fine.
Since it looks like we have a possible solution could we get this bug confirmed and a possible ETA when we could expect this to be committed? Thanks
Comment on attachment 223061 [details] [review] proposed patch (In reply to comment #6) > Since it looks like we have a possible solution could we get this bug confirmed > and a possible ETA when we could expect this to be committed? Confirming is not really used much on GNOME Bugzilla, but I set the status to NEW. ETA is whenever someone comes up with a suitable patch. I do have time at the moment.
(In reply to comment #7) That should have read “I do _not_ have time at the moment.”
(In reply to comment #8) > (In reply to comment #7) > That should have read “I do _not_ have time at the moment.” No worries, but, are we under the agreement that adding a boolean for vnc-auth-enable is the correct approach? If so, I can begin working on a patch for that. Thanks David
Created attachment 238907 [details] [review] proposed patch update patchset, uses vnc-auth boolean var to track auth method. This deprecates authentication-method.
Comment on attachment 238907 [details] [review] proposed patch I went with the simpler approach of removing the choices for the "authentication-methods" key in commit 130d6a4a1f4f6e90586d42903484e5899459c403.
Created attachment 239387 [details] [review] proposed patch probably not important, but this will fix the description string to match the new behaviour.
Comment on attachment 239387 [details] [review] proposed patch This will have to be applied after the freeze.
that commit seems slightly buggy, with it (applied on 3.6 for raring) I get that warning: "vino_server_set_auth_methods: assertion `auth_methods != VINO_AUTH_INVALID' failed" $ gsettings get org.gnome.Vino authentication-methods @as []
(In reply to comment #14) Ah, thanks for the report. I missed that the "none" string was required (rather than an empty list) to match the flag value generated with glib-mkenums from the VinoAuthType enum. The warning should be harmless for the moment, and after the freeze I will revert the code changes in the patch, and update the schema changes.
Thanks David, if you have a patch ready, can you add it to the bug? Ritz is trying to get that fix SRUed to Ubuntu precise for some Canonical customer request
Created attachment 239550 [details] [review] proposed patch Hi @David I assume, you would drop the change to set_auth capplet/vino-preferences.c, and drop choices listing from common/org.gnome.Vino.gschema.xml.in . TIA -- ritz
Comment on attachment 239550 [details] [review] proposed patch I pushed a different patch to master and gnome-3-8, as commits b22249a387c2e6a80a1d7fee256dc14b71b70235 and 6b6349b204d8b4e576ba8f682d201a8de656945d, respectively.
Comment on attachment 239387 [details] [review] proposed patch No longer needed.
Hi David this patch results in a warning message, if the user inputs an incorrect value into the field. $ vino ... "(vino-server:20654): GLib-GIO-WARNING **: Unable to lookup flags nick 'one' via GType" I believe attachment# 238907 [details] is a more appropriate fix. Cheers -- ritz
(In reply to comment #20) > this patch results in a warning message, if the user inputs an incorrect > value into the field. > > "(vino-server:20654): GLib-GIO-WARNING **: Unable to lookup flags nick 'one' > via GType" Right, it is intended that bad input generates warnings. > I believe attachment# 238907 [details] is a more appropriate fix. No, it is fine as is.