Hi JS, On Thu, Jan 7, 2010 at 3:20 PM, Jean-Sébastien Guay <[email protected]> wrote: > Here are the changes. Essentially: > > 1. When reading, a tag can end with either ">" or "/>". If it ends with > "/>", then we don't read any children. > 2. When writing, if a node has no children then it is written as <blah ... > />. > > With these changes, I can read a relatively complex XML file saved from an > application (which is what I want to read from the start), then re-write it > to another file and the application loads that new file correctly. >
Changes look reasonable, and all the p3d files I have read just fine, so changes now merged and submitted to svn/trunk. > Note that the read code assumes that "/" can only occur if it's part of the > "/>" (except if it's part of a value in a property). I think that's OK. Umm... perhaps my review has thorough enough. So with your changes what would happen if we had: <mytag> 1.0 / 2.0 </mytag> ? > But > it highlights the main problem of writing OSG's own parser... We're not XML > experts and the parser is surely not standards-compliant (which was also the > cause for this very submission). Using the parser from some 3rd party would > have been better in this regard, but I can appreciate the will to keep the > number of dependencies down. It's a fine line to walk. I just wanted to > highlight the fact that I hope my assumption is OK, but I'm not sure. The external dependency issue is a big one for core OSG features, the recent thread about problems with getting png images loaded under Windows is a great example. Lots of osg-users traffic for a dumb issue that is can be really painful under certainly platforms... With the introduction of xml configuration/file formats in the core OSG I'd rather forgo the higher capabilities of dedicated xml parsing libs for the easy of build and use of the core OSG. In the case of p3d parsing, the code actually got a lot simpler and easier to maintain with the introduction of the new parser, dropping the 3rd party xml parser actually a breath of fresh air for this particular plugin. The ease of use of XmlParser also has led me to use xml for other little projects, where without it I would have probably written my own custom parser. > Last thing, concerning the writing code, before my change the cases for > GROUP and NODE were identical. But on read, we set the type to GROUP if the > node has any children, so does the NODE case need to write its children at > all? If the type is NODE it shouldn't have any children. Or is it written > that way in case user code added children to a NODE without changing the > type to GROUP? And in that case, do we really need a distinction between > NODE and GROUP? Perhaps they should be treated exactly the same with regard > to writing the file? I wrote XmlNode as composite capable of having children right in the base class for simplicity, the types are also a hint to both users traversing the XmlNode graph and the writing out of the graph to file. In the case of NODE one would typically have it without any children, but if users add them then they should also change the type of GROUP to be consistent. I guess one could just drop the GROUP type completely, but this does lose some information about the intent of that particular node in the Xml graph. The same children issue applies to most the XmlNode types - should a COMMENT record have children? What about an ATOM? Personally I'm not inclined to get too hung up about enforcing strict rules for this particular class - it's not intended to be a be all and end all of Xml parsers, just a simple helper class for reading/writing basic xml files easily. Robert. _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
