djspiewak / anti-xml

The scala.xml library has some very annoying issues. Time for a clean-room replacement!
http://anti-xml.org
Other
169 stars 35 forks source link

Zipper.unselect does not preserve order of flatMapped children #36

Closed josharnold52 closed 13 years ago

josharnold52 commented 13 years ago

If Zipper.flatMap is used to replace one node with many, then Zipper.unselect does not preserve the order of the many. This is because a Map[Int,Set[Int]] is used to correlate elements from the original group to elements in the modified group, and Set makes no ordering guarantees.

For Zipper as it stands, I think the elements of that set will always form a contiguous range, so a Set might actually be overkill here. I'm going to try replacing it with a Range or similar and see if that holds up. Assuming that works, do you envision ever needing the more general correlation provided by Set?

scala> val tree = <top><a /></top>.convert
tree: com.codecommit.antixml.Elem = <top><a/></top>

scala> val expand = (tree \ 'a) flatMap {
     |   case e:Elem =>
     |   for(indx <- 0 until 10) yield e.copy(name=e.name+indx)  
     | }
expand: com.codecommit.antixml.Zipper[com.codecommit.antixml.Elem] = <a0/><a1/><a2/><a3/><a4/><a5/><a6/><a7/><a8/><a9/>

scala> val newTree = expand.unselect
newTree: com.codecommit.antixml.Zipper[com.codecommit.antixml.Node] = <top><a0/><a5/><a1/><a6/><a9/><a2/><a7/><a3/><a8/><a4/></top>
josharnold52 commented 13 years ago

The above commit (pull request #37 ) addresses the issue. I was actually able to replace the Set[Int] with just an Int defining the # of nodes in the Zipper corresponding to the source child group. That suffices to reconstruct the correspondence if you make 2 assumptions:

  1. Every node in the Zipper corresponds to a single node in the source.
  2. If a N1 precedes N2 in the source, then the Zipper's nodes corresponding to N1 precede the Zipper's nodes corresponding to N2.

These assumptions already held true for all Zipper instances, so nothing is really changing in terms of functionality. It's just that now those assumptions are required for ZContext to make sense, whereas before it allowed for more general mappings.

djspiewak commented 13 years ago

I'm so sorry it's taken me so long to get back to you on this. Basically, the new restriction is that the mapping end-points need to be contiguous for a particular element? I think that's a fairly safe assumption. In fact, it falls precisely within the constraints of normal sequence operations (map, flatMap, etc). I'm going to pull in your changes; thanks!