Bugzilla – Full Text Bug Listing
| Summary: | [DataBinding] ObservableList iterator implementation does not delegate back to outer class | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Boris Bokowski <bokowski> | ||||||||||
| Component: | UI | Assignee: | Boris Bokowski <bokowski> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | Ed.Merks, mallo.ovidio, nigelipse, qualidafial | ||||||||||
| Version: | 3.4 | Keywords: | helpwanted | ||||||||||
| Target Milestone: | --- | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 88735 [details]
work in progress
This is harder than I thought. I'm giving up for now - I agree that this is not as convenient as it could be, but if I am not mistaken, these operations are spec'd as being optional, and there is nothing that prevents subclasses from implementing iterator() and listIterator() in a more complete way.
Created attachment 88736 [details]
mylyn/context/zip
Btw, just delegating back to the outer class does not work unless you come up with a complete implementation of listIterator that keeps track of the modCount. Otherwise, you will see ConcurrentModificationException being thrown because the modCounts of the underlying collection and the one of the iterator don't match any more. Remove on the iterator is optional, but it should only throw an exception if the list itself is read-only... (In reply to comment #4) > Remove on the iterator is optional, but it should only throw an exception if > the list itself is read-only... I don't understand: * @exception UnsupportedOperationException if the <tt>remove</tt> * operation is not supported by this Iterator. this is the Javadoc for Iterator.remove(). It does not say under which circumstances an iterator should or should not support the remove operation. I don't think this can be reasonably fixed in ObservableList, for the reason stated in comment #3. ObservableList is designed to be unmodifiable unless subclassed and overridden explicitly. I think the best option is to document iterator() and listIterator() to state that subclasses need to override these methods if they want the iterator's mutator methods to work. If you want to be a lawyer, that would be a good point. :-P But, in my opinion, if list.remove(index) is supported then list.listIterator(index).remove should do the same thing, i.e., one should be able to assume that if the list supports remove, the list's iterator supports it too and vice versa. I doubt you'll be able to find an exception to that general rule. The comment in #6 seems reasonable, but if it can be solved in a subclass one might ask why it can't be solved in the base class once and for all? I.e., how will WritableList fix the problem and why isn't that solution applicable in the base class? And again, I don't think making the iterator's supported operations match those of the list itself is just a discretionary thing... I'm not sure which of us you're accusing of lawyer-speak, but I agonized over the wording of my comment--after 20 minutes I gave up and hit submit. :) We should definitely make modifications possible in the WritableList iterators, taking care to fire the proper list change events. I'll take a look at it. Don't worry. I'm not accusing anyone of being a lawyer. That would be mean! Hehehe. I can try to help out as well. But I just got back from a two week vacation and the back log is a little overwhelming. Created attachment 93650 [details] Make iterators modifiable in WritableSet and WritableList -- needs unit tests Checking for comodification of wrappedSet|wrappedList field per bug 224129 Created attachment 93651 [details]
mylyn/context/zip
I don't think it will be possible to implement modifiable iterators in a way that is reusable between observable collections. My experience has been that each class's iterators need to do something special for the particular class. E.g. ValidatedObservableList has to synchronize on the target observable list, whereas WritableList just has to modify its internal list. Matthew is right that if you want an iterator that supports 'remove' then each implementation needs to override iterator() and provide the appropriate support. I have written the test for WritableList and checked in Matthew's code (only needing to update it for generics). This has been checked into the e4 repository (available at http://download.eclipse.org/e4/downloads/). *** Bug 308105 has been marked as a duplicate of this bug. *** |
From Ed Merks: > I found it odd that the iterator mutator methods don't delegate > back to the corresponding out class methods: > > public Iterator iterator() { > getterCalled(); > final Iterator wrappedIterator = wrappedList.iterator(); > return new Iterator() { > > public void remove() { > throw new UnsupportedOperationException(); > } > > public boolean hasNext() { > return wrappedIterator.hasNext(); > } > > public Object next() { > return wrappedIterator.next(); > } > }; > }