[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: Re: Proof of concept for fluent bindings for ObservableValue
From:       Eric Bresie <ebresie () gmail ! com>
Date:       2021-09-26 19:40:53
Message-ID: CAHw6w1Q4-3HWsHWwEUG-RZVqPFYsff+t5Fe7AhSqZENuYK3+yA () mail ! gmail ! com
[Download RAW message or body]

I'm no expert here so take it with a grain of salt  but…

I was proposing moving this sorts of ObservableValue type interfaces  and
implementations out of jfx (and React) context and into JDK context. Then
these sorts of observable type properties/ bindings can be used outside of
just UI contexts.

Eric

On Tue, Sep 21, 2021 at 12:27 PM Nir Lisker <nlisker@gmail.com> wrote:

> I'm not sure what you're proposing. Move what, to where, and for what use?
>
> On Tue, Sep 21, 2021 at 8:02 PM Eric Bresie <ebresie@gmail.com> wrote:
>
>> I'm very much an Observer here ;-) but
>>
>> Given the comparisons with FX and React implementations, is there value
>> in moving some of this out of here and into the JDK proper context making
>> it potentially usable outside of fx or react circles?
>>
>> I'm reminded of old JDK workings when someone might be working a headless
>> application needing a Swing dependency.
>>
>> Eric Bresie
>> Ebresie@gmail.com (mailto:Ebresie@gmail.com)
>
>
>>
>> > On September 21, 2021 at 5:36:13 AM CDT, Nir Lisker <nlisker@gmail.com
>> (mailto:nlisker@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@xs4all.nl (mailto:
>> hjohn@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@xs4all.nl
>> (mailto:hjohn@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
>> > > > >
>> > > >
>> > >
>>
> --
Eric Bresie
ebresie@gmail.com
[prev in list] [next in list] [prev in thread] [next in thread]