Re: RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak
On Tue, 22 Aug 2023 09:54:11 GMT, Prasanta Sadhukhan wrote: > Issue is when setting the content of a SwingNode, the old content is not > garbage collected owing to the fact > JLightweightFrame is never being released by SwingNodeDisposer > > The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that > prevents its collection > > Modified `SwingNode.setContentImpl` function to use a WeakReference to > properly release the memory. As suggested, optimized without WeakReference - PR Comment: https://git.openjdk.org/jfx/pull/1219#issuecomment-1689213243
Re: RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak [v2]
> Issue is when setting the content of a SwingNode, the old content is not > garbage collected owing to the fact > JLightweightFrame is never being released by SwingNodeDisposer > > The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that > prevents its collection > > Modified `SwingNode.setContentImpl` function to use a WeakReference to > properly release the memory. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Optimize without WeakReference - Changes: - all: https://git.openjdk.org/jfx/pull/1219/files - new: https://git.openjdk.org/jfx/pull/1219/files/d3a753f2..66b4d791 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1219&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1219&range=00-01 Stats: 23 lines in 3 files changed: 10 ins; 3 del; 10 mod Patch: https://git.openjdk.org/jfx/pull/1219.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1219/head:pull/1219 PR: https://git.openjdk.org/jfx/pull/1219
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced
Thanks for the heads-up Kevin,
I gave it a spin and found that generally because I use Task to load my
fxml views I had problems.
Some of these I could resolve by wrapping the offending line with runlater
in the fxml initialise method. This reminded me though of the days when
Tooltips had to be wrapped as well and it felt wrong because generally a
view may be constructed and modified on any thread as long as it's not yet
attached to a Scene in a Window that is showing.
This is highlighted further because I also have some third party controls
and libraries that are being initialized as part of the view, which now
just crash my application. This means that I cannot instantiate these
controls or libraries on any thread I want but have to make sure its done
on the FX thread, even though they're not attached to a Scene yet.
As a possible solution I was wondering since the Animation API says that
calls to play() and stop() are asynchronous if it wouldn't then be valid
to instead of throwing an exception, if the call to it isn't being made on
the FX thread, that it rather be delegated to the FX thread with for
example something like:
public abstract class Animation {
public void play() {
if ( Platform.isFxApplicationThread() ) playFX();
else Platform.runLater( () -> playFX() );
}
private void playFX() {
// previous play() code
}
}
This would then prevent the NPE errors that sometimes occur but not put a
burden on the existing code in the wild and allow views to be loaded with
Task call() without worries.
Thanks, regards
Jurgen
On 8/18/2023 4:17 PM, Kevin Rushforth wrote:
As a heads-up for app developers who use JavaFX animation (including
Animation, along with any subclasses, and AnimationTimer), a change went
into the JavaFX 22+5 build to enforce that the play, pause, and stop
methods must be called on the JavaFX Application thread. Applications
should have been doing that all along (else they would have been subject
to unpredictable errors), but for those who aren't sure, you might want
to take 22+5 for a spin and see if you have any problems with your
application. Please report them on the list if you do.
See JDK-8159048 [1] and CSR JDK-8313378 [2] for more information on this
change.
Thanks.
-- Kevin
[1] https://bugs.openjdk.org/browse/JDK-8159048
[2] https://bugs.openjdk.org/browse/JDK-8313378
Re: RFR: 8283675: Line not removed from LineChart when series cleared
On Tue, 22 Aug 2023 14:52:50 GMT, Andy Goryachev wrote: >> Could be useful. Do you think it should be done as part of this PR or should >> we create separate bug and take it up in test sprint? > > sure, we can use the upcoming test sprint to add these tests. would you > please create a ticket? Created [JDK-8314779](https://bugs.openjdk.org/browse/JDK-8283675) - PR Review Comment: https://git.openjdk.org/jfx/pull/1214#discussion_r1301959727
Re: [jfx21] RFR: 8312058: Documentation improvements for subscription based listeners
On Tue, 22 Aug 2023 06:30:02 GMT, John Hendrikx wrote: > Backport of https://bugs.openjdk.org/browse/JDK-8312058 Marked as reviewed by arapte (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1218#pullrequestreview-1589905070
Re: RFR: 8087700: [KeyCombination, Mac] KeyCharacterCombinations behave erratically
On Mon, 14 Aug 2023 16:28:20 GMT, Martin Fox wrote: > A KeyCharacterCombination should match a key if the target character is > printed on that key. For example, the user should be able to invoke the > `Shortcut+'+' ` combination by holding down the Shortcut key and pressing a > key that has '+' printed on it. This should work even if '+' is a shifted > symbol but the user doesn't hold down the Shift key. > > The Mac implements KeyCharacterCombinations by monitoring keystrokes to > discover the relationship between keys and characters. Currently the system > only records the character the user typed and no other characters on the same > key. This means a shortcut targeting a shifted character may not work until > the user types that character using Shift so the system learns the > relationship. > > This PR keeps the same mechanism in place but always records the shifted and > unshifted character for each keystroke. > > For the Mac the KeyboardTest app was modified to remove tests for characters > accessed using Option. We don't look for these characters because under the > hood just about every key has some symbol assigned to the Option modifier > that the user probably isn't even aware of. For these character we fall back > to the existing logic; once the user types the character it will start > working as a shortcut. Updated the JBS and PR title and added a new test case. The underlying problem is that KeyCharacterCombinations don't work until the user types the character directly which leads to confusing behavior e.g. Cmd+'+' doesn't work on the main keyboard until the user types '+' but then stops working if they type '+' on the numeric keypad. - PR Comment: https://git.openjdk.org/jfx/pull/1209#issuecomment-1688551134
Re: RFR: 8313650: Add hasProperties method to MenuItem and Toggle [v3]
On Tue, 22 Aug 2023 06:03:01 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains six additional
>> commits since the last revision:
>>
>> - javadoc
>> - contains properties interface
>> - Merge remote-tracking branch 'origin/master' into 8313650.properties
>> - review comments
>> - test
>> - has properties
>
> modules/javafx.graphics/src/main/java/javafx/scene/ContainsProperties.java
> line 36:
>
>> 34: * @since 22
>> 35: */
>> 36: public interface ContainsProperties {
>
> This name fails the "is-a" test "is a contains properties", like for example
> "is an `EventTarget`" or "is a `Styleable`". Suggestions:
>
> `ApplicationPropertyProvider`
> `PropertyProvider`
> `ApplicationPropertyAccessor`
> `PropertyAccessor`
>
> I'd personally go for the most descriptive one (the first one). It's long,
> but it's unlikely to be ever encountered as a type for a variable.
>
> Suggestion for the docs:
>
> /*
> * Provides access to properties for use primarily by application
> developers.
> */
Shouldn't it be `ApplicationPropertiesProvider` then, since multiple properties
are involved?
> modules/javafx.graphics/src/main/java/javafx/scene/ContainsProperties.java
> line 48:
>
>> 46: * @return true if this object has properties.
>> 47: */
>> 48: public boolean hasProperties();
>
> Suggestion:
>
> boolean hasProperties();
personal preference: prefer not to think when I see it in the "Declaration"
window in Eclipse.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301825295
PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301838535
Re: RFR: 8313650: Add hasProperties method to MenuItem and Toggle [v4]
> 1. Creating a new `javafx.scene.ContainsProperties` interface that declares > two methods: > - public ObservableMap getProperties(); > - public boolean hasProperties(); > > 2. Node, MenuItem, and Toggle now extend ContainsProperties interface. > > 3. Making implemented methods in Node, MenuItem, and Toggle final. > > While this change is an improvement from a design point of view, it > introduces a larger compatibility risk (by adding 'final' keyword similar to > [JDK-8313651](https://bugs.openjdk.org/browse/JDK-8313651)) > > This change requires CSR. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1215/files - new: https://git.openjdk.org/jfx/pull/1215/files/cd643cfd..effbc857 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1215&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1215&range=02-03 Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1215.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1215/head:pull/1215 PR: https://git.openjdk.org/jfx/pull/1215
Re: RFR: 8313651: Add 'final' keyword to public property methods in controls [v4]
On Tue, 22 Aug 2023 09:13:08 GMT, Nir Lisker wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review comments
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
> line 172:
>
>> 170: @Test
>> 171: public void testMissingFinalMethods() {
>> 172: for (Class c: allControlClasses()) {
>
> I think we use a space before `:`. Don't know if it's being enforced. If you
> change it there are 2 more places.
Adding superfluous spaces increases our carbon footprint ;-)
Modified the formatter to increase the footprint.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301807735
Re: RFR: 8313651: Add 'final' keyword to public property methods in controls [v5]
> In the Control hierarchy, all property accessor methods must be declared > `final`. > > Added a test to check for missing `final` keyword and added the said keyword > where required. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1213/files - new: https://git.openjdk.org/jfx/pull/1213/files/505c8482..38a4ba54 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1213&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1213&range=03-04 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1213.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1213/head:pull/1213 PR: https://git.openjdk.org/jfx/pull/1213
Re: RFR: 8313651: Add 'final' keyword to public property methods in controls [v4]
On Tue, 22 Aug 2023 05:51:32 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review comments
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
> line 98:
>
>> 96: public class ControlPropertiesTest {
>> 97:
>> 98: private static final boolean FAIL_FAST = !true;
>
> Did you intend to commit this? It was `true` before which seems good for
> tests, otherwise I'd really suggest using `false` here.
sorry, committed by accident. thanks for noticing!
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301801316
Re: RFR: 8283675: Line not removed from LineChart when series cleared
On Tue, 22 Aug 2023 09:37:46 GMT, Karthik P K wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/chart/AreaChartTest.java
>> line 502:
>>
>>> 500:
>>> 501: //JDK-8283675
>>> 502: @Test public void testChartFillRemovedOnClearingSeries() {
>>
>> should we add a similar test for other chart types (even if those do not
>> have the issue)?
>
> Could be useful. Do you think it should be done as part of this PR or should
> we create separate bug and take it up in test sprint?
sure, we can use the upcoming test sprint to add these tests. would you please
create a ticket?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1214#discussion_r1301774292
Re: RFR: 8283675: Line not removed from LineChart when series cleared
On Fri, 18 Aug 2023 07:25:28 GMT, Karthik P K wrote: > The issue is present in AreaChart along with the LineChart. Issue is fixed in > both the charts as part of this PR. > The line elements in case of Line chart and both line element and fill > element in the case of Area charts were not cleared in `makePaths()` method > in AreaChart.java. Hence the line and fill area were not getting cleared when > series was cleared. > > Made changes in code to clear line element and fill element as required in > the `makePaths()` method. > > Added tests to validate the changes in both LineChart and AreaChart. This looks good, with 2 more tickets out of scope for this PR. Could we have another reviewer to take a look please? - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1214#pullrequestreview-1589678151
Re: RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak
On Tue, 22 Aug 2023 09:54:11 GMT, Prasanta Sadhukhan wrote: > Issue is when setting the content of a SwingNode, the old content is not > garbage collected owing to the fact > JLightweightFrame is never being released by SwingNodeDisposer > > The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that > prevents its collection > > Modified `SwingNode.setContentImpl` function to use a WeakReference to > properly release the memory. I should note, there is a memory leak still when using `Disposer`. Each time content is switched, but the `SwingNode` is re-used, a `DisposerRecord` is created that only gets cleaned up when `SwingNode` goes out of scope. In other words, a million content changes will leave behind a million `DisposerRecord`s. So, either there needs to be a `removeRecord` added to `Disposer`, or you could switch to the `TreeShowingProperty` solution. - PR Comment: https://git.openjdk.org/jfx/pull/1219#issuecomment-1687998468
Re: RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak
On Tue, 22 Aug 2023 09:54:11 GMT, Prasanta Sadhukhan
wrote:
> Issue is when setting the content of a SwingNode, the old content is not
> garbage collected owing to the fact
> JLightweightFrame is never being released by SwingNodeDisposer
>
> The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that
> prevents its collection
>
> Modified `SwingNode.setContentImpl` function to use a WeakReference to
> properly release the memory.
I think the solution is not optimal.
There are two references to the light weight frame:
1. SwingNode refers it as a field, which is changed on each content change so
no need to worry about that one
2. A `DisposerRecord` refers it to call `dispose` on the light weight frame.
`dispose` must be called in two situations:
1. When a frame is no longer needed because we're switching content
2. When `SwingNode` goes out of scope (as JavaFX has no "dispose" semantics for
`Node`s this requires a creative solution)
Case 1 is easy to solve, just dispose the light weight frame when content
changes. This is already done. What was missing here is that the disposer
record is not cleaned up as well (which also refers the frame), and which would
only get cleaned up when `SwingNode` goes out of scope (which it doesn't yet).
Case 2 has two solutions. One that uses weak references that aren't really
intended for resource management, and one that thinks about the lifecycle of
when the light weight frame is no longer needed.
1. Use a weak reference to `SwingNode` and when it goes out of scope call light
weight frame `dispose`
2. Check the showing status of `SwingNode` (other controls do this) and when it
is not showing, clean up resources (this happens immediately, and doesn't
require relying on the GC).
Now, the easiest solution to fix this without adding even more weak references
is to track the `DisposerRecord` and instead of calling `lwFrame.dispose` you
call `record.dispose`, like this:
private DisposerRecord rec;
/*
* Called on EDT
*/
private void setContentImpl(JComponent content) {
if (lwFrame != null) {
rec.dispose(); // disposes of the record's lwFrame pointer as well
as calling `dispose` on lwFrame
rec = null;
lwFrame = null;
}
if (content != null) {
lwFrame = swNodeIOP.createLightweightFrame();
SwingNodeWindowFocusListener snfListener =
new SwingNodeWindowFocusListener(this);
swNodeIOP.addWindowFocusListener(lwFrame, snfListener);
if (getScene() != null) {
Window window = getScene().getWindow();
if (window != null) {
swNodeIOP.notifyDisplayChanged(lwFrame,
window.getRenderScaleX(),
window.getRenderScaleY());
}
}
swNodeIOP.setContent(lwFrame,
swNodeIOP.createSwingNodeContent(content, this));
swNodeIOP.setVisible(lwFrame, true);
// track the record
rec = swNodeIOP.createSwingNodeDisposer(lwFrame);
Disposer.addRecord(this, rec);
if (getScene() != null) {
notifyNativeHandle(getScene().getWindow());
}
SwingNodeHelper.runOnFxThread(() -> {
locateLwFrame();// initialize location
if (focusedProperty().get()) {
activateLwFrame(true);
}
});
}
}
Now, the even better solution would be to get rid of `Disposer` completely.
This can be done by using a `TreeShowingProperty` which you create in the
constructor of `SwingNode`:
this.treeShowingProperty = new TreeShowingProperty(this);
By adding a `ChangeListener` to this property you get updates on whether the
`SwingNode` is currently showing or not. Note that when it goes out of scope,
it will **always** first be unshown (otherwise it can't go out of scope as
would still be referenced).
In this listener you can then call `dispose` when `showing` is `false`, or
create a new light weight frame when `showing` is `true`.
Examples of using the `TreeShowingProperty` that deal with similar issues can
be found in `PopupWindow` and `ProgressIndicatorSkin`. In both cases they need
to release resources so GC collection can succeed. `ProgressIndicatorSkin` has
to stop its animation (as the animation would otherwise keep a reference to the
skin), and `PopupWindow` needs to call `hide` to release native resources.
-
PR Comment: https://git.openjdk.org/jfx/pull/1219#issuecomment-1687987162
RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak
Issue is when setting the content of a SwingNode, the old content is not garbage collected owing to the fact JLightweightFrame is never being released by SwingNodeDisposer The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that prevents its collection Modified `SwingNode.setContentImpl` function to use a WeakReference to properly release the memory. - Commit messages: - 8262518: SwingNode.setContent does not close previous content, resulting in memory leak - 8262518: SwingNode.setContent does not close previous content, resulting in memory leak Changes: https://git.openjdk.org/jfx/pull/1219/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1219&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8262518 Stats: 14 lines in 2 files changed: 5 ins; 1 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1219.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1219/head:pull/1219 PR: https://git.openjdk.org/jfx/pull/1219
Re: RFR: 8313651: Add 'final' keyword to public property methods in controls [v4]
On Mon, 21 Aug 2023 23:11:49 GMT, Andy Goryachev wrote:
>> In the Control hierarchy, all property accessor methods must be declared
>> `final`.
>>
>> Added a test to check for missing `final` keyword and added the said keyword
>> where required.
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
Looks good. Gave a couple of minor optional suggestions.
As to the topic of class finding, I think it's fine to use a manual list, at
least for now. The complexity of automatically detecting the classes might not
be worth the hassle. Leaving it up to you.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 91:
> 89:
> 90: /**
> 91: * Tests contract for properties and their accessors in the Control type
> hierarchy.
Mutators too?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 168:
> 166:
> 167: /**
> 168: * Tests for missing final keyword in Control properties and their
> accessors.
Technically, we're looking at mutators too, not just accessors.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 172:
> 170: @Test
> 171: public void testMissingFinalMethods() {
> 172: for (Class c: allControlClasses()) {
I think we use a space before `:`. Don't know if it's being enforced. If you
change it there are 2 more places.
-
Marked as reviewed by nlisker (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1213#pullrequestreview-1588935327
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301317702
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301317162
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301316459
Re: RFR: 8283675: Line not removed from LineChart when series cleared
On Mon, 21 Aug 2023 18:41:01 GMT, Andy Goryachev wrote:
> @karthikpandelu would you please check if we already have a similar issue and
> create one if not?
I couldn't find a similar issue, created
[JDK-8314754](https://bugs.openjdk.org/browse/JDK-8314754)
> modules/javafx.controls/src/test/java/test/javafx/scene/chart/AreaChartTest.java
> line 502:
>
>> 500:
>> 501: //JDK-8283675
>> 502: @Test public void testChartFillRemovedOnClearingSeries() {
>
> should we add a similar test for other chart types (even if those do not have
> the issue)?
Could be useful. Do you think it should be done as part of this PR or should we
create separate bug and take it up in test sprint?
-
PR Comment: https://git.openjdk.org/jfx/pull/1214#issuecomment-1687830927
PR Review Comment: https://git.openjdk.org/jfx/pull/1214#discussion_r1301360221
