Bug 24146

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.