I’m no expert here so take it with a grain of salt but… I was proposing moving this sorts of ObservableValue type interfaces and implementations out of jfx (and React) context and into JDK context. Then these sorts of observable type properties/ bindings can be used outside of just UI contexts.
Eric On Tue, Sep 21, 2021 at 12:27 PM Nir Lisker <nlis...@gmail.com> wrote: > 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 >> > > > > >> > > > >> > > >> > -- Eric Bresie ebre...@gmail.com
