Re: Proof of concept for fluent bindings for ObservableValue
>
> In the PoC I made I specifically also disallowed 'null' as an input
>
I like the way ReactFX does it where the property is empty. I think that
this is also what you mean by disallowing `null` (in other contexts,
"disallowing null" would mean throwing an exception).
Not entirely sure what you mean by this.
>
Basically, what you said. My point was that this is a different API
section. The first deals with expanding the observables/properties methods.
The second with listeners methods. Even if mapping a property requires a
new listening model, like subscriptions, this is done under the hood.
Exposing this API should be a separate step. At least that's how I see it.
I'd be happy to spend more time and work on this. Perhaps it would be
> possible to collaborate on this?
>
That would be good. I will need to re-review the ReactFX internals and see
how your proposal differs exactly.
By the way, do you make a distinction between ReactFX's Val and Var in
your proposal (one being read-only)?
On Sun, Apr 4, 2021 at 12:43 PM John Hendrikx wrote:
>
>
> On 02/04/2021 08:47, Nir Lisker wrote:
> > Hi John,
> >
> > I've had my eyes set on ReactFX enhancements for a while too, especially
> as
> > a replacement for the unsafe "select" mechanism. One of the things that
> > kept me from going forward with this is seeing what Valhalla will bring.
> > Generic specialization might save a lot of duplication work on something
> > like this, and Tomas touched another related issue [1], but since it
> could
> > be a long time before that happens, it's worth planning what we can
> extract
> > from ReactFX currently.
>
> Agreed, Valhalla is certainly a highly anticipated feature but I fear it
> is still a couple of years away.
>
> Even without any initial support for dealing with "? extends Number"
> from the various ObservableValue specializations I think looking into
> this can already be tremendous help.
>
> The proof of concept mainly requires you convert the Number to a
> suitable type when reading the property but has no problems in the other
> direction:
>
> label.widthProperty().map(Number::doubleValue).map(x -> x + 1);
>
> Not pretty, but certainly workable. Specific methods could be introduced
> (even at a later time) to make this more streamlined, similar to what
> the Stream API offers with 'mapToDouble' etc.
>
> > I think that we should break the enhancements into parts.
> > The first that I would advise to look at are the additions to
> > properties/observables. Tomas had to create Val and Var because he
> couldn't
> > change the core interfaces, but we can. Fitting them with the Optional
> > methods like `isPresent`, `isEmpty`, `ifPresent`, `map`. `flatMap` etc.;
> > and `select` and friends, is already a good start that will address many
> > common requirements.
>
> Yes, Val/Var had to be created for that reason, and also because
> properties don't quite behave the same as streams -- streams with a
> "toBinding" method results in things people didn't quite expect.
>
> As far as the Optional methods go, I'm not entirely sure properties
> would benefit from all of them. Properties are not immutable like
> Optional and it may make less sense to fit them with 'isPresent',
> 'isEmpty' and 'ifPresent' ('ifPresent' would I think need to behave
> similar to 'addListener' or 'subscribe').
>
> In the PoC I made I specifically also disallowed 'null' as an input for
> functions like 'map' and 'flatMap' (opting to use 'orElse' semantics for
> 'null'), as this for allows much cleaner mapping (and especially flat
> mapping when selecting nested properties). If 'null' were to be allowed,
> I think at a minimum we'd need to add another method to allow for easy
> selecting of nested properties to avoid:
>
> obs.flatMap(x -> x == null ? null : x.otherProperty())
>
> > The second part is related to listeners. The subscription model and event
> > streams try to solve the memory issues with hard and weak references, and
> > allow better composition.
>
> Not entirely sure what you mean by this. JavaFX's current model uses
> weak references which was I think an unfortunate decision as it can
> result in huge confusion. For example, a direct binding will work, but
> with an indirection step a binding stops working:
>
> button.textProperty()
> .concat("World") // weak binding used here
> .addListener((obs, old, cur) -> System.out.println(cur));
>
> The above stops working, but without the 'concat' it keeps working.
>
> I think the use of weak listeners should be avoided and instead other
> mechanisms should be provided to make cleaning up easier. This is the
> main reason for 'conditionOn' and why ReactFX even had a specialized
> version of it: 'conditionOnShowing(Node)'.
>
> > The third part is for collections - things like transformation lists
> > (LiveList) and for other collections.
>
> This is indeed best saved for last. The problems there I think are less
> of an issue for now.
>
> > Since
Re: RFR: 8217955: Problems with touch input and JavaFX 11
On Mon, 5 Apr 2021 22:25:55 GMT, Kevin Rushforth wrote: >> This PR filters out `GDK_TOUCH_EVENT_MASK` from `GDK_ALL_EVENTS_MASK` to >> prevent touch events from being used instead of regular mouse events on >> Linux platforms. Note that the touch events will be delivered as mouse >> pressed/dragged events. >> >> Since we still compile with GTK 2.x the bit flag (that was introduced in GTK >> 3.4) needs to be defined. >> >> This PR has been tested on Ubuntu 20.04 with a touch enabled display. > > @pankaj-bansal It looks OK to me, but can you also take a look at this? If anyone would implement touch events, the change would need to be rolled back? If you catch and consume touch events, would it also work? I don't own a touch device and `GTK_DEBUG=touchscreen` seems to work only with gtk signals (not gdk events). - PR: https://git.openjdk.java.net/jfx/pull/457
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button [v2]
On Tue, 6 Apr 2021 16:01:15 GMT, Kevin Rushforth wrote: >> Marius Hanl has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8263807: Code review changes > > Marked as reviewed by kcr (Lead). > Interesting enough I don't need to enable Github actions, they are already > enabled for all other branches I pushed. I think because I forked OpenJFX > after you added them. Yes, if you fork after GitHub actions were added, then no additional action should be needed. - PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button [v2]
On Tue, 6 Apr 2021 15:43:43 GMT, Marius Hanl wrote: >> When DialogPane#getButtonTypes().setAll() is called twice with the same >> argument(s), DialogPane#lookupButton does not return the node which is shown >> inside the button bar. >> This is due DialogPane adding two list change listeners to 'buttons' >> (#getButtonTypes). They have the wrong order, which will result in the >> button bar not changing at all and the 'buttonNodes' list will recreate the >> dialog button(s). >> Finally, this will make DialogPane#lookupButton returning the 'wrong' >> button, which is in fact not used inside the dialog button bar. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > 8263807: Code review changes Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button [v2]
On Tue, 6 Apr 2021 12:18:49 GMT, Kevin Rushforth wrote: >> Marius Hanl has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8263807: Code review changes > > The fix and test look fine. There is one needed change as noted below. > > > Thanks for the explanation. I'll finish my review shortly. > > > Isn't jcheck running all the tests anyway? > > You mean GitHub Actions (jcheck doesn't build or run any tests). It would, > but for two things: > > 1. You used the `master` branch of your repo, which is not recommended, > and will cause problems for you if you ever do another pull request. See > [this > comment](https://github.com/openjdk/jfx/pull/432#issuecomment-801982830) > added by the Skara bot. > > 2. You haven't enabled GitHub action on your personal fork of the `jfx` > repository. See > [wiki.openjdk.java.net/display/SKARA/Testing](https://wiki.openjdk.java.net/display/SKARA/Testing) > for information on how to enable it. Oh okay, thanks for the clarification. Interesting enough I don't need to enable Github actions, they are already enabled for all other branches I pushed. I think because I forked OpenJFX after you added them. But as you mentioned they are not running for master. I pushed a separate branch based of master (so only this 2 commits made by me), the Github actions were successful: https://github.com/Maran23/jfx/actions/runs/723063232 - PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button [v2]
On Tue, 6 Apr 2021 12:06:05 GMT, Kevin Rushforth wrote: >> Marius Hanl has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8263807: Code review changes > > modules/javafx.controls/src/test/java/test/javafx/scene/control/DialogPaneTest.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2021, 2021, Oracle and/or its affiliates. All rights >> reserved. > > You need to revert this change. There must be only a single year listed if > the start year and last modified year are the same. done - PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button [v2]
> When DialogPane#getButtonTypes().setAll() is called twice with the same > argument(s), DialogPane#lookupButton does not return the node which is shown > inside the button bar. > This is due DialogPane adding two list change listeners to 'buttons' > (#getButtonTypes). They have the wrong order, which will result in the button > bar not changing at all and the 'buttonNodes' list will recreate the dialog > button(s). > Finally, this will make DialogPane#lookupButton returning the 'wrong' button, > which is in fact not used inside the dialog button bar. Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8263807: Code review changes - Changes: - all: https://git.openjdk.java.net/jfx/pull/432/files - new: https://git.openjdk.java.net/jfx/pull/432/files/f3dd7a2a..dc65db19 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=432=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=432=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/432.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/432/head:pull/432 PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Tue, 6 Apr 2021 14:29:28 GMT, Nir Lisker wrote:
> The benchmark might not tell the real story. To test these sorts of
> performance changes you have to use JMH. There's too much relating to JIT
> optimizations and JVM startup to be able to rely on the current benchmark.
> The theoretical point about invalidation listeners not boxing in comparison
> to change listeners is sound, so it won't be worse, but if we are looking for
> performance metrics, they need to be proper ones.
While true, my main motivation for this issue was that the original code is
wrongly implemented because it claims to do one thing, but doesn't do that
thing. So it's not primarily a question of optimization, but of correctness of
the implementation.
Anyway, here's a comparison benchmark done with JMH:
@State(Scope.Benchmark)
public class BindingBenchmark {
DoubleProperty property1 = new SimpleDoubleProperty();
DoubleProperty property2 = new SimpleDoubleProperty();
public BindingBenchmark() {
property2.bindBidirectional(property1);
}
@Benchmark
public void benchmark() {
for (int i = 0; i < 1000; ++i) {
property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
}
}
}
| Benchmark | Mode | Cnt | Score | Error | Units |
|---|--|-|---|---||
| before | thrpt | 5 | 3.455| 0.029 | ops/s |
| after | thrpt | 5 | 10.322 | 0.790 | ops/s |
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v5]
On Tue, 6 Apr 2021 14:16:03 GMT, Jeanette Winzenburg
wrote:
>> I guess I didn't look closely enough. Mystery solved, though.
>
>> I haven't gone back and done a detailed review yet, but I like the overall
>> changes. The one thing I did notice is that the language used to describe
>> the return value of `unregisterInvalidationListeners` and
>> `unregisterListChangeListeners` is different:
>>
>
> just a side-note: technically, both are different because neither the new
> listChangeListener nor the old changeListener methods are updated yet (we are
> still at the first bullet of the plan, lazy me will update all to the same as
> the invalidationListener wording, once we agree on it :)
>
> So what we are arguing right now is the difference between new (=
> invalidationListener) and old (= listChangeListener == c from
> changeListener) method doc.
>
>> ```
>> unregisterInvalidationListeners:
>> * @return a composed consumer that performs all removed operations, or
>> * {@code null} if none have been registered or the observable is
>> {@code null}
>> ```
>>
>> ```
>> unregisterListChangeListeners:
>> * @return A single chained {@link Consumer} consisting of all {@link
>> Consumer consumers} registered through
>> * {@link #registerListChangeListener(ObservableList, Consumer)}.
>> If no consumers have been registered on this
>> * list, null will be returned.
>> ```
>>
>> I find it confusing to say that the returned list "performs all removed
>> operations". Some might interpret this to mean that is is somehow necessary
>> to call the returned chained consumer as part of the removal, which isn't
>> what you meant to say.
>>
>> How about something like this for both newly added unregister methods?
>>
>> ```
>> * @return a composed consumer consisting of all previously registered
>> consumers, or
>> * {@code null} if none have been registered or the observable is {@code
>> null}
>> ```
>
> agree: the _performs_ might be misleading. And I like your suggestion, though
> I modified it a bit to be consistent with the rest of the changed doc which
> focuses on "operations" (a term taken from Consumer api):
>
> * @return a composed consumer representing all previously registered
> operations, or
> * {@code null} if none have been registered or the observable is {@code
> null}
I like this version, using "registered operations".
-
PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Tue, 6 Apr 2021 12:08:39 GMT, Kevin Rushforth wrote: >>> Seems like I forgot to hit the send button on the webform. Here's the >>> tracking number: 9069787. >> >> I see it now. And thanks for providing the benchmark. That's what I was >> looking for. > > The bug is now visible here: https://bugs.openjdk.java.net/browse/JDK-8264770 The benchmark might not tell the real story. To test these sorts of performance changes you have to use JMH. There's too much relating to JIT optimizations and JVM startup to be able to rely on the current benchmark. The theoretical point about invalidation listeners not boxing in comparison to change listeners is sound, so it won't be worse, but if we are looking for performance metrics, they need to be proper ones. - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v5]
On Thu, 1 Apr 2021 15:14:15 GMT, Kevin Rushforth wrote:
>> ahh .. looks like you marked the conversation that contained the original
>> comment as resolved:
>> https://github.com/openjdk/jfx/pull/409#discussion_r604387696 ..
>
> I guess I didn't look closely enough. Mystery solved, though.
> I haven't gone back and done a detailed review yet, but I like the overall
> changes. The one thing I did notice is that the language used to describe the
> return value of `unregisterInvalidationListeners` and
> `unregisterListChangeListeners` is different:
>
just a side-note: technically, both are different because neither the new
listChangeListener nor the old changeListener methods are updated yet (we are
still at the first bullet of the plan, lazy me will update all to the same as
the invalidationListener wording, once we agree on it :)
So what we are arguing right now is the difference between new (=
invalidationListener) and old (= listChangeListener == c from changeListener)
method doc.
> ```
> unregisterInvalidationListeners:
> * @return a composed consumer that performs all removed operations, or
> * {@code null} if none have been registered or the observable is {@code
> null}
> ```
>
> ```
> unregisterListChangeListeners:
> * @return A single chained {@link Consumer} consisting of all {@link
> Consumer consumers} registered through
> * {@link #registerListChangeListener(ObservableList, Consumer)}. If
> no consumers have been registered on this
> * list, null will be returned.
> ```
>
> I find it confusing to say that the returned list "performs all removed
> operations". Some might interpret this to mean that is is somehow necessary
> to call the returned chained consumer as part of the removal, which isn't
> what you meant to say.
>
> How about something like this for both newly added unregister methods?
>
> ```
> * @return a composed consumer consisting of all previously registered
> consumers, or
> * {@code null} if none have been registered or the observable is {@code
> null}
> ```
agree: the _performs_ might be misleading. And I like your suggestion, though I
modified it a bit to be consistent with the rest of the changed doc which
focuses on "operations" (a term taken from Consumer api):
* @return a composed consumer representing all previously registered
operations, or
* {@code null} if none have been registered or the observable is {@code
null}
-
PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v6]
> Changes in Lambda..Handler: > - added api and implemenation to support invalidation and listChange > listeners in the same way as changeListeners > - added java doc > - added tests > > Changes in SkinBase > - added api (and implementation delegating to the handler) > - copied java doc from the change listener un/register methods > - added tests to verify that the new (and old) api is indeed delegating to > the handler > > Note that the null handling is slightly extended: all methods now can handle > both null consumers (as before) and null observables (new) - this allows > simplified code on rewiring "path" properties (see reference example in the > issue) Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: changed javadoc of unregister (invalidation only) - Changes: - all: https://git.openjdk.java.net/jfx/pull/409/files - new: https://git.openjdk.java.net/jfx/pull/409/files/cf3a0228..e7849b21 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=409=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=409=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/409.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/409/head:pull/409 PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v5]
On Thu, 1 Apr 2021 15:10:45 GMT, Jeanette Winzenburg wrote: >> Maybe GitHub decided to prune it. >> >> Btw, you tagged the wrong "Kevin" above. > > ahh .. looks like you marked the conversation that contained the original > comment as resolved: > https://github.com/openjdk/jfx/pull/409#discussion_r604387696 .. I guess I didn't look closely enough. Mystery solved, though. - PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v5]
On Thu, 1 Apr 2021 15:09:34 GMT, Kevin Rushforth wrote: >> @Kevin >> >> don't worry, I have seen your comment (and been thinking about it :) - but >> also can't find it right now (nor can I find Nir's comment which started the >> overhaul ..), no idea what happened. >> >> Seeing your suggestion, it's similar to what keeps running in my mind. > > Maybe GitHub decided to prune it. > > Btw, you tagged the wrong "Kevin" above. ahh .. looks like you marked the conversation that contained the original comment as resolved: https://github.com/openjdk/jfx/pull/409#discussion_r604387696 .. - PR: https://git.openjdk.java.net/jfx/pull/409
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Mon, 5 Apr 2021 21:54:40 GMT, Kevin Rushforth wrote:
>> The internal BidirectionalBinding class implements bidirectional bindings
>> for JavaFX properties. The design intent of this class is to provide
>> specializations for primitive value types to prevent boxing conversions (cf.
>> specializations of the Property class with a similar design intent).
>>
>> However, the primitive BidirectionalBinding implementations do not meet the
>> design goal of preventing boxing conversions, because they implement
>> ChangeListener.
>>
>> ChangeListener is a generic SAM interface, which makes it impossibe to
>> invoke an implementation of ChangeListener::changed with a primitive value
>> (i.e. any primitive value will be auto-boxed).
>>
>> The boxing conversion happens, as with all ChangeListeners, at the
>> invocation site (for example, in ExpressionHelper). Since the boxing
>> conversion has already happened by the time any of the BidirectionalBinding
>> implementations is invoked, there's no point in using primitive
>> specializations of BidirectionalBinding after the fact.
>>
>> This issue can be solved by having BidirectionalBinding implement
>> InvalidationListener instead, which by itself does not incur a boxing
>> conversion. Because bidirectional bindings are eagerly evaluated, the
>> observable behavior remains the same.
>>
>> I've filed a bug report with the same title.
>
> I don't see this or any similar bug filed in our incident tracker. Did the
> submission complete to the point where you have an internal tracking number?
>
> Is there a measurable benefit in doing this? For example, do you have a
> benchmark of some sort that shows garbage generation has been reduced or
> performance has been improved?
Seems like I forgot to hit the send button on the webform. Here's the tracking
number: 9069787.
I've used the following manual benchmark, which bidirectionally binds two
properties and then produces a billion change notifications.
DoubleProperty property1 = new SimpleDoubleProperty();
DoubleProperty property2 = new SimpleDoubleProperty();
property2.bindBidirectional(property1);
long start = System.nanoTime();
for (int i = 0; i < 10; ++i) {
property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
}
long end = System.nanoTime();
System.out.println("Time elapsed (ms): " + (end - start) / 100);
And these are the results I got (time elapsed, in milliseconds):
| | before | after |
| ||---|
| Test run 1: | 28608 | 9122 |
| Test run 2: | 27928 | 9065 |
| Test run 3: | 31140 | 9181 |
| Test run 4: | 28041 | 9127 |
| Test run 5: | 28730 | 9181 |
So in this synthetic benchmark, the new implementation has a 3x performance
improvement compared to the old implementation.
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Mon, 5 Apr 2021 23:38:29 GMT, Kevin Rushforth wrote:
>> Seems like I forgot to hit the send button on the webform. Here's the
>> tracking number: 9069787.
>>
>> I've used the following manual benchmark, which bidirectionally binds two
>> properties and then produces a billion change notifications.
>> DoubleProperty property1 = new SimpleDoubleProperty();
>> DoubleProperty property2 = new SimpleDoubleProperty();
>> property2.bindBidirectional(property1);
>>
>> long start = System.nanoTime();
>>
>> for (int i = 0; i < 10; ++i) {
>> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
>> }
>>
>> long end = System.nanoTime();
>>
>> System.out.println("Time elapsed (ms): " + (end - start) / 100);
>>
>> And these are the results I got (time elapsed, in milliseconds):
>>
>> | | before | after |
>> | ||---|
>> | Test run 1: | 28608 | 9122 |
>> | Test run 2: | 27928 | 9065 |
>> | Test run 3: | 31140 | 9181 |
>> | Test run 4: | 28041 | 9127 |
>> | Test run 5: | 28730 | 9181 |
>>
>> So in this synthetic benchmark, the new implementation has a 3x performance
>> improvement compared to the old implementation.
>
>> Seems like I forgot to hit the send button on the webform. Here's the
>> tracking number: 9069787.
>
> I see it now. And thanks for providing the benchmark. That's what I was
> looking for.
The bug is now visible here: https://bugs.openjdk.java.net/browse/JDK-8264770
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Mon, 5 Apr 2021 23:16:53 GMT, mstr2 wrote: > Seems like I forgot to hit the send button on the webform. Here's the > tracking number: 9069787. I see it now. And thanks for providing the benchmark. That's what I was looking for. - PR: https://git.openjdk.java.net/jfx/pull/454
RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
The internal BidirectionalBinding class implements bidirectional bindings for JavaFX properties. The design intent of this class is to provide specializations for primitive value types to prevent boxing conversions (cf. specializations of the Property class with a similar design intent). However, the primitive BidirectionalBinding implementations do not meet the design goal of preventing boxing conversions, because they implement ChangeListener. ChangeListener is a generic SAM interface, which makes it impossibe to invoke an implementation of ChangeListener::changed with a primitive value (i.e. any primitive value will be auto-boxed). The boxing conversion happens, as with all ChangeListeners, at the invocation site (for example, in ExpressionHelper). Since the boxing conversion has already happened by the time any of the BidirectionalBinding implementations is invoked, there's no point in using primitive specializations of BidirectionalBinding after the fact. This issue can be solved by having BidirectionalBinding implement InvalidationListener instead, which by itself does not incur a boxing conversion. Because bidirectional bindings are eagerly evaluated, the observable behavior remains the same. I've filed a bug report with the same title. - Commit messages: - BidirectionalBinding uses InvalidationListener to prevent boxing conversions Changes: https://git.openjdk.java.net/jfx/pull/454/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=454=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264770 Stats: 111 lines in 3 files changed: 42 ins; 5 del; 64 mod Patch: https://git.openjdk.java.net/jfx/pull/454.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/454/head:pull/454 PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, mstr2 wrote: > The internal BidirectionalBinding class implements bidirectional bindings for > JavaFX properties. The design intent of this class is to provide > specializations for primitive value types to prevent boxing conversions (cf. > specializations of the Property class with a similar design intent). > > However, the primitive BidirectionalBinding implementations do not meet the > design goal of preventing boxing conversions, because they implement > ChangeListener. > > ChangeListener is a generic SAM interface, which makes it impossibe to invoke > an implementation of ChangeListener::changed with a primitive value (i.e. any > primitive value will be auto-boxed). > > The boxing conversion happens, as with all ChangeListeners, at the invocation > site (for example, in ExpressionHelper). Since the boxing conversion has > already happened by the time any of the BidirectionalBinding implementations > is invoked, there's no point in using primitive specializations of > BidirectionalBinding after the fact. > > This issue can be solved by having BidirectionalBinding implement > InvalidationListener instead, which by itself does not incur a boxing > conversion. Because bidirectional bindings are eagerly evaluated, the > observable behavior remains the same. > > I've filed a bug report with the same title. I don't see this or any similar bug filed in our incident tracker. Did the submission complete to the point where you have an internal tracking number? Is there a measurable benefit in doing this? For example, do you have a benchmark of some sort that shows garbage generation has been reduced or performance has been improved? - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing
On Sat, 3 Apr 2021 15:20:33 GMT, Florian Kirmaier wrote:
> @kleopatra
> I've added another test for the case, when index changes in such a way, that
> the cell should no longer be in the editing state,
> and the test seems to work. Do I miss something?
hmm .. that's weird: your test (cell 1 -> listEdit 0 -> cell 0) indeed passes,
doing it the other way round (cell 0 -> listEdit 1 -> cell 1) fails
@Test
public void testChangeCellIndex0ToListEditingIndex1() {
assertChangeIndexToEditing(0, 1);
}
@Test
public void testChangeCellIndex1ToListEditingIndex0() {
assertChangeIndexToEditing(1, 0);
}
private void assertChangeIndexToEditing(int initialCellIndex, int
listEditingIndex) {
list.setEditable(true);
cell.updateListView(list);
cell.updateIndex(initialCellIndex);
list.edit(listEditingIndex);
assertEquals("sanity: list editingIndex ", listEditingIndex,
list.getEditingIndex());
assertFalse("sanity: cell must not be editing", cell.isEditing());
cell.updateIndex(listEditingIndex);
assertEquals("sanity: index updated ", listEditingIndex,
cell.getIndex());
assertEquals("list editingIndex unchanged by cell", listEditingIndex,
list.getEditingIndex());
assertTrue(cell.isEditing());
}
Something obvious that I missed or a bummer in the test setup or what else? Any
ideas?
-
PR: https://git.openjdk.java.net/jfx/pull/441
Re: RFR: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock [v9]
On Tue, 6 Apr 2021 12:23:28 GMT, Kevin Rushforth wrote: >> I've clicked now the finalize button. > > @FlorianKirmaier The CSR needs to be updated (fixVersion set to `openjfx17`) > and moved to Finalize again. The fixVersion is now openfjx17 and it's moved to "Finalized" - PR: https://git.openjdk.java.net/jfx/pull/421
Re: RFR: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock [v9]
On Wed, 31 Mar 2021 13:53:35 GMT, Florian Kirmaier wrote: >> I reviewed the CSR, and it is now ready for you to Finalize. > > I've clicked now the finalize button. @FlorianKirmaier The CSR needs to be updated (fixVersion set to `openjfx17`) and moved to Finalize again. - PR: https://git.openjdk.java.net/jfx/pull/421
Re: RFR: 8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false [v2]
> Fixing leak in ProgressIndicator when treeShowing is false Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: 8264677 changes based on code review - Changes: - all: https://git.openjdk.java.net/jfx/pull/455/files - new: https://git.openjdk.java.net/jfx/pull/455/files/8e539d34..1a49a354 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=455=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=455=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/455.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/455/head:pull/455 PR: https://git.openjdk.java.net/jfx/pull/455
Re: RFR: 8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false [v2]
On Mon, 5 Apr 2021 22:47:54 GMT, Kevin Rushforth wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8264677 >> changes based on code review > > I haven't run it yet, but noticed a couple things during a quick code review. I've done the 2 changes from the code review! - PR: https://git.openjdk.java.net/jfx/pull/455
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button
On Thu, 18 Mar 2021 14:38:18 GMT, Marius Hanl wrote: > When DialogPane#getButtonTypes().setAll() is called twice with the same > argument(s), DialogPane#lookupButton does not return the node which is shown > inside the button bar. > This is due DialogPane adding two list change listeners to 'buttons' > (#getButtonTypes). They have the wrong order, which will result in the button > bar not changing at all and the 'buttonNodes' list will recreate the dialog > button(s). > Finally, this will make DialogPane#lookupButton returning the 'wrong' button, > which is in fact not used inside the dialog button bar. The fix and test look fine. There is one needed change as noted below. modules/javafx.controls/src/test/java/test/javafx/scene/control/DialogPaneTest.java line 2: > 1: /* > 2: * Copyright (c) 2021, 2021, Oracle and/or its affiliates. All rights > reserved. You need to revert this change. There must be only a single year listed if the start year and last modified year are the same. - PR: https://git.openjdk.java.net/jfx/pull/432
Integrated: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
On Wed, 24 Mar 2021 13:38:27 GMT, Kevin Rushforth wrote: > If the macOS system settings for opening documents as tabs is set to > "Always", then all JavaFX windows, including Dialogs, are opened as tabs, > unless they are child windows (that is, a Window with an owner). > > This is a real problem for certain types of dialogs, such as > `APPLICATION_MODAL` dialogs, regardless of whether the app uses `show()` or > uses `showAndWait()` to spin up a nested event loop. Also, if the dialog is > of a different size that the main window, it will resize itself when > switching tabs (which is visually jarring), and will not be sized correctly. > Even for ordinary stages, this isn't the desired behavior. > > The fix is to disallow opening in tabs for all JavaFX windows. There are a > couple existing tests that fail when the setting for opening documents in > tabs is set to "Always", but I also added a new explicit test for this by > creating two Stages and verifying that both are active at the same time. This pull request has now been integrated. Changeset: 58988582 Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/58988582 Stats: 174 lines in 2 files changed: 174 ins; 0 del; 0 mod 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set Reviewed-by: aghaisas, pbansal - PR: https://git.openjdk.java.net/jfx/pull/440
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button
On Tue, 6 Apr 2021 06:26:46 GMT, Marius Hanl wrote: >> I have one question on the fix (see below). Also, have you run all of the >> unit tests (not just the new one)? > >> >> >> I have one question on the fix (see below). Also, have you run all of the >> unit tests (not just the new one)? > > I did run all the normal tests and ever dialog related system test. > Everything passed. > Isn't jcheck running all the tests anyway? Thanks for the explanation. I'll finish my review shortly. > Isn't jcheck running all the tests anyway? You mean GitHub Actions (jcheck doesn't build or run any tests). It would, but for two things: 1. You used the `master` branch of your repo, which is not recommended, and will cause problems for you if you ever do another pull request. See [this comment](https://github.com/openjdk/jfx/pull/432#issuecomment-801982830) added by the Skara bot. 2. You haven't enabled GitHub action on your personal fork of the `jfx` repository. See [wiki.openjdk.java.net/display/SKARA/Testing](https://wiki.openjdk.java.net/display/SKARA/Testing) for information on how to enable it. - PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button
On Mon, 5 Apr 2021 23:01:25 GMT, Kevin Rushforth wrote: > > > I have one question on the fix (see below). Also, have you run all of the > unit tests (not just the new one)? I did run all the normal tests and ever dialog related system test. Everything passed. Isn't jcheck running all the tests anyway? - PR: https://git.openjdk.java.net/jfx/pull/432
Re: RFR: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
On Wed, 24 Mar 2021 13:38:27 GMT, Kevin Rushforth wrote: > If the macOS system settings for opening documents as tabs is set to > "Always", then all JavaFX windows, including Dialogs, are opened as tabs, > unless they are child windows (that is, a Window with an owner). > > This is a real problem for certain types of dialogs, such as > `APPLICATION_MODAL` dialogs, regardless of whether the app uses `show()` or > uses `showAndWait()` to spin up a nested event loop. Also, if the dialog is > of a different size that the main window, it will resize itself when > switching tabs (which is visually jarring), and will not be sized correctly. > Even for ordinary stages, this isn't the desired behavior. > > The fix is to disallow opening in tabs for all JavaFX windows. There are a > couple existing tests that fail when the setting for opening documents in > tabs is set to "Always", but I also added a new explicit test for this by > creating two Stages and verifying that both are active at the same time. Marked as reviewed by pbansal (Committer). - PR: https://git.openjdk.java.net/jfx/pull/440
Re: RFR: 8263807: Button types of a DialogPane are set twice, returns a wrong button
On Mon, 5 Apr 2021 22:58:47 GMT, Kevin Rushforth wrote:
>> When DialogPane#getButtonTypes().setAll() is called twice with the same
>> argument(s), DialogPane#lookupButton does not return the node which is shown
>> inside the button bar.
>> This is due DialogPane adding two list change listeners to 'buttons'
>> (#getButtonTypes). They have the wrong order, which will result in the
>> button bar not changing at all and the 'buttonNodes' list will recreate the
>> dialog button(s).
>> Finally, this will make DialogPane#lookupButton returning the 'wrong'
>> button, which is in fact not used inside the dialog button bar.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/DialogPane.java
> line 1062:
>
>> 1060: boolean hasDefault = false;
>> 1061: for (ButtonType cmd : getButtonTypes()) {
>> 1062: Node button = buttonNodes.get(cmd);
>
> Why was this change needed?
This change is not needed for the fix.
I removed it, because #computeIfAbsent() isn't needed anymore.
The (now) first listener will run and create/remove buttons based off the
changes to #getButtonTypes().
The second listener (which is calling this method) will use all created buttons
(to put them inside the ButtonBar).
-> So only a simple #get() is needed.
#computeIfAbsent() basically masked this bug before, as it was also creating a
button, if missing. But as mentionend, when we are here, all buttons should be
already created by the first listener.
-
PR: https://git.openjdk.java.net/jfx/pull/432
