[jdom-interest] Bug in Element.getChildren().listIterator(pos)

[Paul Cantrell]

Brad --

You're quite right -- and there's a deeper problem here: it's  
impossible to choose a position which satisfies both hasNext() and  
hasPrevious().  It looks like Vadim's patch solves this problem neatly.  
  The alternative would be to add a field which tracks the highest  
matching position, and use that instead of size() for answering  
hasNext(), but that gets awkward quickly -- Vadim's approach seems  
simpler.

Another alternative, perhaps less error-prone (and perhaps not!), would  
be to have FilterListIterator wrap an unfiltered ListIterator, and then  
take an approach similar to InnigCollections.select(Iterator,Selector):

    
http://innig.net/util/innig-util/src/net/innig/collect/ 
InnigCollections.java

This avoids some of the complex machinery of tracking the iterator's  
last operation ... but the simplicity might break down when you're  
allowed to iterate in both directions.

Cheers,

Paul

On Sunday, August 24, 2003, at 01:43  PM, Bradley S. Huffman wrote:

> I think this is the same bug found by Vadim Strizhevsky at the end of  
> July.
> There is a patch at
>
> http://lists.denveronline.net/lists/jdom-interest/2003-July/013289.html
>
> but it hasn't been fully tested either.
>
> Looking at your patch, I'm not sure it is correct. If start = 0 and the
> backing list contains only a single Text node then initializeCursor
> would return 0 cause a call to hasNext to be true, but index 0 in the  
> backing
> list is a Text node not a Element.
>
> Brad
>
> Paul Cantrell writes:
>
>> Working on Macker (at http://innig.net/macker if you're curious), I
>> found a bug in the listIterator(int) method of the list returned by
>> Element.getChildren().
>>
>> The following code demonstrates the bug -- it incorrectly generates a
>> ClassCastException if element contains text content after its last
>> child element:
>>
>>          List children = element.getChildren();
>>          for(ListIterator childIter =
>> children.listIterator(children.size()); childIter.hasPrevious(); )
>>              {
>>              Element subElem = (Element) childIter.previous();
>> 	.......
>>              }
>>
>> The exception occurs because the call to childIter.previous()
>> erroneously returns a Text object the first time it is called.
>>
>> The problem appears to be in the last line
>> ContentList.java.initializeCursor(int start), which is
>> ContentList.java:1277 in the nightly I just picked up.  The final
>> return incorrectly presumes that the last element in the list matches
>> the filter.  The method *should* go something like this:
>>
>>          private int initializeCursor(int start) {
>>              if (start < 0) {
>>                  throw new IndexOutOfBoundsException("Index: " +  
>> start);
>>              }
>>
>>              int count = 0;
>>              int lastMatchPos = -1;  // <---------
>>              for (int i = 0; i < ContentList.this.size(); i++) {
>>                  Object obj = ContentList.this.get(i);
>>                  if (filter.matches(obj)) {
>>                      lastMatchPos = i;  // <----------
>>                      if (start == count) {
>>                          return i;
>>                      }
>>                      count++;
>>                  }
>>              }
>>
>>              if (start > count) {
>>                  throw new IndexOutOfBoundsException("Index: " +  
>> start +
>>                                                      " Size: " +  
>> count);
>>              }
>>
>>              return lastMatchPos + 1; // <---------
>>          }
>>
>> WARNING: I haven't tested this patch -- I just typed it straight into
>> the email.  (Sorry!)  I hope it's not too far off the mark.
>>
>> Cheers,
>>
>> Paul