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

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]