Proof of concept for fluent bindings for ObservableValue
Nir Lisker
nlisker at gmail.com
Tue Oct 5 15:58:00 UTC 2021
Looks good. I assume we will add more bindings in the future like
conditionOn or anything else that I would put under "extras", so maybe the
title could be more specific since there will be more fluent bindings in
the future?
On Tue, Oct 5, 2021 at 1:34 PM John Hendrikx <hjohn at xs4all.nl> wrote:
> I've created https://bugs.openjdk.java.net/browse/JDK-8274771
>
> Please have a look.
>
> --John
>
> On 04/10/2021 17:51, Nir Lisker wrote:
> > I think that a PR can be created. The only point we need to decide is
> about
> > the subscription models we talked about above. ReactFX uses something
> > different for each, you use the same. That can determine if we need
> > different classes for each binding type.
> >
> > I can create the JBS issue for this one and a draft CSR if you want.
> >
> > On Tue, Sep 21, 2021 at 1:36 PM Nir Lisker <nlisker at gmail.com> wrote:
> >
> >> The OrElseBinding is incorrect
> >>>
> >>
> >> Yes, that was a typo with the order of the arguments in the ternary
> >> statement.
> >>
> >> 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?
> >>>
> >>
> >> This is a bigger question. Kevin will have to answer that.
> >>
> >> As for the subscription model: flat map has a more complicated one, but
> >>> orElse, orElseGet and map all have the same one.
> >>>
> >>
> >> From what I saw, ReactFX uses a different subscription model for these.
> I
> >> could have misread it.
> >>
> >> The messages will need to be written from the perspective of the Binding
> >>> class then IMHO.
> >>>
> >>
> >> That's fine.
> >>
> >> As for the Optional methods, I'll have a look in my code base and see if
> >> the places I would like to use them at will become irrelevant anyway
> with
> >> the fluent bindings. I'm fine with proceeding with the current names of
> the
> >> proposed methods.
> >>
> >> On Sun, Sep 19, 2021 at 6:12 PM John Hendrikx <hjohn at xs4all.nl> wrote:
> >>
> >>> 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