Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Sun, 20 Mar 2022 03:08:59 GMT, Nir Lisker wrote:
>> Both seem fine, I don't have any preference over one or the other.
>
> I struggled with finding a good description here
> [previously](https://github.com/openjdk/jfx/pull/675#discussion_r777801130).
> I think that mstr2 gave a good approach. What we can do if we want to have
> "the best of both worlds" is to write something in this form:
>
> . More precisely, convoluted description>
>
>
> I would offer something like this based on your suggestions:
>
>
> Creates a new {@code ObservableValue} that holds the value of a nested {@code
> ObservableValue} supplied by the
> given mapping function. The result is updated when either this or the nested
> {@code ObservableValue} changes.
> If either this or the nested value is {@code null}, the resulting value is
> {@code null} (no mapping is applied if
> this value is {@code null}).
> More precisely, the created {@code ObservableValue} holds the value of an
> {@code ObservableValue} resulting
> from applying a mapping on this {@code ObservableValue}'s value.
>
> I'm honestly not sure the "More precisely" part is even needed at his point.
> Up to you.
>
> The `@return` description can be changed accordingly with the simplified
> explanation if you think it's clearer.
>
> You can also specify a `@throws` NPE if the mapping function parameter is
> `null` instead of writing "cannot be null", like mstr2 suggested in another
> place if you like this pattern.
>
> By the way, if we change "Creates an..." to "Creates a new..." we should
> change it in the other methods. I don't think there's a difference.
Resolving this one as we have this discussion elsewhere as well.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Mon, 21 Mar 2022 09:01:01 GMT, John Hendrikx wrote:
>> I read this comment after what I wrote about `flatMap`, so mstr2 also had
>> the idea of "More precisely", which is good :)
>>
>> I would suggested something similar to what I did there:
>>
>>
>> Creates a new {@code ObservableValue} that holds the value supplied by the
>> given mapping function. The result
>> is updated when this {@code ObservableValue} changes.
>> If this value is {@code null}...
>> More precisely, the created {@code ObservableValue} holds the result of
>> applying a mapping on this
>> {@code ObservableValue}'s value.
>>
>>
>> Same comments about `@return` and `@throws` NPE as I had for `flatMap`.
>>
>> `orElse` will also becomes something like
>>
>>
>> Creates a new {@code ObservableValue} that holds this value, or the given
>> value if it is {@code null}. The
>> result is updated when this {@code ObservableValue} changes.
>> More precisely, the created {@code ObservableValue} holds this {@code
>> ObservableValue}'s value, or
>> the given value if it is {@code null}.
>>
>>
>> Also not sure if the "More precisely" description is needed here.
>
> @nlisker @mstr2 I've done another pass, using all the suggestions, and also
> by looking closely at how things are worded in the documentation for Optional
> and Stream. This resulted in a few additional changes. I've left out the
> "more precisely" parts as I think we all agree it adds little extra value.
> Please have another look.
I've changed the wording also to `Returns an` instead of `Creates an` or
`Creates a new` -- I think we don't need to guarantee a new instance is always
returned, just an instance that works as described (potentially cached or
re-used). Stream and Optional word this similar.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Sun, 20 Mar 2022 03:28:01 GMT, Nir Lisker wrote:
>> Yeah, agreed, it is a bit annoying to have to deal with the fact that these
>> classes are wrappers around an actual value and having to refer to them as
>> such to be "precise". I'm willing to make another pass at all of these to
>> change the wording. What do you think @nlisker ?
>
> I read this comment after what I wrote about `flatMap`, so mstr2 also had the
> idea of "More precisely", which is good :)
>
> I would suggested something similar to what I did there:
>
>
> Creates a new {@code ObservableValue} that holds the value supplied by the
> given mapping function. The result
> is updated when this {@code ObservableValue} changes.
> If this value is {@code null}...
> More precisely, the created {@code ObservableValue} holds the result of
> applying a mapping on this
> {@code ObservableValue}'s value.
>
>
> Same comments about `@return` and `@throws` NPE as I had for `flatMap`.
>
> `orElse` will also becomes something like
>
>
> Creates a new {@code ObservableValue} that holds this value, or the given
> value if it is {@code null}. The
> result is updated when this {@code ObservableValue} changes.
> More precisely, the created {@code ObservableValue} holds this {@code
> ObservableValue}'s value, or
> the given value if it is {@code null}.
>
>
> Also not sure if the "More precisely" description is needed here.
@nlisker @mstr2 I've done another pass, using all the suggestions, and also by
looking closely at how things are worded in the documentation for Optional and
Stream. This resulted in a few additional changes. I've left out the "more
precisely" parts as I think we all agree it adds little extra value. Please
have another look.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Sun, 20 Mar 2022 03:28:01 GMT, Nir Lisker wrote:
>> Yeah, agreed, it is a bit annoying to have to deal with the fact that these
>> classes are wrappers around an actual value and having to refer to them as
>> such to be "precise". I'm willing to make another pass at all of these to
>> change the wording. What do you think @nlisker ?
>
> I read this comment after what I wrote about `flatMap`, so mstr2 also had the
> idea of "More precisely", which is good :)
>
> I would suggested something similar to what I did there:
>
>
> Creates a new {@code ObservableValue} that holds the value supplied by the
> given mapping function. The result
> is updated when this {@code ObservableValue} changes.
> If this value is {@code null}...
> More precisely, the created {@code ObservableValue} holds the result of
> applying a mapping on this
> {@code ObservableValue}'s value.
>
>
> Same comments about `@return` and `@throws` NPE as I had for `flatMap`.
>
> `orElse` will also becomes something like
>
>
> Creates a new {@code ObservableValue} that holds this value, or the given
> value if it is {@code null}. The
> result is updated when this {@code ObservableValue} changes.
> More precisely, the created {@code ObservableValue} holds this {@code
> ObservableValue}'s value, or
> the given value if it is {@code null}.
>
>
> Also not sure if the "More precisely" description is needed here.
I think I've arrived at the conclusing that the "more precisely" description is
basically rehashing the semantics of `ObservableValue` over and over again.
It should be clear for a reader who is familiar with ObservableValue that we're
not suggesting that somehow the given ObservableValue _instance_ is stored in
the new ObservableValue.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 09:55:30 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
>> line 146:
>>
>>> 144: * Creates an {@code ObservableValue} that holds the result of
>>> applying a
>>> 145: * mapping on this {@code ObservableValue}'s value. The result is
>>> updated
>>> 146: * when this {@code ObservableValue}'s value changes. If this
>>> value is
>>
>> I think a lot of the new documentation in this class sacrifices
>> understandability for precision in trying to deal with the difference
>> between "this ObservableValue" and "this ObservableValue's value".
>>
>> However, my feeling is that that's not helping users who are trying to
>> understand the purpose of the new APIs.
>> What do you think about a simplified version like this:
>> `Creates a new {@ObservableValue} that applies a mapping function to this
>> {@code ObservableValue}. The result is updated when this {@code
>> ObservableValue} changes.`
>>
>> Sure, it's not literally mapping _this ObservableValue instance_, but would
>> this language really confuse readers more that the precise language?
>>
>> Another option might be to combine both:
>> `Creates a new {@ObservableValue} that applies a mapping function to this
>> {@code ObservableValue}. More precisely, it creates a new {@code
>> ObservableValue} that holds the result of applying a mapping function to the
>> value of this {@code ObservableValue}.`
>
> Yeah, agreed, it is a bit annoying to have to deal with the fact that these
> classes are wrappers around an actual value and having to refer to them as
> such to be "precise". I'm willing to make another pass at all of these to
> change the wording. What do you think @nlisker ?
I read this comment after what I wrote about `flatMap`, so mstr2 also had the
idea of "More precisely", which is good :)
I would suggested something similar to what I did there:
Creates a new {@code ObservableValue} that holds the value supplied by the
given mapping function. The result
is updated when this {@code ObservableValue} changes.
If this value is {@code null}...
More precisely, the created {@code ObservableValue} holds the result of
applying a mapping on this
{@code ObservableValue}'s value.
Same comments about `@return` and `@throws` NPE as I had for `flatMap`.
`orElse` will also becomes something like
Creates a new {@code ObservableValue} that holds this value, or the given value
if it is {@code null}. The
result is updated when this {@code ObservableValue} changes.
More precisely, the created {@code ObservableValue} holds this {@code
ObservableValue}'s value, or
the given value if it is {@code null}.
Also not sure if the "More precisely" description is needed here.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 23:55:36 GMT, Michael Strauß wrote:
>> I've changed this to use your wording as I think it does read much better.
>>
>> Perhaps also possible:
>>
>> Creates a new {@code ObservableValue} that holds the value of a nested
>> {@code ObservableValue} supplied
>> by the given mapping function.
>>
>> ?
>
> Both seem fine, I don't have any preference over one or the other.
I struggled with finding a good description here
[previously](https://github.com/openjdk/jfx/pull/675#discussion_r777801130). I
think that mstr2 gave a good approach. What we can do if we want to have "the
best of both worlds" is to write something in this form:
. More precisely,
I would offer something like this based on your suggestions:
Creates a new {@code ObservableValue} that holds the value of a nested {@code
ObservableValue} supplied by the
given mapping function. The result is updated when either this or the nested
{@code ObservableValue} changes.
If either this or the nested value is {@code null}, the resulting value is
{@code null} (no mapping is applied if
this value is {@code null}).
More precisely, the created {@code ObservableValue} holds the value of an
{@code ObservableValue} resulting
from applying a mapping on this {@code ObservableValue}'s value.
I'm honestly not sure the "More precisely" part is even needed at his point. Up
to you.
The `@return` description can be changed accordingly with the simplified
explanation if you think it's clearer.
You can also specify a `@throws` NPE if the mapping function parameter is
`null` instead of writing "cannot be null", like mstr2 suggested in another
place if you like this pattern.
By the way, if we change "Creates an..." to "Creates a new..." we should change
it in the other methods. I don't think there's a difference.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 10:17:01 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
>> line 197:
>>
>>> 195: /**
>>> 196: * Creates an {@code ObservableValue} that holds the value of an
>>> {@code ObservableValue}
>>> 197: * resulting from applying a mapping on this {@code
>>> ObservableValue}'s value. The result
>>
>> While technically correct, I think the first sentence should focus more on
>> the purpose of this method.
>>
>> How about something like this:
>> `Creates a new {@code ObservableValue} that holds the value of a nested
>> {@code ObservableValue} by applying a mapping function to extract the nested
>> {@code ObservableValue}.`
>>
>> That's not as precise, but it makes the purpose much more clear.
>
> I've changed this to use your wording as I think it does read much better.
>
> Perhaps also possible:
>
> Creates a new {@code ObservableValue} that holds the value of a nested
> {@code ObservableValue} supplied
> by the given mapping function.
>
> ?
Both seem fine, I don't have any preference over one or the other.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 09:32:18 GMT, John Hendrikx wrote: >> modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java >> line 68: >> >>> 66: }; >>> 67: } >>> 68: } >> >> Several files are missing newlines after the last closing brace. Do we >> enforce this? >> >> Also, if there's a newline after the first line of a class declaration, >> shouldn't there also be a newline before the last closing brace? > > Let me add those new lines at the end of files (everywhere) as Github is also > flagging it with an ugly red marker. I tend to unconsciously remove them > myself on longer files as it looks weird in editors to have an unused line at > the bottom. > > As for the newline before the last closing brace, that doesn't seem to be > done a lot in the current code base. I've added those newlines at the top as > it seems fairly consistent in the code base, although I'm not a fan as I use > empty lines only to separate things when there is no clear separation already > (like an opening brace). I don't think jcheck checks for newlines anywhere. Usually the style that I see is a newline after the definition of the class and at the end of the file (sometimes), but not before the last closing brace. - PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 09:48:39 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java
>> line 67:
>>
>>> 65: */
>>> 66: default Subscription and(Subscription other) {
>>> 67: Objects.requireNonNull(other);
>>
>> This exception could be documented with `@throws NullPointerException if
>> {@code other} is null`
>
> I've updated the docs a bit -- it hasn't received much attention because this
> is not going to be API for now
Yes, in "phase 2" when this class is made public there will be a proper docs
review.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Thu, 17 Mar 2022 20:09:23 GMT, Michael Strauß wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Process review comments (2)
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java
> line 40:
>
>> 38: *
>> 39: * For example:
>> 40: * Subscription s = property.subscribe(System.out::println)
>
> I believe our recommended pattern for example code is:
>
> {@code
> ...
> }
Fixed this everywhere.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java
> line 67:
>
>> 65: */
>> 66: default Subscription and(Subscription other) {
>> 67: Objects.requireNonNull(other);
>
> This exception could be documented with `@throws NullPointerException if
> {@code other} is null`
I've updated the docs a bit -- it hasn't received much attention because this
is not going to be API for now
> modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java
> line 68:
>
>> 66: };
>> 67: }
>> 68: }
>
> Several files are missing newlines after the last closing brace. Do we
> enforce this?
>
> Also, if there's a newline after the first line of a class declaration,
> shouldn't there also be a newline before the last closing brace?
Let me add those new lines at the end of files (everywhere) as Github is also
flagging it with an ugly red marker. I tend to unconsciously remove them
myself on longer files as it looks weird in editors to have an unused line at
the bottom.
As for the newline before the last closing brace, that doesn't seem to be done
a lot in the current code base. I've added those newlines at the top as it
seems fairly consistent in the code base, although I'm not a fan as I use empty
lines only to separate things when there is no clear separation already (like
an opening brace).
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 146:
>
>> 144: * Creates an {@code ObservableValue} that holds the result of
>> applying a
>> 145: * mapping on this {@code ObservableValue}'s value. The result is
>> updated
>> 146: * when this {@code ObservableValue}'s value changes. If this value
>> is
>
> I think a lot of the new documentation in this class sacrifices
> understandability for precision in trying to deal with the difference between
> "this ObservableValue" and "this ObservableValue's value".
>
> However, my feeling is that that's not helping users who are trying to
> understand the purpose of the new APIs.
> What do you think about a simplified version like this:
> `Creates a new {@ObservableValue} that applies a mapping function to this
> {@code ObservableValue}. The result is updated when this {@code
> ObservableValue} changes.`
>
> Sure, it's not literally mapping _this ObservableValue instance_, but would
> this language really confuse readers more that the precise language?
>
> Another option might be to combine both:
> `Creates a new {@ObservableValue} that applies a mapping function to this
> {@code ObservableValue}. More precisely, it creates a new {@code
> ObservableValue} that holds the result of applying a mapping function to the
> value of this {@code ObservableValue}.`
Yeah, agreed, it is a bit annoying to have to deal with the fact that these
classes are wrappers around an actual value and having to refer to them as such
to be "precise". I'm willing to make another pass at all of these to change
the wording. What do you think @nlisker ?
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 153:
>
>> 151: *
>> 152: * var text = new SimpleStringProperty("abcd");
>> 153: * ObservableValueString upperCase =
>> text.map(String::toUpperCase);
>
> Escaping `<` and `>` is not necessary if the code block is wrapped in
> `{@code}`
Also fixed everywhere
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 197:
>
>> 195: /**
>> 196: * Creates an {@code ObservableValue} that holds the value of an
>> {@code ObservableValue}
>> 197: * resulting from applying a mapping on this {@code
>> ObservableValue}'s value. The result
>
> While technically correct, I think the first sentence should focus more on
> the purpose of this method.
>
> How about something like this:
> `Creates a new {@code ObservableValue} that holds the value of a nested
> {@code ObservableValue} by applying a mapping function to extract the nested
> {@code ObservableValue}.`
>
> That's not as precise, but it makes the purpose much more clear.
I've changed this to use your wording as I think it does read much better.
Perhaps also possible:
Creates a new {@code ObservableValue} that holds the value of a nested
{@code ObservableValue} supplied
by the given mapping function.
?
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 227:
>
>>
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Thu, 10 Mar 2022 17:49:38 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process review comments (2)
The code looks good. I've left some inline comments.
modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line
40:
> 38: *
> 39: * For example:
> 40: * Subscription s = property.subscribe(System.out::println)
I believe our recommended pattern for example code is:
{@code
...
}
modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line
67:
> 65: */
> 66: default Subscription and(Subscription other) {
> 67: Objects.requireNonNull(other);
This exception could be documented with `@throws NullPointerException if {@code
other} is null`
modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java
line 68:
> 66: };
> 67: }
> 68: }
Several files are missing newlines after the last closing brace. Do we enforce
this?
Also, if there's a newline after the first line of a class
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Thu, 10 Mar 2022 17:49:38 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process review comments (2)
@mstr2 Could you perhaps review this PR as well?
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Thu, 10 Mar 2022 17:49:38 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process review comments (2)
Re-approving
-
Marked as reviewed by nlisker (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
> This is an implementation of the proposal in
> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
> (@nlisker) have been working on. It's a complete implementation including
> good test coverage.
>
> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
> API footprint. Compared to the PoC this is lacking public API for
> subscriptions, and does not include `orElseGet` or the `conditionOn`
> conditional mapping.
>
> **Flexible Mappings**
> Map the contents of a property any way you like with `map`, or map nested
> properties with `flatMap`.
>
> **Lazy**
> The bindings created are lazy, which means they are always _invalid_ when not
> themselves observed. This allows for easier garbage collection (once the last
> observer is removed, a chain of bindings will stop observing their parents)
> and less listener management when dealing with nested properties.
> Furthermore, this allows inclusion of such bindings in classes such as `Node`
> without listeners being created when the binding itself is not used (this
> would allow for the inclusion of a `treeShowingProperty` in `Node` without
> creating excessive listeners, see this fix I did in an earlier PR:
> https://github.com/openjdk/jfx/pull/185)
>
> **Null Safe**
> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
> when the value they would be mapping is `null`. This makes mapping nested
> properties with `flatMap` trivial as the `null` case does not need to be
> taken into account in a chain like this:
> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
> Instead a default can be provided with `orElse`.
>
> Some examples:
>
> void mapProperty() {
> // Standard JavaFX:
> label.textProperty().bind(Bindings.createStringBinding(() ->
> text.getValueSafe().toUpperCase(), text));
>
> // Fluent: much more compact, no need to handle null
> label.textProperty().bind(text.map(String::toUpperCase));
> }
>
> void calculateCharactersLeft() {
> // Standard JavaFX:
>
> label.textProperty().bind(text.length().negate().add(100).asString().concat("
> characters left"));
>
> // Fluent: slightly more compact and more clear (no negate needed)
> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + "
> characters left"));
> }
>
> void mapNestedValue() {
> // Standard JavaFX:
> label.textProperty().bind(Bindings.createStringBinding(
> () -> employee.get() == null ? ""
> : employee.get().getCompany() == null ? ""
> : employee.get().getCompany().getName(),
> employee
> ));
>
> // Fluent: no need to handle nulls everywhere
> label.textProperty().bind(
> employee.map(Employee::getCompany)
> .map(Company::getName)
> .orElse("")
> );
> }
>
> void mapNestedProperty() {
> // Standard JavaFX:
> label.textProperty().bind(
> Bindings.when(Bindings.selectBoolean(label.sceneProperty(), "window",
> "showing"))
> .then("Visible")
> .otherwise("Not Visible")
> );
>
> // Fluent: type safe
> label.textProperty().bind(label.sceneProperty()
> .flatMap(Scene::windowProperty)
> .flatMap(Window::showingProperty)
> .orElse(false)
> .map(showing -> showing ? "Visible" : "Not Visible")
> );
> }
>
> Note that this is based on ideas in ReactFX and my own experiments in
> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
> that this is much better directly integrated into JavaFX, and I'm hoping this
> proof of concept will be able to move such an effort forward.
John Hendrikx has updated the pull request incrementally with one additional
commit since the last revision:
Process review comments (2)
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/675/files
- new: https://git.openjdk.java.net/jfx/pull/675/files/29dc2af7..8f9bf897
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=675=09
- incr: https://webrevs.openjdk.java.net/?repo=jfx=675=08-09
Stats: 7 lines in 2 files changed: 5 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jfx/pull/675.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/675/head:pull/675
PR: https://git.openjdk.java.net/jfx/pull/675
