BaseObservableList

[Michael Heinrichs]

Hi Martin,

as I said earlier, I do not like this approach, and I am still not persuaded otherwise. :-)

I think it is a rather complex implementation. (Ok, I am in an unfair advantage here, because I have seen the implementation already.) My feeling is, that you cannot do anything out of ordinary without knowing the source code of BaseObservableList. And this is always a bad sign. For example there are two patterns how to generate the Change object, one for simple operations and one for complex. What is a simple operation and what is a complex operation? Can an extending class change the pattern that is used by default for a certain operation? Why do I have to call commit() after generating a simple Change and when should I call it?

There is a lot of functionality in this class that is only needed for writable lists. I expect almost all extensions of BaseObservableList will be read-only. It seems to be a waste to have all of that functionality in a class, if it is most of the time not needed. IMO BaseObservableList should be optimized for the read-only case and be extendible for the few writable cases. (Or we introduce a two classes for both the two cases.)

If you define a public class that is supposed to be extended, you commit not only on the public API, but also on the implementation. My feeling is, we are not at a point yet, where we can commit on the implementation.

Overall, I do not think we need this for any of the features that is scheduled for the 2.1 release. We already discussed a good solution for http://javafx-jira.kenai.com/browse/RT-15029 internally that does not require the introduction of BaseObservableList. I would suggest, we focus on the smaller solution for now, discuss that on the public alias, and keep experimenting a little more with other options for BaseObservableList after the 2.1 release.

- Michael



On 17.12.2011, at 08:45, Martin Sladecek wrote:

> On 12/17/2011 01:22 AM, Richard Bair wrote:
>> Hi Martin,
>> 
>>> Hello,
>>> I'm working on http://javafx-jira.kenai.com/browse/RT-15029 and as part of this effort, I'm doing a refactoring of ObservableListWrapper.
>>> 
>>> So I've created an abstract class BaseObservableList (extends AbstractList) that could serve as a base class for all ObservableLists implementations.
>> OK, the "Base" naming convention is one that I really regret, I wish we had just done AbstractFoo. Oh well. The things you end up regretting are often not the things you thought you might :-) But the "Base" convention we're using (hopefully everywhere...) is FooBase rather than BaseFoo, so I guess this should be ObservableListBase.
> 
> OK.
>> 
>>> For a simple (modifiable) implementation, one would need to implement following methods:
>>> 
>>>    public int size() {
>>>    }
>>> 
>>>    public E get(int index) {
>>>    }
>>> 
>>>    protected void doAdd(int index, E element) {
>>>    }
>>> 
>>>    protected E doRemove(int index) {
>>>    }
>>> 
>>>    protected E doSet(int index, E element) {
>>>    }
>>> 
>>> 
>>> 
>>> So something like ObservableListWrapper would look like this:
>> Is the proposal also to make ObservableListWrapper public?
> 
> Yes. I've added invalidate() methods, we were discussing before, into BaseObservableList. So making ObservableListWrapper public would also allow calling invalidate(from, to) or invalidate(index) on it.
>> 
>>> Now when somebody would want to override something else or extend the implementation, an IterableChangeBuilder, that's used in BaseObservableList, needs to be used for this purpose.
>>> 
>>> So when creating something by delegating to the methods from the List, you just need to wrap the block in
>>> changeBuilder.beginComplexChange(); and changeBuilder.endComplexChange();
>>> This builds up the change in the calls in between instead of calling the observers multiple times.
>>> 
>>> E.g. setAll method works like this
>>> 
>>>    public boolean setAll(Collection<? extends E>   col) {
>>>        changeBuilder.beginComplexChange();
>>>        clear(); //does not call ListChangeListeners
>>>        addAll(col); //neither does this
>>>        changeBuilder.endComplexChange(); //this calls the ListChangeListeners using just one Change object
>>>        return true;
>>>    }
>>> 
>>> 
>>> On the other hand, if you want to override something simple like add/remove/set or use just doSet, doAdd and doRemove methods or you directly manipulate with your data structure, you have to use a different pattern and use methods from the IterableChangeBuilder (there are methods like nextAdd, nextRemove, nextSet). And then end it with commit() call.
>>> 
>>> So, e.g. you want to implement some fast clear() (the original clear in AbstractList iterates over the elements and remove them one by one, building up the resulting change on the way), you would do:
>>> 
>>> public void clear() {
>>>    //do the fast clear
>>>    changeBuilder.nextRemove(0, listOfRemovedItems);
>>>    changeBuilder.commit(); //Calls the ListChangeListeners, but only if not in ComplexChange block
>>> }
>>> 
>>> 
>>> Of course, these 2 patterns could be combined and you can do something like this:
>>> 
>>> changeBuilder.beginComplexChange();
>>> clear(); //no call to observers, as we are in ComplexChange block.
>>> // do some stuff using doAdd or by directly changing my data structures
>>> chnageBuilder.nextAdd(0, x); //I've added something so I need to report this
>>> changeBuilder.endComplexChange(); //calls the ListChangeListeners
>>> 
>>> 
>>> What do you think about this API?
>> Why is it called an IterableChangeBuilder? Is there a complete description of the API I can see (maybe attach to the issue?).
> We had a NonIterableChange in com.sun.javafx.collections, which did not allow iterating using next(), so this one was for creating IterableChanges. But it doesn't make much sense in javafx.collections package to call it like this, so maybe we could call it ListChangeBuilder?
> 
> I don't have any javadoc for that yet, but I'll try to describe it briefly:
> (package-private) IterableChangeBuilder(BaseObservableList<E> list); // Create a new builder associated with list. This is called by BaseObservableList in it's constructor
> 
> public void beginComplexChange(); // Begins a complex change, building up a change until endComplexChangeIsCalled(). Nested complex changes are allowed.
> 
> public void  endComplexChange(); // If this is a topmost complex change, create a Change object a call callObservers on the list.
> 
> public void commit(); // If we are not in a complexChange, create an Change object and call callObservers
> 
> public boolean isEmpty(); // if there's some change stored in the builder
> 
> public void nextAdd(int from, int to); // added something on interval [from, to)
> 
> public void nextPermutation(int from, int to, int[] perm); //permutation perm on interval [from, to)
> 
> public void nextRemove(int idx, E removed); //single element removed from idx
> 
> public void nextRemove(int idx, List<E> removed); //multiple elements removed from idx.
> 
> public void nextReplace(int from, int to, List<E> removed); replaced removed with elements on interval [from, to)
> 
> public void nextSet(int idx, E old); // changed element "old" on index "idx"
> 
> public void nextUpdate(int pos); // updated element on position pos
> 
> public void reset(); //builder reset
> 
> 
> Right now, it's tightly coupled with BaseObservableList, calling directly it's callObservers method, but I'm going to separate them, so it can be used in any implementation, not just the one based on BaseObservableList.
> Right now, the only way to use it is to use protected field in BaseObservableList.
> 
> -Martin
> 
>> 
>> Thanks!
>> Richard
>