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