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 <ebre...@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 > ebre...@gmail.com (mailto:ebre...@gmail.com) > > > On September 21, 2021 at 5:36:13 AM CDT, 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 > > > > > > > > > > > > >
