Re: [jfx19] RFR: 8290348: TreeTableView jumping to top [v2]

[Johan Vos]

On Mon, 18 Jul 2022 15:43:06 GMT, Kevin Rushforth  wrote:

>> Johan Vos has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix issue with IDE-indicated unused statement
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
>  line 3113:
> 
>> 3111: adjustPosition();
>> 3112: }
>> 3113: recalculating = false;
> 
> Is it likely that this method might get an exception? I was wondering whether 
> the assignment to false should be in a try / finally. If there is no probable 
> way an exception could occur, or if an exception is non-recoverable (which is 
> quite possible), then no need for a try / finally.

Good question. There should be no non-recoverable exceptions in the VirtualFlow 
specific code. However, since IndexedCell.updateItem() and others might get 
called from this method, exceptions there might propagate. 
My initial thought was to fail-fast, but it is probably better to use a 
try/finally so that at least this method is still "working" somehow. I'll add 
that.

-

PR: https://git.openjdk.org/jfx/pull/836

Re: Proposal: Bump minimum JDK version for JavaFX 20 to JDK 17

[Thomas Reinhardt]

Hi,

I would assume that all projects with many dependencies are upgrading 
their JDK slowly. Because you obviously need all your dependencies to 
support the new JDK. Which means that the JDK is necessarily the last 
thing to update.


With our project we have hundreds of dependencies, some quite critical 
like wildfly or hibernate which are not always just drop-in upgrades. 
And of course everything needs to be validated. And on top of that, all 
build tools need to work with the new JDK as well. In our company there 
are dozens of in-house tools that generate code or inspect generated 
classes.


So I really would like to see a conservative approach for minimum JDK 
requirements. Upgrading the JDK is a huge expense both in time and 
money, so we are doing it for LTS versions only. We are still on JDK-11, 
the switch to JDK-17 should be done in the next 2-3 months for our main 
product.


We might be an outlier and I get there are many smaller projects that 
can easily upgrade their dependencies. But in my opinion libraries 
really should only ever have LTS versions as requirements.



-Thomas



On 20/07/2022 20:13, Matthias Bläsing wrote:

Hi,

Am Mittwoch, dem 20.07.2022 um 15:40 +0300 schrieb Nir Lisker:

The idea that an LTS JDK version is a good jumping point relies on
the assumption that users upgrade their JDK slowly (once every 2-3
years), but their JavaFX fast (once every 6 months). That is, they
want LTS for their JDK but not for their JavaFX.


a use-case where the assumption holds, that the JDK is slow being
updated, but OpenJFX can be updated are projects relying on installed
JDKs.

Apache projects can't bundle the JDK (it contains libraries with
licenses less liberal than ALv2). The JDK is hard to install with JDK
methods, so it needs to be "there". OpenJFX can be installed at runtime
and thus can be more or less trivially installed with JDK tools.
NetBeans for example currently bundles (as in has provisions to install
JavaFX with downloads from maven central) it.

Before you say it, yes you can create installers, that install the JDK
at runtime, but they are a PITA and running from ZIP is just
convenient.

Just my thoughts

Greetings

Matthias

RFR: 8290765: Remove parent disabled/treeVisible listeners

[Michael Strauß]

`Node` adds InvalidationListeners to its parent's `disabled` and `treeVisible` 
properties and calls its own `updateDisabled()` and 
`updateTreeVisible(boolean)` methods when the property values change.

These listeners are not required, since `Node` can easily call the 
`updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, 
saving the memory cost of maintaining listeners and bindings.

-

Commit messages:
 - Merge
 - added tests
 - Remove Node.disabled and Node.treeVisible listeners

Changes: https://git.openjdk.org/jfx/pull/841/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=841&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8290765
  Stats: 69 lines in 3 files changed: 57 ins; 10 del; 2 mod
  Patch: https://git.openjdk.org/jfx/pull/841.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/841/head:pull/841

PR: https://git.openjdk.org/jfx/pull/841

Re: RFR: 8271395: Fixing crash at printing [v2]

[Kevin Rushforth]

On Wed, 8 Sep 2021 07:52:31 GMT, Florian Kirmaier  wrote:

>> This PR switches the Thread to the QuantumRenderer, in the case the Disposer 
>> is called from another Thread - the printing Thread.
>> I'm open for better solutions on how to fix this Issue.
>> Initially i thought there is also a Race Condition in the resource pool, but 
>> a new one is created for the Printing Thread.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8271395
>   QuantumRenderer is no longer public

Inline

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
 line 451:

> 449: try {
> 450: CountDownLatch latch = new CountDownLatch(1);
> 451: 
> com.sun.javafx.tk.quantum.QuantumRenderer.getInstance().execute(() -> {

Minor: this is in the same package, so you don't need the full package name. As 
long as you are making other changes, you might change this, too.

A bigger question is that I think this is the first time we've used execute 
directly. I'm not sure that's a problem, but want to take a second look at it. 
The other places where we run a job on the renderer all use 
`Toolkit.getToolkit(). addRenderJob()`, which ultimately calls `submit()`.

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
 line 453:

> 451: 
> com.sun.javafx.tk.quantum.QuantumRenderer.getInstance().execute(() -> {
> 452: runnable.run();
> 453: latch.countDown();

If you do stick with the current approach, you would need a try / finally here, 
to avoid blocking the caller indefinitely. But in that case the exception isn't 
thrown back to the caller. I think using the existing method that returns a 
Future might be better.

-

PR: https://git.openjdk.org/jfx/pull/618

Re: RFR: 8271395: Fixing crash at printing [v2]

[Kevin Rushforth]

On Wed, 20 Jul 2022 19:33:43 GMT, Phil Race  wrote:

>> Florian Kirmaier has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   JDK-8271395
>>   QuantumRenderer is no longer public
>
> modules/javafx.graphics/src/main/java/com/sun/prism/impl/Disposer.java line 
> 131:
> 
>> 129:  */
>> 130: public static void cleanUp() {
>> 131: if 
>> (!Thread.currentThread().getName().startsWith("QuantumRenderer")) {
> 
> This seemed like a dubious way to detect the render thread so I looked around 
> for how it is done elsewhere but instead I found
> com/sun/javafx/sg/prism/NGCanvas.java
> private static void runOnRenderThread(final Runnable r) {
> // We really need a standard mechanism to detect the render thread !
> if (Thread.currentThread().getName().startsWith("QuantumRenderer")) {
> 
> And .. it is also used by printing, and I added it in 2013 :-(
> 
> In my defence I copied the code from webview as mentioned in 
> https://bugs.openjdk.org/browse/JDK-8094524
> 
> oh well.

Thanks, Phil, for digging into this. I was going to question it as well.

I'll file a follow-on bug to add such a method (not super urgent, but it would 
be worth having a consistent way to do this). I see that I suggested filing 
such an issue in the JBS issue Phil pointed to, but didn't do it.

-

PR: https://git.openjdk.org/jfx/pull/618

Proposal: Add Skin.install() method

[Andy Goryachev]

Hi,

I'd like to propose an API change in Skin interface (details below).  Your 
feedback will be greatly appreciated!

Thank you,
-andy





Summary
---

Introduce a new Skin.install() method with an empty default implementation.  
Modify Control.setSkin(Skin) implementation to invoke install() on the new skin 
after the old skin has been removed with dispose().


Problem
---

Presently, switching skins is a two-step process: first, a new skin is 
constructed against the target Control instance, and is attached (i.s. 
listeners added, child nodes added) to that instance in the constructor.  Then, 
Control.setSkin() is invoked with a new skin - and inside, the old skin is 
detached via its dispose() method.

This creates two problems:

 1. if the new skin instance is discarded before setSkin(), it remains 
attached, leaving the control in a weird state with two skins attached, causing 
memory leaks and performance degradation.

 2. if, in addition to adding listeners and child nodes, the skin sets a 
property, such as an event listener, or a handler, it overwrites the current 
value irreversibly.  As a result, either the old skin would not be able to 
cleanly remove itself, or the new skin would not be able to set the new values, 
as it does not know whether it should overwrite or keep a handler installed 
earlier (possibly by design).  Unsurprisingly, this also might cause memory 
leaks.

We can see the damage caused by looking at 
JDK-8241364 ☂ Cleanup skin 
implementations to allow switching, which refers a number of bugs:

JDK-8245145 Spinner: throws IllegalArgumentException when replacing skin
JDK-8245303 InputMap: memory leak due to incomplete cleanup on remove mapping
JDK-8268877 TextInputControlSkin: incorrect inputMethod event handler after 
switching skin
JDK-8236840 Memory leak when switching ButtonSkin
JDK-8240506 TextFieldSkin/Behavior: misbehavior on switching skin
JDK-8242621 TabPane: Memory leak when switching skin
JDK-8244657 ChoiceBox/ToolBarSkin: misbehavior on switching skin
JDK-8245282 Button/Combo Behavior: memory leak on dispose
JDK-8246195 ListViewSkin/Behavior: misbehavior on switching skin
JDK-8246202 ChoiceBoxSkin: misbehavior on switching skin, part 2
JDK-8246745 ListCell/Skin: misbehavior on switching skin
JDK-8247576 Labeled/SkinBase: misbehavior on switching skin
JDK-8253634 TreeCell/Skin: misbehavior on switching skin
JDK-8256821 TreeViewSkin/Behavior: misbehavior on switching skin
JDK-8269081 Tree/ListViewSkin: must remove flow on dispose
JDK-8273071 SeparatorSkin: must remove child on dispose
JDK-8274061 Tree-/TableRowSkin: misbehavior on switching skin
JDK-8244419 TextAreaSkin: throws UnsupportedOperation on dispose
JDK-8244531 Tests: add support to identify recurring issues with controls et al


Solution


This problem does not exist in e.g. Swing because the steps of instantiation, 
uninstalling the old ComponentUI ("skin"), and installing a new skin are 
cleanly separated.  ComponentUI constructor does not alter the component 
itself, ComponentUI.uninstallUI(JComponent) cleanly removes the old skin, 
ComponentUI.installUI(JComponent) installs the new skin.  We should follow the 
same model in javafx.

Specifically, I'd like to propose the following changes:

 1. Add Skin.install() with a default no-op implementation.
 2. Clarify skin creation-attachment-detachment sequence in Skin and 
Skin.install() javadoc
 3. Modify Control.setSkin(Skin) method (== invalidate listener in skin 
property) to call oldSkin.dispose() followed by newSkin.install()
 4. Many existing skins that do not set properties in the corresponding control 
may remain unchanged.  The skins that do, such as TextInputControlSkin 
(JDK-8268877), must be refactored to utilize the new install() method.  I think 
the refactoring would simply move all the code that accesses its control 
instance away from the constructor to install().


Impact Analysis
-

The changes should be fairly trivial.  Only a subset of skins needs to be 
refactored, and the refactoring itself is trivial.

The new API is backwards compatible with the existing code, the 
customer-developed skins can remain unchanged (thanks to default 
implementation).  In case where customers could benefit from the new API, the 
change is trivial.

The change will require CSR as it modifies a public API.

Re: RFR: 8271395: Fixing crash at printing [v2]

[Phil Race]

On Wed, 8 Sep 2021 07:52:31 GMT, Florian Kirmaier  wrote:

>> This PR switches the Thread to the QuantumRenderer, in the case the Disposer 
>> is called from another Thread - the printing Thread.
>> I'm open for better solutions on how to fix this Issue.
>> Initially i thought there is also a Race Condition in the resource pool, but 
>> a new one is created for the Printing Thread.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8271395
>   QuantumRenderer is no longer public

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.org/jfx/pull/618

Re: RFR: 8271395: Fixing crash at printing [v2]

[Phil Race]

On Wed, 8 Sep 2021 07:52:31 GMT, Florian Kirmaier  wrote:

>> This PR switches the Thread to the QuantumRenderer, in the case the Disposer 
>> is called from another Thread - the printing Thread.
>> I'm open for better solutions on how to fix this Issue.
>> Initially i thought there is also a Race Condition in the resource pool, but 
>> a new one is created for the Printing Thread.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8271395
>   QuantumRenderer is no longer public

modules/javafx.graphics/src/main/java/com/sun/prism/impl/Disposer.java line 131:

> 129:  */
> 130: public static void cleanUp() {
> 131: if 
> (!Thread.currentThread().getName().startsWith("QuantumRenderer")) {

This seemed like a dubious way to detect the render thread so I looked around 
for how it is done elsewhere but instead I found
com/sun/javafx/sg/prism/NGCanvas.java
private static void runOnRenderThread(final Runnable r) {
// We really need a standard mechanism to detect the render thread !
if (Thread.currentThread().getName().startsWith("QuantumRenderer")) {

And .. it is also used by printing, and I added it in 2013 :-(

In my defence I copied the code from webview as mentioned in 
https://bugs.openjdk.org/browse/JDK-8094524

oh well.

-

PR: https://git.openjdk.org/jfx/pull/618

Re: Proposal: Bump minimum JDK version for JavaFX 20 to JDK 17

[Matthias Bläsing]

Hi,

Am Mittwoch, dem 20.07.2022 um 15:40 +0300 schrieb Nir Lisker:
> The idea that an LTS JDK version is a good jumping point relies on
> the assumption that users upgrade their JDK slowly (once every 2-3
> years), but their JavaFX fast (once every 6 months). That is, they
> want LTS for their JDK but not for their JavaFX.

a use-case where the assumption holds, that the JDK is slow being
updated, but OpenJFX can be updated are projects relying on installed
JDKs.

Apache projects can't bundle the JDK (it contains libraries with
licenses less liberal than ALv2). The JDK is hard to install with JDK
methods, so it needs to be "there". OpenJFX can be installed at runtime
and thus can be more or less trivially installed with JDK tools.
NetBeans for example currently bundles (as in has provisions to install
JavaFX with downloads from maven central) it.

Before you say it, yes you can create installers, that install the JDK
at runtime, but they are a PITA and running from ZIP is just
convenient.

Just my thoughts

Greetings

Matthias

Re: [jfx19] RFR: 8290348: TreeTableView jumping to top [v2]

[Kevin Rushforth]

On Sun, 17 Jul 2022 18:59:22 GMT, Johan Vos  wrote:

>> Do not recalculate total estimated size recursively.
>> 
>> In the (unlikely) event that the recalculation triggers a new recalculation 
>> (e.g. when the height of a Cell is changed), do not start this recalculation.
>> The cache and cache size may become inconsistent if a recursive calculation 
>> is started.
>> This fixes JDK-8290348
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix issue with IDE-indicated unused statement

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
 line 3113:

> 3111: adjustPosition();
> 3112: }
> 3113: recalculating = false;

Is it likely that this method might get an exception? I was wondering whether 
the assignment to false should be in a try / finally. If there is no probable 
way an exception could occur, or if an exception is non-recoverable (which is 
quite possible), then no need for a try / finally.

-

PR: https://git.openjdk.org/jfx/pull/836

Re: [jfx19] RFR: 8290348: TreeTableView jumping to top [v3]

[Kevin Rushforth]

On Tue, 19 Jul 2022 10:36:07 GMT, Johan Vos  wrote:

>> Do not recalculate total estimated size recursively.
>> 
>> In the (unlikely) event that the recalculation triggers a new recalculation 
>> (e.g. when the height of a Cell is changed), do not start this recalculation.
>> The cache and cache size may become inconsistent if a recursive calculation 
>> is started.
>> This fixes JDK-8290348
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix test (so that it fails before the patch to VirtualFlow)

Looks good. I confirm that both the visual test and updated unit test fail 
without the fix and pass with the fix. I left one question inline, but I doubt 
it is a problem.

-

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.org/jfx/pull/836

Re: Proposal: Bump minimum JDK version for JavaFX 20 to JDK 17

[Nir Lisker]

In my opinion, each JDK version should be evaluated according to its
benefits to JavaFX and not according to LTS, which is a foreign concept to
OpenJDK.

The JDK moved to a 6 months release plan so it can innovate faster without
waiting several years to release an important feature, and if we go with a
2 year minimum JDK-bump plan we will be working against this idea.
There are features that are mostly invisible to consumers of JavaFX,
usually those that come from Amber (algebraic data types and pattern
matching). We really don't need to bump the JDK version for every
incremental improvement of the 'switch' construct, but maybe when there is
enough accumulation. Also, these features decrease the amount of bugs, so
they pay dividends. For features that the consumers benefit from, like
large performance increases, it will be a disservice to hold users back 2
years when they can already use them in their own applications. If Valhalla
comes out with value classes that can improve performance dramatically
(should be benchmarked), or with generalized generics, or Panama comes out
with its foreign access API that JavaFX makes heavy use of, it would be
counterproductive to wait 2 years for these improvements.

The idea that an LTS JDK version is a good jumping point relies on the
assumption that users upgrade their JDK slowly (once every 2-3 years), but
their JavaFX fast (once every 6 months). That is, they want LTS for their
JDK but not for their JavaFX. If this assumption is correct for the
majority of cases, then there is merit to it. From what I see, companies
that rely on LTS for their JDK don't bump their other dependencies fast
either. The result of tying JavaFX to OpenJDK LTS will be that slow moving
consumers won't notice the change, and fast moving consumers will be
restricted. JavaFX LTS is already tied to OpenJDK LTS for a reason, and I'm
wondering what the reason is for tying non-LTS JavaFX to LTS OpenJDK. Maybe
Johan can give some statistics there.

On Wed, Jul 20, 2022 at 10:40 AM Johan Vos  wrote:

> Dealing with the second question (when do we bump the version post-JFX20),
>
>
> On Tue, Jul 19, 2022 at 10:39 PM Philip Race 
> wrote:
> ...
>
>> What to do in future (ie what is the policy) is a separate question
>>
>> OpenJDK LTS releases will be every two years in future, so there is a
>> reasonable argument that
>> is too frequent to bump the FX minimum without a really compelling reason.
>>
>> Also consider that FX 20 will GA 4 1/2 years after FX 11  and the next
>> JDK LTS will be 21 ..
>> just 6 months later .. we surely aren't going to bump the minimum again
>> immediately.
>>
>> So we should not have a policy of the "latest LTS" should always be the
>> minimum - just that it should
>> be "some" LTS
>>
>> And perhaps we skip every other LTS or at the very least we don't bump
>> until the LTS has been available for 1 year ..
>>
>
> A possible idea is to apply the following 2 rules:
>
> 1) Whenever we bump the JDK version in JavaFX, it should be bumped to a
> JDK LTS version that is at least 1,5 years (minus 1 week) old at JFX
> release date.  This is exactly what will happen if the JFX 20 has a minimum
> JDK version of 17. This gives OpenJFX developers enough time to play with
> the new JDK features before using them in OpenJFX code.
>
> 2) We evaluate the options provided by the next LTS much in advance, and
> case by case (every 2 years), we discuss whether it is worth bumping the
> release (following rule 1)) .
>
> - Johan
>

Re: [jfx19] RFR: 8290348: TreeTableView jumping to top [v3]

[Ajit Ghaisas]

On Tue, 19 Jul 2022 10:36:07 GMT, Johan Vos  wrote:

>> Do not recalculate total estimated size recursively.
>> 
>> In the (unlikely) event that the recalculation triggers a new recalculation 
>> (e.g. when the height of a Cell is changed), do not start this recalculation.
>> The cache and cache size may become inconsistent if a recursive calculation 
>> is started.
>> This fixes JDK-8290348
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix test (so that it fails before the patch to VirtualFlow)

Looks good to me!

-

Marked as reviewed by aghaisas (Reviewer).

PR: https://git.openjdk.org/jfx/pull/836

Re: Proposal: Bump minimum JDK version for JavaFX 20 to JDK 17

[Johan Vos]

Dealing with the second question (when do we bump the version post-JFX20),


On Tue, Jul 19, 2022 at 10:39 PM Philip Race  wrote:
...

> What to do in future (ie what is the policy) is a separate question
>
> OpenJDK LTS releases will be every two years in future, so there is a
> reasonable argument that
> is too frequent to bump the FX minimum without a really compelling reason.
>
> Also consider that FX 20 will GA 4 1/2 years after FX 11  and the next
> JDK LTS will be 21 ..
> just 6 months later .. we surely aren't going to bump the minimum again
> immediately.
>
> So we should not have a policy of the "latest LTS" should always be the
> minimum - just that it should
> be "some" LTS
>
> And perhaps we skip every other LTS or at the very least we don't bump
> until the LTS has been available for 1 year ..
>

A possible idea is to apply the following 2 rules:

1) Whenever we bump the JDK version in JavaFX, it should be bumped to a JDK
LTS version that is at least 1,5 years (minus 1 week) old at JFX release
date.  This is exactly what will happen if the JFX 20 has a minimum JDK
version of 17. This gives OpenJFX developers enough time to play with the
new JDK features before using them in OpenJFX code.

2) We evaluate the options provided by the next LTS much in advance, and
case by case (every 2 years), we discuss whether it is worth bumping the
release (following rule 1)) .

- Johan