Children order management

[sylvaneau]

Hey ! That's me gain ;-)

We found a bug (actually already there before version 4.0), the order of serialized elements in KML is not correct regarding to KML format specifications.

On the current version of SharpKML, for a given object, children types are serialized after all element fields (properties). But it is actually not always the case, the best example if for Feature : StyleSelector(s) are not located after all fields (actually at position 14), so before "Region" for instance.

Also, extensions are not serialized after all children, the complete order is the following (for a Placemark) :

• Base Type elements (Feature)
  • Element (name)
  • [Collection of Elements (StyleSelector)]
  • Element (Region)
  • [Collection of Base type Extensions (FeatureExtension)]
• Child Type elements (Placemark)
  • Element (geometry)
  • [Collection of child type Extensions (PlacemarkExtension)]

To achieve this, I decided to not separate Elements serialization and Children serialization, now every Element / ChildType is managed by the TypeBrower the same way (they both produce an ElementInfo). The sorting is achieved in a slightly different way than before, for a given depth level (for instance feature) we sort both elements AND ChildTypeAttributes (so you can place children wherever you want in the serialization order) and we do it for each depth.

We do not need to sort Element.Children any more, so I switched back to a simple List.

The best way to check KML integrity is to use Notepadd++ and XSD schema to valid produced files. I might add some UnitTests soon to check this.

I have made some performance tests and I haven't noticed any memory or speed issue.

Feel free to contact me if there is anything.

Regards

[samcragg]

Thanks for submitting this - I'll take a look at the weekend.

In the meantime, do you have any unit tests for some of the changes (especially around the changes to TypeBrowser) as currently the build is failing because the unit test project hasn't been updated?

[sylvaneau]

My bad.... I'll try to update unit tests by this week end

[sylvaneau]

Hmmm I don't understand why the tests went wrong :-(

I have no access to the Internet where I am today, I will check again this evening

[samcragg]

The build seems to hang when running the unit tests - I'm guessing there's an infinite loop somewhere. Do they run locally, in which case I can investigate the CI settings?

[sylvaneau]

Mhhh that's strange. There are quite a lot of tests being in error. I will try to understand what's going wrong.

I made a fix on my branch (thanks to Unit tests) that prevents the infinite loop.

Sorry, this is not really straight forward.... I work in an environnement with very limited access to the internet and I was not able to run unit tests until this early afternoon

[sylvaneau]

Finally ! :-)

I haven't performed performance tests yet, I will do it today. My modifications might have slow down export a little bit but I think it's acceptable.

There is one point I'm not confident at all, in TypeBrowser, I do not understand what is the purpose of the "elements" member. To be more precise, I don't know if this piece of code still works correctly :

        private TypeBrowser(Type type)
        {
            this.ExtractAttributes(type);

            **// this part : ???**
            // Go in reverse so we store the first element
            for (int i = this.orderedElements.Count - 1; i >= 0; i--)
            {
                ElementInfo info = this.orderedElements[i];
                this.elements[info.Component] = info;
            }
        }

[samcragg]

Thanks for the hard work and sorry for the delay in getting back to you.

Looking through the whole code base and I'm thinking we should actually just get rid of the Children property (it used to be internal/protected) and each class can just have separate lists for each property. You've already shown that the ElementWalker doesn't need to use it and we'd be able to get rid of the ChildTypeAttribute and then the properties in the child could just use the normal KmlElement attribute to specify the order, e.g.:

public class Feature
{
    private readonly List<StyleSelector> styles = new List<StyleSelector>();
...
    [KmlElement(null, 14)]
    public IReadOnlyCollection<StyleSelector> Styles => this.styles;
...
    public void AddStyle(Style style)
    {
        // This will check for null arguments and that the element hasn't been
        // added to another instance
        base.AddChildTo(this.styles, style);
    }
}

I'll have a little play around and will let you know when I've pushed the branch up so you an have a look and decide which approach you prefer.

Thanks again

[sylvaneau]

I think it's a good idea indeed :-)

We have implemented some unit tests and found some issues, mainly :

• "Schema" should inherit from "KmlObject" as it does not have a "TargetId" attribute
• Some bugs in "Xal" namespace (wrong elements order)

I will get back to soon very soon with some fixes. But I'm not sure how to handle the first issue yet.

[sylvaneau]

(I use this MR as a To-Do)

There is also a improvement to TimePrimitive to avoid creating an instance of TimePrimitive from the kml namespace in the GXTimePrimitive property of the AbstractView.

[samcragg]

Cool, perhaps create a separate PR for those changes?

The branch I'm working on doesn't quite build yet due to having temporarily remove the extension work, which is why I haven't pushed it up yet, however, I have introduced an "IntegrationTests" folder that makes it easier to write some Kml and verify that it parses/serializes to the same value (useful for checking the order of the elements) and lets you check the object properties have been correctly read.

Hopefully at the weekend it might be in a publishable state for some feedback.

[samcragg]

I've just pushed up a pre-release so you can get an early play with it:

https://www.nuget.org/packages/SharpKml.Core/5.0.0-alpha1

I'll have a go at merging in your changes, as I think the example is really useful. I also want to do some benchmarking to make sure performance hasn't dropped too much and that we're not using excessive memory with all the new lists.

[samcragg]

I've pulled in your changes into my branch RemoveChildren if you want to see your example in action - I'll hold fire for a bit before merging into master so we can make any required tweaks (or if it's still not working, I'll just scrap it and try again 🙂)

[sylvaneau]

Hi !

I'll have a look on the next few days and tell you if i see any problem :-)

[samcragg]

I've just merged this in under 5.0.0, which will be in NuGet shortly