Proof of concept for fluent bindings for ObservableValue

[John Hendrikx]

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