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 <hj...@xs4all.nl
<mailto:hj...@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 <nlis...@gmail.com
<mailto:nlis...@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 <hj...@xs4all.nl
<mailto:hj...@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 <hj...@xs4all.nl
<mailto:hj...@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
>>>>>
>>>>
>>>
>>
>
