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
