Proof of concept for fluent bindings for ObservableValue

[Nir Lisker]

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.

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.

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.

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.

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.

[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
>