As pubsub uses extensively data forms (XEP-0004), I refactored some data forms code from gabble and moved it to a separated object. I focused on the form parsing and submit part as that's what I'll need for pubsub. http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/dataforms
While writing unit tests for this component in Wocky I fixed some bugs. Maybe this branch should be reviewed at the same time as the Gabble one as that's really the same code: http://git.collabora.co.uk/?p=user/cassidy/wocky;a=shortlog;h=refs/heads/dataforms
Taking a look.
Partial review up to 8fc141547d96a1ca69: wocky_data_forms_field and wocky_data_forms_field_option should be camel-cased. extract_value_list() doesn't NULL-terminate the returned value. This suggests that that code path isn't tested. :) + DEBUG ("Wrong field type for '%s': %s", type_str, var); It's not a wrong type, it's an invalid type. + DEBUG ("No option provided for '%s'", var); Options. Why does wocky_data_forms_field_new() take ownership of the 'options' list, but not of (eg) the default value? + if (value== NULL) Style: add a space. extract_search_keys_from_froms(): The last word should be "forms" :) + "server doesn't support any known search key"); keys plural. + /* list will be reveresed */ reversed. + if (!wocky_strdiff (value, "true")) + return wocky_g_value_slice_new_boolean (TRUE); + else if (!wocky_strdiff (value, "false")) + return wocky_g_value_slice_new_boolean (FALSE); + else + DEBUG ("Invalid boolean value: %s", value); + break; From the footnote assigned to the boolean type in XEP-0004 §3.3: > 10. In accordance with Section 3.2.2.1 of XML Schema Part 2: > Datatypes, the allowable lexical representations for the xs:boolean > datatype are the strings "0" and "false" for the concept 'false' and > the strings "1" and "true" for the concept 'true'; implementations > MUST support both styles of lexical representation. Amusingly, this branch produces "0" and "1", but only accepts "false" and "true". :) The name of wocky_data_forms_submit() seems a bit misleading. It doesn't actually submit the form. Obviously it doing so would be strange... Maybe it's worth the misleading name for brevity's sake?
(In reply to comment #3) > Partial review up to 8fc141547d96a1ca69: > > wocky_data_forms_field and wocky_data_forms_field_option should be > camel-cased. done > extract_value_list() doesn't NULL-terminate the returned value. This > suggests that that code path isn't tested. :) Fixed in the Wocky branch (and tested!) :) > + DEBUG ("Wrong field type for '%s': %s", type_str, var); > > It's not a wrong type, it's an invalid type. fixed. > + DEBUG ("No option provided for '%s'", var); > > Options. fixed. > Why does wocky_data_forms_field_new() take ownership of the 'options' > list, but not of (eg) the default value? No good reason; fixed. > + if (value== NULL) > > Style: add a space. already fixed. > extract_search_keys_from_froms(): The last word should be "forms" :) fixed. > + "server doesn't support any known search key"); > > keys plural. fixed. > + /* list will be reveresed */ > > reversed. fixed. > + if (!wocky_strdiff (value, "true")) > + return wocky_g_value_slice_new_boolean (TRUE); > + else if (!wocky_strdiff (value, "false")) > + return wocky_g_value_slice_new_boolean (FALSE); > + else > + DEBUG ("Invalid boolean value: %s", value); > + break; > > From the footnote assigned to the boolean type in XEP-0004 §3.3: > > > 10. In accordance with Section 3.2.2.1 of XML Schema Part 2: > > Datatypes, the allowable lexical representations for the xs:boolean > > datatype are the strings "0" and "false" for the concept 'false' and > > the strings "1" and "true" for the concept 'true'; implementations > > MUST support both styles of lexical representation. > > Amusingly, this branch produces "0" and "1", but only accepts "false" > and "true". :) Oooops, nice catch. Fixed. :) > The name of wocky_data_forms_submit() seems a bit misleading. It > doesn't actually submit the form. Obviously it doing so would be > strange... Maybe it's worth the misleading name for brevity's sake? > Don't know. IMHO the current name isn't that bad but I'm open to suggestions.
I've rebased your branch, reviewed it, and fixed the things I would have complained about. :) It's mostly just tidying and a bit of refactoring; review would be lovely!
Looks all good to me!
review+ from cassidy, and I think this got merged? If so, please close it.
Yep.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.