Re: bonobo-ui-node re-write ...

[Darin Adler <darin bentspoon com>]

I spent a few minutes looking over the code. Here are some review comments 
that you might find helpful.

Did you do any timings to see if it's faster, or are you expecting other 
people to do that? (Nice luxury if you can get it.)

It seems that you've reinvented GQuark here. You should use GQuarks for 
the IDs, rather than having the additional complexity of your own hash 
tables. I'd suggest calling g_quark_from_string instead of 
bonobo_ui_node_get_id (or as the implementation of bonobo_ui_node_get_id), 
and g_quark_to_string instead of bonobo_ui_node_str_from_id (or as the 
implementation of bonobo_ui_node_str_from_id). For clarity I'd also change 
the parameters to be of type GQuark instead of guint32. This even allows a 
bit of additional optimization by using g_quark_from_static_string when 
appropriate.

Overall, the change seems like a great idea. It does seem gratuitous to 
make the code rely on the BonoboUINode structure rather than continuing to 
use function calls to get things like next, previous, and parent. If that 
change results in a measurable speedup, then I guess it's worth it, but I 
doubt it does. Keeping the abstraction if it's not prohibitively expensive 
might make it easier to make a future change in this area.

> +	if (!(txt = bonobo_ui_node_get_attr_by_id (node, id_id))) {
>
> +		txt = bonobo_ui_node_get_attr_by_id (node, verb_id);
>
> +		if (txt && txt [0] == '\0')
> +			txt = bonobo_ui_node_get_attr_by_id (node, name_id);
> +	}

Looks to me like the above code is missing a ! in the if statement.

> -	hidden = bonobo_ui_node_get_attr (node, "hidden");
> +	hidden = bonobo_ui_node_get_attr_by_id (node, hidden_id);
>  	if (hidden && atoi (hidden))

I think there's a faster way to check for non-zero than calling atoi. But 
perhaps this is not a bottleneck.

> +static void
> +cleanhash ()

You should put a (void) in there, instead of ().

> +	if (str_to_id_hash) {
> +		g_hash_table_foreach (str_to_id_hash, (GHFunc) g_free, NULL);
> +		g_hash_table_destroy (str_to_id_hash);
> +		g_hash_table_destroy (id_to_str_hash);
> +	}

Why the if statement here?

>  bonobo_ui_node_get_attr (BonoboUINode *node,
>                           const char   *name)
>  {
> -        return xmlGetProp (XML_NODE (node), name);
> +	const char *val;
> +
> +	val = bonobo_ui_node_get_attr_by_id (
> +		node, bonobo_ui_node_get_id (name));
> +
> +	return val ? g_strdup (val) : NULL;
>  }

You don't need the ? : here. All strdup functions (g_strdup, xmlStrdup) in 
GNOME are designed to work on NULL too. So you can just do:

    return g_strdup (val);

>  char *
>  bonobo_ui_node_get_content (BonoboUINode *node)
>  {
> -        return xmlNodeGetContent (XML_NODE (node));
> +	const char *content = bonobo_ui_node_peek_content (node);
> +
> +	return content ? xmlStrdup (content) : NULL;
>  }

You don't need the ? : here. All strdup functions (g_strdup, xmlStrdup) in 
GNOME are designed to work on NULL too. So you can just do:

    return xmlStrdup (bonobo_ui_node_peek_content (node));

> +	for (i = 0; i < src->attrs->len; i++) {
> +		BonoboUIAttr *as = &attr (src, i);
> +		BonoboUIAttr *ad = &attr (dest, i);
> +
> +		ad->id    = as->id;
> +		ad->value = as->value ? xmlStrdup (as->value) : NULL;

Another opportunity to avoid a ? :

> +static void
> +uiCharacters (ParseState *ps, const xmlChar *chars, int len)
> +{
> +    int i;
> +
> +    for (i = 0; i < len; i++)
> +	    g_string_append_c (ps->content, chars [i]);
> +}

Using

> +	g_return_val_if_fail (xml != NULL, NULL);
>
> +	len = strlen (xml);
> +	if (len < 4)
>  		return NULL;

Why a warning for NULL, a warning for XML that's not well formed, but no 
warning for a too-short string? You should warn here too, I think. Is 
there a reason you need this check? Won't xmlSAXUserParseMemory detect the 
error? Is there a reason to optimize the case of passing in a bad string? 
Does that happen enough to affect speed?

> +	contains = node->content || (node->children && recurse);

Why put this in a local variable? You should just put this test in the if 
statement below.

    -- Darin