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