[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    BaseObservableList
From:       michael.heinrichs () oracle ! com (Michael Heinrichs)
Date:       2011-12-19 10:31:08
Message-ID: 8A651B2F-A393-4819-AF74-26F9F28FCA70 () oracle ! com
[Download RAW message or body]

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
> 


[prev in list] [next in list] [prev in thread] [next in thread]