On 07/04/2021 03:41, Nir Lisker wrote:
In the PoC I made I specifically also disallowed 'null' as an input
I like the way ReactFX does it where the property is empty. I think that
this is also what you mean by disallowing `null` (in other contexts,
"disallowing null" would mean throwing an exception).
Yes, it is the same concept as ReactFX calling a property "empty", but I
was hesitant to call this as `null` is a valid value for many JavaFX
properties (a Scene can be null, a String can be null, etc.) which I
don't think means the same as it being empty (in the Optional sense).
But as long as the documentation is clear, I don't mind calling it either.
Not entirely sure what you mean by this.
Basically, what you said. My point was that this is a different API
section. The first deals with expanding the observables/properties
methods. The second with listeners methods. Even if mapping a property
requires a new listening model, like subscriptions, this is done under
the hood. Exposing this API should be a separate step. At least that's
how I see it.
Yes, I think it is good to limit new API as much as possible to reduce
scope and increase the chances of its acceptance. The subscription parts
can be designed separately and do not need to be public at this point.
They can be moved to a helper, or the implementation can take the extra
effort to use standard listeners.
I'd be happy to spend more time and work on this. Perhaps it would be
possible to collaborate on this?
That would be good. I will need to re-review the ReactFX internals and
see how your proposal differs exactly.
Yes, I think that would be good to do.
I've done some comparisons myself and didn't find a difference in
functionality with `Val` (so far). It is a new implementation though, I
didn't really look at how `Val` was done internally as implementing it
directly into JavaFX is quite different (I had to make a few minor
changes in `ObjectBinding` to allow for the choice of lazy binding). I
was also initially more focused on Streams only to realize at a later
point that having a Stream implement ObservableValue was not going to be
pretty (I suspect this also happened when ReactFX was created, which is
why Val/Var were later introduced in 2.x).
Both the PoC and Val do lazy binding and are null safe and provide
methods to deal with null/empty.
The main thing I didn't do yet is provide a `filter` method. Filtering
properties that you want to use for bindings seems awkard as a binding
should always have some kind of value. The `filter` method in ReactFX
basically maps the value to `null` when it doesn't match the filter.
I've left this out as you can easily achieve this with `map` and
`filter` seems to be too easy to misunderstand.
Aside from that, ReactFX's Val offers a lot of other methods that are I
think a bit too specialized to consider at this point, like the
`animate`, `pin`, `mapDynamic` and `suspendable` methods.
Val also has all the other `Optional` methods (ifPresent, isPresent,
isEmpty) but I think they may make the API a bit confusing (an
observable value is not the same as an optional). I've also not had a
need for these so far in practice and you can easily convert the current
value to get this functionality with `Optional.ofNullable`.
Finally Val offers a few methods to convert to ReactFX's streams. While
convenient, I think static methods like `Values.of`, `Invalidations.of`
or `Changes.of` would make for a less cluttered API to do stream
conversions -- this would also make it possible to leave this part of
the API up to a 3rd party.
By the way, do you make a distinction between ReactFX's Val and Var in
your proposal (one being read-only)?
No, `ObservableValue` is basically the same as `Val`, and the equivalent
to `Var` is `ObjectProperty`. Aside from it being a good companion to
`Val` (and less typing), I don't see a reason to implement `Var`.
--John
On Sun, Apr 4, 2021 at 12:43 PM John Hendrikx <hj...@xs4all.nl
<mailto:hj...@xs4all.nl>> wrote:
On 02/04/2021 08:47, Nir Lisker wrote:
> Hi John,
>
> I've had my eyes set on ReactFX enhancements for a while too,
especially as
> a replacement for the unsafe "select" mechanism. One of the things
that
> kept me from going forward with this is seeing what Valhalla will
bring.
> Generic specialization might save a lot of duplication work on
something
> like this, and Tomas touched another related issue [1], but since
it could
> be a long time before that happens, it's worth planning what we
can extract
> from ReactFX currently.
Agreed, Valhalla is certainly a highly anticipated feature but I
fear it
is still a couple of years away.
Even without any initial support for dealing with "? extends Number"
from the various ObservableValue specializations I think looking into
this can already be tremendous help.
The proof of concept mainly requires you convert the Number to a
suitable type when reading the property but has no problems in the
other
direction:
label.widthProperty().map(Number::doubleValue).map(x -> x + 1);
Not pretty, but certainly workable. Specific methods could be
introduced
(even at a later time) to make this more streamlined, similar to what
the Stream API offers with 'mapToDouble' etc.
> I think that we should break the enhancements into parts.
> The first that I would advise to look at are the additions to
> properties/observables. Tomas had to create Val and Var because he
couldn't
> change the core interfaces, but we can. Fitting them with the Optional
> methods like `isPresent`, `isEmpty`, `ifPresent`, `map`. `flatMap`
etc.;
> and `select` and friends, is already a good start that will
address many
> common requirements.
Yes, Val/Var had to be created for that reason, and also because
properties don't quite behave the same as streams -- streams with a
"toBinding" method results in things people didn't quite expect.
As far as the Optional methods go, I'm not entirely sure properties
would benefit from all of them. Properties are not immutable like
Optional and it may make less sense to fit them with 'isPresent',
'isEmpty' and 'ifPresent' ('ifPresent' would I think need to behave
similar to 'addListener' or 'subscribe').
In the PoC I made I specifically also disallowed 'null' as an input for
functions like 'map' and 'flatMap' (opting to use 'orElse' semantics
for
'null'), as this for allows much cleaner mapping (and especially flat
mapping when selecting nested properties). If 'null' were to be
allowed,
I think at a minimum we'd need to add another method to allow for easy
selecting of nested properties to avoid:
obs.flatMap(x -> x == null ? null : x.otherProperty())
> The second part is related to listeners. The subscription model
and event
> streams try to solve the memory issues with hard and weak
references, and
> allow better composition.
Not entirely sure what you mean by this. JavaFX's current model uses
weak references which was I think an unfortunate decision as it can
result in huge confusion. For example, a direct binding will work, but
with an indirection step a binding stops working:
button.textProperty()
.concat("World") // weak binding used here
.addListener((obs, old, cur) -> System.out.println(cur));
The above stops working, but without the 'concat' it keeps working.
I think the use of weak listeners should be avoided and instead other
mechanisms should be provided to make cleaning up easier. This is the
main reason for 'conditionOn' and why ReactFX even had a specialized
version of it: 'conditionOnShowing(Node)'.
> The third part is for collections - things like transformation lists
> (LiveList) and for other collections.
This is indeed best saved for last. The problems there I think are less
of an issue for now.
> Since these share behavior under the hood, we need to look ahead,
but in
> terms of functionality, I think we should take smaller steps. It
will also
> be easier to propose these then.
I've for this reason kept the PoC small with only the most basic
functionality. I did however add some work for a different
subscription
model, mainly because the internals of this code benefits greatly from
it. It is however kept to a minimum.
I'd be happy to spend more time and work on this. Perhaps it would be
possible to collaborate on this?
--John
>
> - Nir
>
> [1]
>
https://github.com/TomasMikula/ReactFX/wiki/Creating-a-Val-or-Var-Instance#the-javafx-propertynumber-implementation-issue
>
> On Wed, Mar 24, 2021 at 11:49 PM John Hendrikx <hj...@xs4all.nl
<mailto:hj...@xs4all.nl>> wrote:
>
>> I just wanted to draw some attention to a recent proof of concept
I made
>> in this pull request: https://github.com/openjdk/jfx/pull/434
>>
>> It is based on the work I did in
>> https://github.com/hjohn/hs.jfx.eventstream which is in part based on
>> work done in ReactFX by Tomas Mikula. The PR itself however shares no
>> code with ReactFX and is
>> completely written by me.
>>
>> If there is interest, I'm willing to invest more time in
smoothing out
>> the API and documentation, investigating further how this would
interact
>> with the primitive types and adding unit test coverage (I have
extensive
>> tests, but thesea are written in JUnit 5, so they would require
>> conversion or JavaFX could move to support JUnit 5).
>>
>> What follows below is the text of the PR for easy reading.
Feedback is
>> appreciated.
>>
>> ================
>>
>> This is a proof of concept of how fluent bindings could be
introduced to
>> JavaFX. The main benefit of fluent bindings are ease of use, type
safety
>> and less surprises. Features:
>>
>> Flexible Mappings
>> Map the contents of a property any way you like with map, or map
nested
>> properties with flatMap.
>>
>> Lazy
>> The bindings created are lazy, which means they are always
invalid when
>> not themselves observed. This allows for easier garbage
collection (once
>> the last observer is removed, a chain of bindings will stop observing
>> their parents) and less listener management when dealing with nested
>> properties. Furthermore, this allows inclusion of such bindings in
>> classes such as Node without listeners being created when the binding
>> itself is not used (this would allow for the inclusion of a
>> treeShowingProperty in Node without creating excessive listeners, see
>> this fix I did in an earlier PR: #185)
>>
>> Null Safe
>> The map and flatMap methods are skipped, similar to
java.util.Optional
>> when the value they would be mapping is null. This makes mapping
nested
>> properties with flatMap trivial as the null case does not need to be
>> taken into account in a chain like this:
>>
node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty).
>>
>> Instead a default can be provided with orElse or orElseGet.
>>
>> Conditional Bindings
>> Bindings can be made conditional using the conditionOn method. A
>> conditional binding retains its last value when its condition is
false.
>> Conditional bindings donot observe their source when the condition is
>> false, allowing developers to automatically stop listening to
properties
>> when a certain condition is met. A major use of this feature is
to have
>> UI components that need to keep models updated which may outlive
the UI
>> conditionally update the long lived model only when the UI is
showing.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>>
label.textProperty().bind(text.length().negate().add(100).asString().concat("
>>
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 -
v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> void updateLongLivedModelWhileAvoidingMemoryLeaks() {
>> // Standard JavaFX: naive, memory leak; UI won't get garbage
collected
>> listView.getSelectionModel().selectedItemProperty().addListener(
>> (obs, old, current) ->
>> longLivedModel.lastSelectedProperty().set(current)
>> );
>>
>> // Standard JavaFX: no leak, but stops updating after a while
>> listView.getSelectionModel().selectedItemProperty().addListener(
>> new WeakChangeListener<>(
>> (obs, old, current) ->
>> longLivedModel.lastSelectedProperty().set(current)
>> )
>> );
>>
>> // Standard JavaFX: fixed version
>> listenerReference = (obs, old, current) ->
>> longLivedModel.lastSelectedProperty().set(current);
>>
>> listView.getSelectionModel().selectedItemProperty().addListener(
>> new WeakChangeListener<>(listenerReference)
>> );
>>
>> // Fluent: naive, memory leak... fluent won't solve this...
>> listView.getSelectionModel().selectedItemProperty()
>> .subscribe(longLivedModel.lastSelectedProperty()::set);
>>
>> // Fluent: conditional update when control visible
>>
>> // Create a property which is only true when the UI is visible:
>> ObservableValue<Boolean> showing = listView.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false);
>>
>> // Use showing property to automatically disconnect long lived
model
>> // allowing garbage collection of the UI:
>> listView.getSelectionModel().selectedItemProperty()
>> .conditionOn(showing)
>> .subscribe(longLivedModel.lastSelectedProperty()::set);
>>
>> // Note that the 'showing' property can be provided in
multiple ways:
>> // - create manually (can be re-used for multiple bindings though)
>> // - create with a helper: Nodes.showing(Node node) ->
>> ObservableValue<Boolean>
>> // - make it part of the Node class; as the fluent bindings
only bind
>> themselves
>> // to their source when needed (lazy binding), this won't create
>> overhead
>> // for each node in the scene
>> }
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the
conclusion
>> that this is much better directly integrated into JavaFX, and I'm
hoping
>> this proof of concept will be able to move such an effort forward.
>>
>> --John
>>
>
