Attempt to fix string escaping and template problems.

[Kiisu-Master]

Fix #23

I don't know exactly what cdata is. Should it be escaped too?

I'm a bit concerned about the performance impact of the weird template formatter i made.
I made it because it's way cleaner to work with templates obviously and it would be ugly to switch it all to concatenating.
In my testing it seemed to solve the problem though.

[Kiisu-Master]

Ok I just did some testing on a random website HTML file (30kb).
Before it would not preserve the content one to one after parsing and dumping.
With this PR it seems to preserve it.

Edit: As the file is 30kb and it doesn't seem to take considerably longer to process than before, i don't think there is any big performance impact with this.

Edit: My testing is inaccurate

[elenakrittik]

I don't know exactly what cdata is. Should it be escaped too?

SO post. In general, it should be escaped as well, but i'm afraid we won't be able to use String.xml_escape for it as it has a different set of characters that need escaping.

[Kiisu-Master]

Ah so its like a comment but part of the document. That's weird

[Kiisu-Master]

So i believe i fixed the problems my format function had before, but i don't know if it's really that much better to use a whole custom non-recursive string formatter instead of just concatenating the strings.

I'm also unsure about what exactly xml_escape does and when to use its option to escape quotes. It seems to work though.

[Kiisu-Master]

I don't know exactly what cdata is. Should it be escaped too?

SO post. In general, it should be escaped as well, but i'm afraid we won't be able to use String.xml_escape for it as it has a different set of characters that need escaping.

Reading up on that, according to the specification explained in this SO answer, you can have anything in a CDATA block without it getting parsed, except its end sequence. The end sequence cannot be escaped, anything else doesn't need escaping. That's how I understand it.

[elenakrittik]

i don't know if it's really that much better to use a whole custom non-recursive string formatter instead of just concatenating the strings.

I don't remember saying that i was against concatenating method specifically 🤔 If you could re-do this with concatenation instead, it would be much appreciated (and more concise as well).

what exactly xml_escape does and when to use its option to escape quotes

It turns e.g. " hello '>.<' " into " hello '>.<' ", and .xml_unescape() turns that back into " hello '>.<' ".

[elenakrittik]

The end sequence cannot be escaped

If we really want to, we can do what's suggested in this answer.

[Kiisu-Master]

I don't remember saying that i was against concatenating method specifically 🤔 If you could re-do this with concatenation instead, it would be much appreciated (and more concise as well).

😐?

    if self.standalone:
        return "<" + self.name + attribute_string + "/>"
    else:
        return "".join([
            "<" + self.name + attribute_string + ">",
            self.content.xml_escape() + cdata_string + children_string,
            "</" + self.name + ">"
        ])

[elenakrittik]

I don't remember saying that i was against concatenating method specifically 🤔 If you could re-do this with concatenation instead, it would be much appreciated (and more concise as well).

😐?

    if self.standalone:
        return "<" + self.name + attribute_string + "/>"
    else:
        return "".join([
            "<" + self.name + attribute_string + ">",
            self.content.xml_escape() + cdata_string + children_string,
            "</" + self.name + ">"
        ])

I meant that as in "more concise than the 'escaper' you currently have". Additionally, you can use parentheses instead of .join()ing parts and extract parts into variables.

[elenakrittik]

Looks good to me, sorry for the review taking so long and thanks for contributing!

[Kiisu-Master]

Your commit introduced double unescaping i think...
Godots xml parser already does unescaping.

(These errors don't happen without your commit.)

[elenakrittik]

If you mean that when e.g. <node attr="&quot;"> gets parsed into a node it has .attributes["attr"] == '"', then that's intentional. Is this not what you expected?

[Kiisu-Master]

You added xml_unescape but Godot xml parser already does unecaping.
This means it gets unescaped twice now.

func test_unescaping() -> void:
	var xml := XMLNode.new()
	xml.content = """
	xml.attributes["one"] = "<"
	xml.attributes["two"] = "& "
	var parsed_xml := XML.parse_str(xml.dump_str(true)).root
	# This is a function i wrote that uses GDUnit to test if all properties match (It will fail in this case)
	assert_xml(xml, parsed_xml)