TomasMikula / ReactFX

Reactive event streams, observable values and more for JavaFX.
BSD 2-Clause "Simplified" License
375 stars 47 forks source link

[Feature Request] Vetoable Listeners, LiveSet & LiveMap #23

Open DirkToewe opened 9 years ago

DirkToewe commented 9 years ago

Hello,

I have a scene graph-like data structure that heavily relies on observable properties to keep the components mutually updated. I would like to use listeners to prevent/veto invalid changes of properties. Using a Change(Performed)Listener to change back values is not satisfactory for two reasons:

Is there any way to improve Val and Var in the ReactFX library in a way that also allows for vetoable listeners? I understand that this would not work in interaction with FX properties as they do not support VetoListeners. But in my application the data-stucture is separate from the JavaFX Views so I would like to use the ReactFX properties as "standalone".

I have written my own little property library that has an additional listener that is listening to events before changes are performed (ChangeProposedListener). The problem is my library is non-lazy, will create memory leaks left and right and is over all not nearly as awesome and powerful as the ReactFX library (It was created in 2 hours....).

In addition to that I would also require both a vetoable LiveSet and LiveMap as they are used in the model as well.

I feel like this is too much to ask for. A the same time I believe many other people could use vetoability as well. Just look at the horrible abomination of a VetoableListDecorator that can be found in the JavaFX library.

cheers Dirk

TomasMikula commented 9 years ago

Hi Dirk,

can you perhaps give a small example of what you mean by vetoable listeners and how you would use them (e.g. to prevent inconsistency)?

The original reason why I didn't include LiveSet or LiveMap is that I wasn't sure what should the semantics of map (mapKeys for LiveMap) operation be. It is tricky when the map is not injective, i.e. when it maps two or more elements to one. A solution could be that the result of map/mapKeys is a MultiSet/MultiMap. map also cannot be implemented as a view of the original Set/Map.

More recently, I'm not convinced the approach taken by JavaFX (and adopted by ReactFX's LiveList) of having a special kind of observable objects (ObservableList, ObservableSet, ...) for each collection type is the right approach. Maybe we should instead search for a generalization of ObservableValue/Val that can be specialized to conveniently handle collections.

DirkToewe commented 9 years ago

Two possibilities:

Either throwing Exceptions:

Var<Float> var = simpleVar(1f);
Val<Float> val = var.map( x -> x*x );
val.addChangeProposedListener(
  (oldVal,newVal) -> { if(newVal > 3) throw new IllegalArgumentException(); }
);
var.setValue(2); // <- should throw the IllegalArgumentException

A maybe cleaner way would be to return a Vote:

Var<Float> var = simpleVar(1f);
Val<Float> val = var.map( x -> x*x );
val.addChangeProposedListener(
  (oldVal,newVal) -> newVal > 3 ? new Veto("square too large") : new Approval().onRejection( () -> {} ).onPass( () -> {} }
);
var.setValue(2); // <- should throw the IllegalArgumentException
var.proposeValue(2); // <- should return some kind of answer Object

Hope this makes sense.

Also I would be perfectly happy if LiveSet.map would throw an UnsupportedOperationException or would not exist at all.

I would argue that JavaFX's separation between ObservableList and ObservableValue does not go far enough: ObservableValue[ObservableList[E]] and ObservableList[E] have completely different purposes in my opinion and should not be mixed up. ObservableValue<ObservableList[E]> is supposed to keep track of the reference to a list. While ObservableList[E] is supposed to keep track of the entries of a list.

TomasMikula commented 9 years ago

In both cases, the result of var.setValue/var.proposeValue depends on what listeners are attached to val. Could you handle the exception/negative answer in any meaningful way?

My view is this: If var should never be set to anything bigger than sqrt(3), it is OK to throw an exception, because it is a bug that should be fixed. If there is no restriction on the value held by var, but val should never be greater than 3, then filter out values greater than 3 from val (leaving var set to whatever).

DirkToewe commented 9 years ago

In 95% of all cases that is absolutely true, but:

In my opinion it is bad practice to throw Exceptions after changes have been performed.

I see the val as a user/consumer of the var. It has the right to complain but no right to change it.

TomasMikula commented 9 years ago

Let's say it's a Bug. It may still be important that the Model stays intact. What if you need to do a backup of the data for recovery? You cannot save a corrupted data model.

It seems to me that whatever check you do to veto an update, you could do before pushing an invalid value into the dataflow. Moreover, by the time you detect an inconsistency, the data may have been corrupted already, by a bug that went undetected. Therefore, I don't see any rollback buying you much.

In my case it's a user input.

You should validate all user input in the first place.

Anyway, so you imagine a dry run for each update and if everything goes well (no veto), then do the actual update? What if the actual update causes an exception?

DirkToewe commented 9 years ago

You should validate all user input in the first place.

The problem is that the validity of a text value depends on where it is in the whole data structure. Imagine a name of a tree node that needs to be unique within the parent. I would like to not have to traverse the datastructure inside the JavaFX components. Checks become a lot more complex as well as they have to aquire context/traverse the data structure.

Anyway, so you imagine a dry run for each update and if everything goes well (no veto), then do the actual update? What if the actual update causes an exception?

Sh\ hits the fan :smile: ... There is no way to make this bullet proof. But my idea is that simple small checks by individual components are simpler and safer than complex queries before pushing a change to the data structure. It is also more modular and extensible. The data model guy can do things the GUI guy does not even have to think about. The different data guys can write modular checks.

TomasMikula commented 9 years ago

I see your motivation. Still, the idea that values accepted by a property depend on who is listening seems fragile to me

Modular checks? Sure! You can construct a Val<Predicate<String>>, where the current predicate would be constructed based on the currently selected tree node. Then use that predicate to test the input. The predicate would be updated appropriately as you traverse the tree.