DataNode Write bug

[luvcraft]

if you put an empty List or TNet.List in a DataNode, and then do a non-binary write, it'll write the name of the List, followed immediately by a tab and then the next variable in the DataNode. The DataNode will then break horribly when you try to load it.

For example:

1. List<int> foo = new List<int>();
2. int bar = 1;
3.  

will write as:

1.         foo     bar = 1
2.  

the culprit is DataNode.cs line 483 and 497; in both of those cases, the

1.                                                 writer.Write(" = ");
2.                                                 writer.Write(Serialization.TypeToName(type));
3.                                                 writer.Write('\n');
4.  

should come before the

1. if (list.Count > 0)

I haven't tested to see if this happens with a binary write.

[ArenMook]

Good catch, thanks. Binary shouldn't be affected.

I'd fix it differently on my end though... I would add an "else" instead:

1.                                         if (list.Count > 0)
2.                                         {
3.                                                 writer.Write(" = ");
4.                                                 writer.Write(Serialization.TypeToName(type));
5.                                                 writer.Write('\n');
6.  
7.                                                 for (int i = 0, imax = list.Count; i < imax; ++i)
8.                                                         Write(writer, tab + 1, "Add", list.Get(i), false);
9.                                         }
10.                                         else writer.Write('\n');  // <-- this right here

Does that also make it work for you?

Edit: After some consideration... I like your approach better.

[luvcraft]

it looks like booleans are also broken? If I try to include a boolean in the DataNode, it gets stuck in a loop of creating System.Boolean values with a System.Boolean child, which itself has a System.Boolean child... and so on until it crashes from stack overflow.

Adding code to correctly write booleans around line 382 gets them writing correctly, but they don't read correctly (when it tries to read, it passes null as the type to SetValue).

EDIT: Got it reading correctly by adding the following at line 646:

1.             bool flag;
2.             if (Boolean.TryParse(line, out flag)) {
3.                 mValue = flag;
4.                 return true;
5.             }
6.  

there's probably a more elegant way to do that.

[ArenMook]

I think I already fixed it a while back, just haven't pushed an updated version for a while. But thanks for letting me know in any case. Currently bools are handled fine from everything I can tell. I don't use Boolean.TryParse, I just compare the string value "true" or "false".

[luvcraft]

There's still a bug writing bools in 1.9.6. at DataNode.cs you're missing a

1. writer.Write('\n');

after writing the bool at line 380, so it breaks the formatting.

also, can you please explain why you're using the strings "true" and "false" instead of writing with value.ToString() and reading with Boolean.TryParse()? Is the string comparison just a lot faster?

[ArenMook]

Thanks for pointing that out, I will fix it.

I use printed strings because I want them to be saved as lowercase, but I want them read in both lowercase as well as starting with a capital. It's easier when filling the data or modifying saved data by hand to not have to capitalize it for it to be understood, and it matches the code as well.

[luvcraft]

The MSDN article is confusing, because it initially says that Boolean.TryParse compares the value to the strings "True" and "False", but then in the remarks it says "The value parameter can be preceded or followed by white space. The comparison is ordinal and case-insensitive.", and in the example it correctly parses "true" and "     true     ":

http://msdn.microsoft.com/en-us/library/system.boolean.tryparse(v=vs.110).aspx

I just tested it myself, and found that it does ignore leading/trailing whitespace and case, so it'll also correctly parse "    true    ", "false\n", "TRUE", "FALSE", "tRuE" and "FaLsE".

1.     void test() {
2.         {
3.             string[] values = { null, System.String.Empty, "True", "False",
4.                           "true", "false", "    true    ", "false\n", "TRUE", "FALSE",
5.                           "tRuE", "FaLsE", "T", "F", "yes", "no", "0",
6.                           "1", "-1", "string" };
7.             foreach (var value in values) {
8.                 bool flag;
9.                 if (System.Boolean.TryParse(value, out flag))
10.                     Debug.Log("\"" + value + "\" --> " + flag.ToString());
11.                 else
12.                     Debug.Log("Can't parse \"" + (value == null ? "<null>" : value) + "\"");
13.             }
14.         }
15.     }
16.  

also, in your current implementation, "True" resolves as false anyway, because ResolveValue only compares the value to "true" and "false", and doesn't pass it on to SetValue as a bool because ResolveValue is called with a type of "null". I recommend adding

1. else { // is it a boolean?
2.                 bool b;
3.  
4.                 if (bool.TryParse(line, out b)) {
5.                     mValue = b;
6.                     return true;
7.                 }
8.             }
9.  

at 693, and removing the string comparisons at 652.

[ArenMook]

That's good to know about it not being as sensitive as the documentation initially seems to suggest.

The change you posted seems fine to me with one exception... you would be effectively doing a bool.TryParse on every single item that arrives, which is likely far more expensive than a pair of string comparisons.

That said, sending data via text isn't particularly performant either, so this should be fine.