Re: [Nautilus-list] [PATCH] Custom icon patch

[Darin Adler <darin bentspoon com>]

On Wednesday, June 6, 2001, at 01:20  PM, Anders Carlsson wrote:

> Did that, and it seems to work. Patches are attached.

Looking good. I think we still have some more refinement to do.

Does this work with named entities, or only numbered ones?

I'm not too happy with the eel_xml_get_escaped_property name. What's 
"escaped" got to do with it? I mean I know this interprets numeric 
entities, but those aren't usually called "escaped" characters, and it's 
just a bug workaround, so it should probably just be eel_xml_get_property.

> +	if (node == NULL)
> +		return NULL;

This needs braces around the single statement to fit Nautilus coding 
conventions.

> +	for (attrs = node->properties; attrs && attrs->name; attrs = 
> attrs->next) {

This needs to be "attrs != NULL && attrs->name != NULL" to fit Nautilus 
coding conventions.

> +			for (node_value = attrs->val; node_value; node_value = 
> node_value->next) {

This needs to be node_value != NULL to fit Nautilus coding conventions. I 
also think it should be value_node, not node_value.

> +					node_value->name && node_value->name[0] == '#') {

This needs to be node_value->name != NULL to fit Nautilus coding 
conventions.

> +					if (entity_char != 0)
> +						g_string_append_c (result, entity_char);

This needs braces around the single statement to fit Nautilus coding 
conventions.

> +				else {
> +					g_string_append (result, node_value->content);
> +				}

Are there really only two types of nodes here? You check for 
XML_ENTITY_REF_NODE and handle that, but are there other types. Also, when 
node_value->name is NULL or doesn't have a '#', you fall into this code 
that appends the content, which I think could cause trouble (seg. faults)? 
I'd like to see a switch statement here instead of the if.

I notice that you changed the 3 calls to xmlGetProp that are used to read 
metadata. But what about the 35 other calls to xmlGetProp in Nautilus? And 
what about the 4 functions in eel-xml-extensions.c itself that use 
xmlGetProp? Are you sure those functions aren't used by metadata code? If 
they are we definitely need them to use the fixed xmlGetProp. At least 
some of the non-metadata callers might need the same bug-fix workaround.

Finally, this creates a dependency between Nautilus and a new Eel. We have 
avoided that since 1.0.3 up until this point. So if you check this in 
before tomorrow, I'll have to make a new Eel release along with the new 
Nautilus release. So we might want to put this in 1.0.5 rather than 1.0.4,
 depending on the timing.

Anyway, I think you should take another cut at this after we talk over 
these points. Thanks for tackling this problem!

    -- Darin