WalkKmlDom not traversing children of Kml containers (e.g. KmlDocument, KmlFolder)

[GoogleCodeExporter]

The problem:
-----------
   I was trying to use 'GEHelpers.FlyToObject' with a simple example KML (myplaces.kml) which contains
  3 placemarks contained within a folder element and the folder element is inside a document element.
  The problem is that bounds returned by 'KmlHelper.ComputeBounds' are empty and so the
  'KmlHelpers.SetBoundsView' method is never called.

  The reason the bounds are empty appears to be because the 'WalkKmlDom' never traverses deeper
  than the top-level 'KmlDocument' element/object.  And the reason why the traversal stops is because
  'HasChildNodes' is returning false.

   The 'HasChildNodes' returns false because it is being passed the local 'objectContainer' variable
   and not the 'KmlDocument' itself.  That is, in 'WalkKmlDom', if we are at a container node type,
   then
       objectContainer = feature.getFeatures();

   However, in 'HasChildNodes', the 'getFeatures()' is invoked again on the argument, i.e.:
      return Convert.ToBoolean(feature.getFeatures().hasChildNodes());

   The redundant call of 'getFeatures()' on the objectContainer causes a runtime exception
   that is caught and false is returned.

Possible Fix:
----------
   Instead of changing 'WalkKmlDom', I propose making 'KmlHelpers.HasChildNodes' more
   flexible.  That is, replace the following line:
      return Convert.ToBoolean(feature.getFeatures().hasChildNodes());
   with something like this:
      if ( IsKmlContainer(feature) ) {
        feature = feature.getFeatures();
      }
      return Convert.ToBoolean(feature.hasChildNodes());

   This would fix the problem and is less likely to break existing uses of the 'HasChildNodes'
    method.

Side-effects:
-----------
   If the 'HasChildNodes' method is changed, then a nearly identical fix has to be made to the
   'KmlHelpers.GetChildNodes' method as well.  Otherwise, 'GetChildNodes' will return null
   and the original problem still occurs.

   Related to these changes, I also suggest modifying the following line in the 'WalkKmlDom'
   method:
      int count = childNodes.getLength();
   and changing it to something like:
      int count = ( childNodes == null ? 0 : childNodes.getLength() );

   The problem is that 'GetChildNodes' returns null if an error occurs and perhaps under other
   circumstances.

   The design question here is whether the tree traversal method should ignore the failure to get
   children (although 'HasChildNodes' has already returned true) or should the traversal fail because 
   obviously there's an inconsistency?  One way to handle this would be to "punt", e.g. add a error
   handler argument (callback) to the method signature and hence put the onus back on the caller.
   While this does make the traversal method very flexible, it is probably not worth the effort. 
   Instead I'd suggest just choosing a behavior, fail or ignore, and documenting it in comments.

System Info:
----------
   FC.GEPluginCtrls revision 513
   Window 7

   I search for reported issues with 'HasChildNodes' and 'KmlHelpers' and did not see any that
   pertained to this particular problem.  My apologies if this had been previously reported. 
   Also, I'm just starting to learn Google Earth API, the plugin API and your library so it is quite
   possible that behavior of the methods is correct and I've just misunderstood the functionality.

Last but not least, many thanks for all the hard work you put into this 
library.  It is very much
appreciated.

Original issue reported on code.google.com by kenneth....@gmail.com on 5 Nov 2012 at 8:55

[GoogleCodeExporter]

Hi,

I really appreciate the detailed information you have provided here. I have not 
had a chance to work on the library for a while but I will certainly have a 
good look through your suggestions and make changes as applicable asap.

I will respond here as soon as I have done this.

Best,

Fraser

Original comment by fraser.c...@gmail.com on 18 Nov 2012 at 7:01

• Changed state: Accepted
• Added labels: OpSys-Windows, Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

These changes have been made in the latest commit and seem to be working well. 
The inconsistency you outlined had been introduced when I rewrote the treeview 
to handle node creation dynamically. I really appreciate the time you have 
taken to report this. Many thanks.

Original comment by fraser.c...@gmail.com on 20 Nov 2012 at 4:19

• Changed state: Fixed