Proof of concept for fluent bindings for ObservableValue

John Hendrikx hjohn at xs4all.nl
Sun Sep 19 15:11:29 UTC 2021


I need to get you the tests I wrote, unfortunately, they're JUnit 5, 
please see the tests here:

https://github.com/hjohn/MediaSystem-v2/tree/master/mediasystem-jfx/src/test/java/javafx/beans/value

The OrElseBinding is incorrect, the compute value should read:

     protected T computeValue() {
       T value = source.getValue();
       return value == null ? constant : value;
     }

Similar for OrElseGetBinding.

I can rewrite the tests for JUnit 4 but I'd need a Nested runner (I 
think the tests would become pretty unreadable and less useful / 
thourough otherwise).

What are the options?

- Integrate a nested runner (there is an Apache 2.0 licensed one available)
- Create our own nested runner (about 200 lines of code)
- Can we introduce JUnit 5?
- Rewrite to plain JUnit 4?

Let me know, and I can integrate them.

--John

On 19/09/2021 02:12, Nir Lisker wrote:
> I've played around with the implementation and pushed a modified
> prototype to the sandbox [1]. I ended up with something similar to ReactFX:
> the orElse and orElseGet methods have their own binding classes that
> extend LazyObjectBinding, just like MapBinding and FlatMapBinding. The
> reason being that both their compute value and their subscription models
> are slightly different. While they can be matched to MapBinding like you
> did, it entails a bit of a roundabout way in my opinion. Creating a
> supplier for the constant value of orElse somewhat defeats the purpose I
> think.

I have no strong opinion here, just wanted to keep the MR small. The 
supplier should be an inline candidate, but creating a separate class is 
fine too.

As for the subscription model: flat map has a more complicated one, but 
orElse, orElseGet and map all have the same one.

> In addition, I think that it's fine to move the arguments' null checks to
> the binding classes themselves, even if it's a couple of levels deeper on
> the stack, while adding a message in the 2nd argument of
> Objects.requireNonNull for clarity. These classes are "self-contained" so
> it makes sense for them to check their arguments. They might be used in
> other places, perhaps in the public Bindings class.

The messages will need to be written from the perspective of the Binding 
class then IMHO.

> I also moved the subscription-related methods from ObservableValue to
> static methods in Subscription, at least for now, to defer the API related
> to Subscription.

Sounds good.

> The branch doesn't compile because I didn't finish working on the
> visibility issue, but it's close enough to what I envision it like for the
> first PR.

I've ported the changes over to my branch and ran the tests on it, they 
all pass apart from the above mentioned problem in the OrElse bindings.

> As for the java,util.Optional methods, I indeed did something like
> `x.orElse(5).getValue()` in the past in other cases, but this approach
> creates a new binding just to get the wrapped value out, which is very
> inefficient. (In one case I did booleanValue.isNull().get(), which creates
> a boolean binding, because isPresent() does not exist). I would like to see
> what others think about the Optional methods, The binding method variants
> are much more important, but I want to avoid a name clash if the Optional
> ones will have support.

Okay, some more things to consider:

ObservableValue is not an Optional, their get methods respond 
differently with Optional#get throwing an exception when null -- the 
#orElse is a necessity; this is not the case for ObservableValue#getValue.

When creating mapping chains, you are going to do this because you want 
to bind this to another property or to listen on it (with subscribe). 
You don't want to do this as a one time thing. If you are creating a 
chain just to calculate a value you can just do this directly. Instead of:

     widthProperty().map(x -> x * 2).getValue()

You'd do:

     getWidth() * 2;

With flat mapping however this is not as easy:

     [1]
     node.sceneProperty()
       .flatMap(Scene::windowProperty)
       .flatMap(Window::showingProperty)
       .orElse(false);

You could try:

     node.getScene().getWindow().isShowing();

But that will not work when Scene or Window is null.  You'd have to 
write it as:

     [2]
     Optional.ofNullable(node.getScene())
        .map(Scene::getWindow)
        .map(Window::isShowing)
        .orElse(false);

If you only offer a "specialized" orElse, you'd still need to create 
several bindings:

     [3]
     node.sceneProperty()
       .flatMap(Scene::windowProperty)
       .flatMap(Window::showingProperty)
       .orElse2(false);  // orElse which returns a value not a binding

If you compare [2] (which is possible now) with [3] the difference is 
minimal. A bit more ceremony at the start for [2] but a shorter, cleaner 
and faster mapping (no bindings and no need to use the property methods).

Now, if you already HAVE the binding for some other purpose, then it is 
highly likely it also already has an #orElse that is needed as part of 
the binding. In that case calling #getValue is fine without much need 
for another #orElse variant.

--John

>
> [1]
> https://github.com/openjdk/jfx-sandbox/tree/jfx-sandbox/nlisker_fuent_bindings
>
> On Wed, Sep 15, 2021 at 1:59 PM John Hendrikx <hjohn at xs4all.nl> wrote:
>
>>
>>
>> On 15/09/2021 02:28, Nir Lisker wrote:
>>>     Sorry, I'm not quite sure what you mean by this. Could you elaborate?
>>>     The methods orElse and orElseGet are present in the PoC, and I think
>>>     they're highly useful to have to deal with nulls.
>>>
>>>
>>> The methods that you call orElse and orElseGet return an
>>> ObservableValue<T>. The Optional methods with the same names return the
>>> wrapped value (of type T). For comparison, ReactFX has:
>>> T getOrElse(T defaultValue)
>>> T getOrSupply(Supplier<? extends T> defaultSupplier)
>>> Val<T> orElseConst(T other)
>>> Val<T> orElse(ObservableValue<T> other)
>>
>> I see what you mean now. The methods from java.util.Optional will return
>> an unwrapped value, while the ones from ObservableValue in the PoC
>> return an ObservableValue<T>, but they have the same name.
>>
>> So java.util.Optional offers:
>>
>>      T orElse(T other)
>>      T orElseGet(Supplier<? extends T> supplier)
>>
>> And the PoC:
>>
>>      ObservableValue<T> orElse(T alternativeValue)
>>      ObservableValue<T> orElseGet(Supplier<? extends T> supplier)
>>
>> The main difference is in the returned types. Personally, I think it is
>> rarely useful for a binding to be queried directly and I've never used
>> the #getOrElse or #getOrSupply variants that ReactFX offers. On top of
>> that:
>>
>>      x.orElse(5).getValue()    ===    x.getOrElse(5)
>>
>> So it introduces another method in the interface to avoid having to
>> write ".getValue()". The opposite, introducing only the "unwrapped"
>> variants would still require the "wrapped" variants to be present as well.
>>
>> So, what I would suggest is to not add variants for #getOrElse and
>> #getOrSupply at all as they are of questionable value and would just
>> bloat the API for a bit less typing. In that case I think we can still
>> use the signatures as they are.
>>
>> ReactFX also offers:
>>
>>      Val<T> orElse(ObservableValue<T> other)
>>
>> This is another rarely used feature IMHO, and I think Optional#or would
>> a better match for this functionality:
>>
>>      Optional<T> or(Supplier<? extends Optional<? extends T>> supplier)
>>
>> In the POC the signature would be:
>>
>>      ObservableValue<T> or(
>>        Supplier<? extends ObservableValue<? extends T>> supplier
>>      )
>>
>> I didn't implement this one in the PoC to keep it small, but I did
>> implement this in my JavaFX EventStream library before.
>>
>>> So it deals with both getting the wrapped value in null cases and with
>>> getting a "dynamic value" in null cases. I find the Optional-like method
>>> 'T getOrElse(T defaultValue)' useful (along with others such as
>>> ifPresent because Optional is just useful for dealing with wrapped
>>> values). What I'm saying is that we should be careful with the names if
>>> we include both "constant" and "dynamic" versions of 'orElse'-like
>> methods.
>>
>> I think #ifPresent can be added, not so sure about the usefulness of
>> #getOrElse (see above).
>>
>>>     The null check in ReactFX's #computeValue is
>>>     actually checking the result of the mapping function, not whether the
>>>     function instance itself was null.
>>>
>>> I didn't mean the null-ness of the map argument. What I meant was that
>>> there is this implementation, which is similar to what ReactFX does:
>>
>> Sorry, I see it now. You have a good point, the current implementation
>> indeed wraps another Lambda to add the null check which could have been
>> done in #computeValue. I think it would be a good move to avoid the
>> extra lambda simply because the end result would be more readable -- any
>> performance improvement would be a bonus, I don't know if there will be
>> any.
>>
>>> As for my points 3 and 4, I'll have to try and play with the
>>> implementation myself, which will be easier to do when there is some PR
>>> in the works.
>>
>> Perhaps this is useful:
>> https://github.com/hjohn/MediaSystem-v2/tree/master/mediasystem-jfx
>>
>> When you add this as a maven dependency to your project, you will get
>> the new PoC functionality. It basically uses the Maven shade plugin to
>> replace a few classes in javafx-base -- I use this sometimes to fix bugs
>> I need fixed immediately by patching jfx, but found it also works very
>> nicely to experiment with this PoC.
>>
>> Also, the PoC PR compiles fine, it may need rebasing.
>>
>>>     To close "Bindings.select*(): add new type-safe template based API
>>>     instead of legacy-style set of methods" we'd need the flatMap/select
>>>     method to be included.
>>>
>>> Yes. I think that we can include flatMap in this iteration, perhaps as
>>> a separate PR?
>>
>> That should be no problem, I can split it up.
>>
>>>     I don't think we really need specialized methods for primitives (or
>> at
>>>     least, not right away).  At this point the primitive versions only
>>>     really differ in what value they'd return if the binding would be
>> null
>>>     and perhaps they do a little less boxing/unboxing. Since you can
>> select
>>>     the default value with #orElse which is more flexible. I don't see
>> much
>>>     use to add those variants.
>>>
>>> I agree, I would avoid bloating the primitive specialization classes for
>>> now, especially when Valhalla is planned to take care of those.
>>
>> Yes, I think we can easily do without for now.
>>
>>>     The ticket is a bit unclear as I can't see what type "x" is.
>>>
>>> Yes, but I got the impression that either way it can be solved with map
>>> (or flatMap).
>>
>> Agreed.
>>
>> --John
>>
>


More information about the openjfx-dev mailing list