Re: New API: Animation/Timeline improvement
I don't think that's the correct solution, as it opens up a whole new
avenue for subtle bugs, bringing GC/JVM/OS whims and settings into the
mix. We want the clean-up to be easier to reason about, not harder.
Even if it were a good idea, it's likely also going to be a breaking
change to add weak references at this stage, without some kind of opt-in
(which would then require new API, in which case we might as well go for
a solution that has no need of weak references).
I feel I have to keep repeating this, but there almost zero guarantees
made by the JVM or Java with regards to references; they are fine for
internal processes carefully designed to have no user visible side
effects, but burdening the user with side effects (delayed clean-up, or
early unexpected clean-up due to lack of a hard reference) without the
user actually choosing to use a reference type themselves is not a good
design.
--John
On 18/12/2023 17:18, Andy Goryachev wrote:
Would making Timeline to use WeakReferences solve the issue without
the need for a new API?
-andy
*From: *openjfx-dev on behalf of John
Hendrikx
*Date: *Friday, December 15, 2023 at 21:53
*To: *openjfx-dev
*Subject: *New API: Animation/Timeline improvement
Hi list,
I've noticed that Animations and Timelines are often a source of leaks,
and their clean-up is often either non-existent or incorrect. The
reason they cause leaks easily is because a running animation or
timeline is globally referred from a singleton PrimaryTimer. The
animation or timeline then refers to properties or event handlers which
refer to controls (which refer to parents and the entire scene).
For example:
- ScrollBarBehavior uses a Timeline, but neglects to clean it up. If it
was running at the time a Scene is detached from a Window, and that
Scene is left to go out of scope, it won't because Timeline refers it;
this can happen if the behavior never receives a key released event.
- ScrollBarBehavior has no dispose method overridden, so swapping Skins
while the animation is running will leave a Timeline active (it uses
Animation.INDEFINITE)
- SpinnerBehavior has flawed clean up; it attaches a Scene listener and
disables its timeline when the scene changed, but the scene doesn't have
to change for it to go out of scope as a whole... Result is that if you
have a spinner timeline running, and you close the entire window (no
Scene change happens), the entire Scene will still be referred. It also
uses an indefinite cycle count. It also lacks a dispose method, so
swapping Skins at a bad moment can also leave a reference.
I think these mistakes are common, and far too easy to make. The most
common use cases for animations revolve around modifying properties on
visible controls, and although animations can be used for purposes other
than animating UI controls, this is extremely rare. So it is safe to
say that in 99% of cases you want the animation to stop once a some Node
is no longer showing. For both the mentioned buggy behaviors above, this
would be perfect. A spinner stops spinning when no longer showing, and
a scroll bar stops scrolling when no longer showing. It is also likely
to apply for many other uses of timelines and animations.
I therefore want to propose a new API, either on Node or Animation (or
both):
/**
* Creates a new timeline which is stopped automatically when
this Node
* is no longer showing. Stopping timelines is essential as they
may refer
* nodes even after they are no longer used anywhere, preventing
them from
* being garbage collected.
*/
Node.createTimeline(); // and variants with the various Timeline
constructors
And/or:
/**
* Links this Animation to the given Node, and stops the animation
* automatically when the Node is no longer showing. Stopping
animations
* is essential as they may refer nodes even after they are no
longer used
* anywhere, preventing them from being garbage collected.
*/
void stopWhenHidden(Node node);
The above API for Animation could also be provided through another
constructor, which takes a Node which will it be linked to.
Alternatives:
- Be a lot more diligent about cleaning up animations and timelines
(essentially maintain the status quo which has led to above bugs)
- Use this lengthy code fragment below:
Timeline timeline = new Timeline();
someNode.sceneProperty()
.when(timeline.statusProperty().map(status -> status !=
Status.STOPPED))
.flatMap(Scene::windowProperty)
.flatMap(Window::showingProperty)
.orElse(false)
.subscribe(showing -> {
if (!showing) timeline.stop();
});
The `when` line ensures that the opposite problem (Nodes forever
referencing Timelines) doesn't occur if you are creating a new Timeline
for each use (not recommended, but nonetheless a common occurrence).
--John
Re: RFR: 8301219: JavaFX crash when closing with the escape key [v2]
On Fri, 15 Dec 2023 20:57:01 GMT, Martin Fox wrote:
>> While processing a key down event the Glass GTK code sends out PRESSED and
>> TYPED KeyEvents back to back. If the stage is closed during the PRESSED
>> event the code will end up referencing freed memory while sending out the
>> TYPED event. This can lead to intermittent crashes.
>>
>> In GlassApplication.cpp the EventCounterHelper object ensures the
>> WindowContext isn't deleted while processing an event. Currently the helper
>> object is being created *after* IME handling instead of before. If the IME
>> is enabled it's possible for the WindowContext to be deleted in the middle
>> of executing a number of keyboard-related events.
>>
>> The fix is simple; instantiate the EventCounterHelper object earlier. There
>> isn't always a WindowContext so I tweaked the EventCounterHelper to do
>> nothing if the context is null.
>>
>> To make the crash more reproducible I altered the WindowContext such that
>> when it's deleted the freed memory is filled with 0xCC. This made the crash
>> more reproducible and allowed me to test the fix. I did the same with
>> GlassView since that's the only other Glass GTK class that's instantiated
>> with `new` and discarded with `delete`.
>
> Martin Fox has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Debugging code turned off by default. Empty line removed.
LGTM now. I did ask one question and will reapprove if you make the change.
modules/javafx.graphics/src/main/native-glass/gtk/DeletedMemDebug.h line 46:
> 44: static void operator delete[](void* ptr, std::size_t sz)
> 45: {
> 46: ::memset(ptr, 0xcc, sz);
Should this use `FILL` instead of hard-coded `0xcc`?
-
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1307#pullrequestreview-1786885134
PR Review Comment: https://git.openjdk.org/jfx/pull/1307#discussion_r1430137769
Re: RFR: 8301219: JavaFX crash when closing with the escape key [v3]
> While processing a key down event the Glass GTK code sends out PRESSED and > TYPED KeyEvents back to back. If the stage is closed during the PRESSED event > the code will end up referencing freed memory while sending out the TYPED > event. This can lead to intermittent crashes. > > In GlassApplication.cpp the EventCounterHelper object ensures the > WindowContext isn't deleted while processing an event. Currently the helper > object is being created *after* IME handling instead of before. If the IME is > enabled it's possible for the WindowContext to be deleted in the middle of > executing a number of keyboard-related events. > > The fix is simple; instantiate the EventCounterHelper object earlier. There > isn't always a WindowContext so I tweaked the EventCounterHelper to do > nothing if the context is null. > > To make the crash more reproducible I altered the WindowContext such that > when it's deleted the freed memory is filled with 0xCC. This made the crash > more reproducible and allowed me to test the fix. I did the same with > GlassView since that's the only other Glass GTK class that's instantiated > with `new` and discarded with `delete`. Martin Fox has updated the pull request incrementally with one additional commit since the last revision: Consistent use of FILL in mem debug code. - Changes: - all: https://git.openjdk.org/jfx/pull/1307/files - new: https://git.openjdk.org/jfx/pull/1307/files/6a4a4e63..09f8ede5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1307&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1307&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1307.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1307/head:pull/1307 PR: https://git.openjdk.org/jfx/pull/1307
Re: RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView [v2]
On Mon, 18 Dec 2023 14:13:51 GMT, Jeanette Winzenburg wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> address feedback > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java > line 87: > >> 85: tableView.setItems(items); >> 86: >> 87: stageLoader = new StageLoader(new Scene(tableView)); > > this changes the context of unrelated tests - no idea whether or not it > matters, but I would try not to :) Good catch, actually this changes is a leftover after some changes I did while getting the test to fail... but now I see it is not needed after all. - PR Review Comment: https://git.openjdk.org/jfx/pull/1308#discussion_r1430332070
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:36:32 GMT, Jose Pereda wrote: >> That's only if we want to keep building working on 16.04. I think it makes >> easier to test on it. >> But, it's already failing for Platform preferences API code. > > In any case, if `GdkSeat` is available only since 3.20, then we need to add > the compile-time checks anyway, since minimum supported is 3.8. It seems like this is the best we can do for now. What is means is that we won't be able to build on a system with 3.8 - 3.19 and distribute a library that will use the new functionality when running on 3.20 or later. That's probably OK in this case, but is worth noting. What is important, and why the runtime check is needed (as noted above) is that we can build a production release of JavaFX on a system that has a later GTK and it will run on a system with an older GTK. It won't use the feature on those older systems, but won't crash. - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1430679284
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
> This PR replaces the deprecated `gdk_pointer_grab` with `gdk_seat_grab`, and > `gdk_pointer_ungrab ` with `gdk_seat_ungrab`, using runtime checks and > wrapped functions for GTK 3.20+ (so systems without it still run with GTK > 3.8+), and fixes the dragging issue on Wayland. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Add compile-time checks to GdkSeat - Changes: - all: https://git.openjdk.org/jfx/pull/1305/files - new: https://git.openjdk.org/jfx/pull/1305/files/99230ca6..f8ffe87f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=01-02 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1305.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1305/head:pull/1305 PR: https://git.openjdk.org/jfx/pull/1305
Re: Compiling Plataform Preferences on Ubuntu 16.04
This is really about gcc rather than a version of Ubuntu. I see little value in making the javafx.graphics native code compile with such an old version of gcc that it doesn't support C++11. (Worth noting that WebKit requires an even newer version, since it uses C++20 features). So, while yes, I'd like to see this continue to be buildable on Ubuntu 16.04 -- and so far I have no problems compiling on that OS -- it seems fine to require a newer compiler in order for you to do so. -- Kevin On 12/18/2023 2:12 AM, Thiago Milczarek Sayão wrote: Hi, I get some compilation errors when building JavaFX on Ubuntu 16.04, which seems related to the newly introduced Platform preferences API. Should we bother to make it work? In file included from /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp:45:0: /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.h:30:7: error: override controls (override/final) only available with -std=c++11 or -std=gnu++11 [-Werror] class PlatformSupport final ^ /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.h:35:47: error: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11 [-Werror] PlatformSupport(PlatformSupport const&) = delete; ^ /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.h:36:58: error: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11 [-Werror] PlatformSupport& operator=(PlatformSupport const&) = delete; -- Thiago.
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 21:56:09 GMT, Thiago Milczarek Sayao wrote: >> As an FYI, we use GTK 3.22 on our CI build machines so I wouldn't want to >> see the build-time dependency move any higher than it currently is. > > The suggestion was to allow building on 16.04 - so it's easier to test. But > it currently does not build because of code introduced on Platform > Preferences API. On the other hand, maybe it's best to fail so we make sure > building requires 3.20. The native Platform Preferences code is failing for an entirely different reason (a too-old compiler), but you raise an interesting point. If we really can't support building a distributable binary on a system with a too-old GTK library in a way that enables using a new feature like this, we might prefer to "fail fast" at build time, rather than produce a binary that doesn't support the new capability even when running on a newer system. If we decide that is a reasonable thing to do, we might want the minimum _build-time_ version of GTK to be higher than the minimum version we support at _runtime_. I think we shouldn't necessarily tie that decision to this PR. So I think the current approach in this PR: allow it to build on an older GTK without using the new functionality, is probably OK (although not ideal as evidenced by this discussion). - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1430703990
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v4]
> A listener was added but never removed. > This patch removes the listener when the menu it links to is cleared. Fix for > https://bugs.openjdk.org/browse/JDK-8319779 Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Fix more memoryleaks due to listeners never being unregistered. - Changes: - all: https://git.openjdk.org/jfx/pull/1283/files - new: https://git.openjdk.org/jfx/pull/1283/files/ba840397..47f4d65d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=02-03 Stats: 12 lines in 1 file changed: 1 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1283.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1283/head:pull/1283 PR: https://git.openjdk.org/jfx/pull/1283
Re: RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView
On Mon, 18 Dec 2023 12:09:21 GMT, Jose Pereda wrote: > This PR fixes an issue when a new `TableColumn` is added to a `TableView` > control with fixed cell size set, where the `TableRowSkinBase` failed to add > the cells for the new column. > > A test is included that fails before applying this PR and passes with it. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java line 87: > 85: tableView.setItems(items); > 86: > 87: stageLoader = new StageLoader(new Scene(tableView)); this changes the context of unrelated tests - no idea whether or not it matters, but I would try not to :) - PR Review Comment: https://git.openjdk.org/jfx/pull/1308#discussion_r1430196552
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 21:47:05 GMT, Kevin Rushforth wrote: >> In any case, if `GdkSeat` is available only since 3.20, then we need to add >> the compile-time checks anyway, since minimum supported is 3.8. > > It seems like this is the best we can do for now. What is means is that we > won't be able to build on a system with 3.8 - 3.19 and distribute a library > that will use the new functionality when running on 3.20 or later. That's > probably OK in this case, but is worth noting. > > What is important, and why the runtime check is needed (as noted above) is > that we can build a production release of JavaFX on a system that has a later > GTK and it will run on a system with an older GTK. It won't use the feature > on those older systems, but won't crash. As an FYI, we use GTK 3.22 on our CI build machines so I wouldn't want to see the build-time dependency move any higher than it currently is. - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1430682992
RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView
This PR fixes an issue when a new `TableColumn` is added to a `TableView` control with fixed cell size set, where the `TableRowSkinBase` failed to add the cells for the new column. A test is included that fails before applying this PR and passes with it. - Commit messages: - Add cells to tableRow after new columns are added to tableView Changes: https://git.openjdk.org/jfx/pull/1308/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1308&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321970 Stats: 26 lines in 2 files changed: 24 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1308.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1308/head:pull/1308 PR: https://git.openjdk.org/jfx/pull/1308
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v3]
> A listener was added but never removed. > This patch removes the listener when the menu it links to is cleared. Fix for > https://bugs.openjdk.org/browse/JDK-8319779 Johan Vos 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 three additional commits since the last revision: - These changes are related to JBS-8318841 so we want to have that code in as well. Merge branch 'master' into 8319779-systemmenu - process reviewers comments - A listener was added but never removed. This patch removes the listener when the menu it links to is cleared. Fix for https://bugs.openjdk.org/browse/JDK-8319779 - Changes: - all: https://git.openjdk.org/jfx/pull/1283/files - new: https://git.openjdk.org/jfx/pull/1283/files/c7fe11cb..ba840397 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=01-02 Stats: 53018 lines in 578 files changed: 24682 ins; 20433 del; 7903 mod Patch: https://git.openjdk.org/jfx/pull/1283.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1283/head:pull/1283 PR: https://git.openjdk.org/jfx/pull/1283
Re: RFR: 8301219: JavaFX crash when closing with the escape key [v3]
On Mon, 18 Dec 2023 19:15:03 GMT, Martin Fox wrote: >> While processing a key down event the Glass GTK code sends out PRESSED and >> TYPED KeyEvents back to back. If the stage is closed during the PRESSED >> event the code will end up referencing freed memory while sending out the >> TYPED event. This can lead to intermittent crashes. >> >> In GlassApplication.cpp the EventCounterHelper object ensures the >> WindowContext isn't deleted while processing an event. Currently the helper >> object is being created *after* IME handling instead of before. If the IME >> is enabled it's possible for the WindowContext to be deleted in the middle >> of executing a number of keyboard-related events. >> >> The fix is simple; instantiate the EventCounterHelper object earlier. There >> isn't always a WindowContext so I tweaked the EventCounterHelper to do >> nothing if the context is null. >> >> To make the crash more reproducible I altered the WindowContext such that >> when it's deleted the freed memory is filled with 0xCC. This made the crash >> more reproducible and allowed me to test the fix. I did the same with >> GlassView since that's the only other Glass GTK class that's instantiated >> with `new` and discarded with `delete`. > > Martin Fox has updated the pull request incrementally with one additional > commit since the last revision: > > Consistent use of FILL in mem debug code. Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1307#pullrequestreview-1787650273
[jfx17u] Integrated: 8181084: JavaFX show big icons in system menu on macOS with Retina display
clean backport of 8181084: JavaFX show big icons in system menu on macOS with Retina di…splay Reviewed-by: jpereda, kcr - Commit messages: - 8181084: JavaFX show big icons in system menu on macOS with Retina display Changes: https://git.openjdk.org/jfx17u/pull/174/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=174&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8181084 Stats: 103 lines in 14 files changed: 97 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jfx17u/pull/174.diff Fetch: git fetch https://git.openjdk.org/jfx17u.git pull/174/head:pull/174 PR: https://git.openjdk.org/jfx17u/pull/174
Re: RFR: 8301219: JavaFX crash when closing with the escape key [v2]
On Mon, 18 Dec 2023 13:22:07 GMT, Kevin Rushforth wrote:
>> Martin Fox has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Debugging code turned off by default. Empty line removed.
>
> modules/javafx.graphics/src/main/native-glass/gtk/DeletedMemDebug.h line 46:
>
>> 44: static void operator delete[](void* ptr, std::size_t sz)
>> 45: {
>> 46: ::memset(ptr, 0xcc, sz);
>
> Should this use `FILL` instead of hard-coded `0xcc`?
Yes, it should use FILL. Will fix.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1307#discussion_r1430550148
Re: RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView [v2]
> This PR fixes an issue when a new `TableColumn` is added to a `TableView` > control with fixed cell size set, where the `TableRowSkinBase` failed to add > the cells for the new column. > > A test is included that fails before applying this PR and passes with it. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: address feedback - Changes: - all: https://git.openjdk.org/jfx/pull/1308/files - new: https://git.openjdk.org/jfx/pull/1308/files/9c138c49..04cc76b4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1308&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1308&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1308.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1308/head:pull/1308 PR: https://git.openjdk.org/jfx/pull/1308
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v4]
On Mon, 18 Dec 2023 13:18:02 GMT, Johan Vos wrote: >> A listener was added but never removed. >> This patch removes the listener when the menu it links to is cleared. Fix >> for https://bugs.openjdk.org/browse/JDK-8319779 > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Fix more memoryleaks due to listeners never being unregistered. As for the testing issue: the memoryleak before this fix leads to an increasing number of `com.sun.glass.ui.MenuItem` instances. We could test the previous memoryleak, as that was creating an increasing number of `javafx.scene.control.MenuItem` instances, which could be handled inside a JavaFX application and stored as WeakReferences in a list, that could then be required to be empty after GC. Our systemtest does not have access to the `com.sun.glass.ui.MenuItem` instances, so we can't use the same approach. In theory, it should be possible to mock the whole path that leads to this leak, but it is complex. I do understand the value of tests for this, though, as the fix is brittle. When a new InvalidationListener is added referencing the to-be-removed menuItems, the memoryleak will re-occur and go unnoticed. - PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1860481040
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 21:51:13 GMT, Kevin Rushforth wrote: >> It seems like this is the best we can do for now. What is means is that we >> won't be able to build on a system with 3.8 - 3.19 and distribute a library >> that will use the new functionality when running on 3.20 or later. That's >> probably OK in this case, but is worth noting. >> >> What is important, and why the runtime check is needed (as noted above) is >> that we can build a production release of JavaFX on a system that has a >> later GTK and it will run on a system with an older GTK. It won't use the >> feature on those older systems, but won't crash. > > As an FYI, we use GTK 3.22 on our CI build machines so I wouldn't want to see > the build-time dependency move any higher than it currently is. The suggestion was to allow building on 16.04 - so it's easier to test. But it currently does not build because of code introduced on Platform Preferences API. On the other hand, maybe it's best to fail so we make sure building requires 3.20. - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1430688501
Re: [External] : Re: Issues running SwingNodeJDialogTest on Linux
Glad the problem is understood, and you were able to find a fix. -- Kevin On 12/17/2023 4:39 PM, Martin Fox wrote: I was running the tests using the default JDK 19 supplied by Ubuntu. In that environment AWT loads GTK 2 and then JavaFX throws an exception. The hang occurs after the test times out. The cleanup code calls Util.shutdown() which eventually calls into Platform.runLater(). The code in PlatformImpl.java then waits forever on the startupLatch which will never reach zero because of the exception thrown earlier. I switched to Java SE 19 where AWT loads GTK 3, JavaFX has no complaints, and the test proceeds. Thanks, Martin On Dec 15, 2023, at 1:35 PM, Kevin Rushforth wrote: Hmm. That's odd. you could pass "-Djdk.gtk.verbose=true" and see what's happening on the GTK loading side. Both AWT and JavaFX will print some info if that system property it set. To find out where the test process is hung you can use "jstack PID". If the test itself is hung, then that test would be a good candidate for adding a timeout parameter to the "@Test" tag. -- Kevin On 12/15/2023 11:36 AM, Martin Fox wrote: I’m having an issue with one of the robot tests on my Linux machine. Before I dig any further I could use some guidance on where to look. I’m running Ubuntu 22.04 on ARM64 (in a Parallels VM). The command line I’m using is: bash ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests SwingNodeJDialogTest The truly puzzling behavior is that the test never stops executing. I’ve got it running right now and it says it’s been executing for over 40 minutes. Shouldn’t the testing framework time this out? Under the hood I believe that the Glass code in launcher.c is detecting that a GTK 2 library is already loaded so it bails. From what I can tell both JavaFX and Swing should default to GTK 3 so that’s also puzzling. Any help would be appreciated. Thanks, Martin
[jfx17u] Integrated: 8181084: JavaFX show big icons in system menu on macOS with Retina display
On Mon, 18 Dec 2023 15:08:58 GMT, Johan Vos wrote: > clean backport of 8181084: JavaFX show big icons in system menu on macOS with > Retina di…splay > > Reviewed-by: jpereda, kcr This pull request has now been integrated. Changeset: 9fdfb679 Author:Johan Vos URL: https://git.openjdk.org/jfx17u/commit/9fdfb679891e0f529fdccaea86167d03e8cc3ea7 Stats: 103 lines in 14 files changed: 97 ins; 3 del; 3 mod 8181084: JavaFX show big icons in system menu on macOS with Retina display Backport-of: 2618bf8aeef3c9d9d923576e8a610f5e9b2123f1 - PR: https://git.openjdk.org/jfx17u/pull/174
Re: New API: Animation/Timeline improvement
Would making Timeline to use WeakReferences solve the issue without the need
for a new API?
-andy
From: openjfx-dev on behalf of John Hendrikx
Date: Friday, December 15, 2023 at 21:53
To: openjfx-dev
Subject: New API: Animation/Timeline improvement
Hi list,
I've noticed that Animations and Timelines are often a source of leaks,
and their clean-up is often either non-existent or incorrect. The
reason they cause leaks easily is because a running animation or
timeline is globally referred from a singleton PrimaryTimer. The
animation or timeline then refers to properties or event handlers which
refer to controls (which refer to parents and the entire scene).
For example:
- ScrollBarBehavior uses a Timeline, but neglects to clean it up. If it
was running at the time a Scene is detached from a Window, and that
Scene is left to go out of scope, it won't because Timeline refers it;
this can happen if the behavior never receives a key released event.
- ScrollBarBehavior has no dispose method overridden, so swapping Skins
while the animation is running will leave a Timeline active (it uses
Animation.INDEFINITE)
- SpinnerBehavior has flawed clean up; it attaches a Scene listener and
disables its timeline when the scene changed, but the scene doesn't have
to change for it to go out of scope as a whole... Result is that if you
have a spinner timeline running, and you close the entire window (no
Scene change happens), the entire Scene will still be referred. It also
uses an indefinite cycle count. It also lacks a dispose method, so
swapping Skins at a bad moment can also leave a reference.
I think these mistakes are common, and far too easy to make. The most
common use cases for animations revolve around modifying properties on
visible controls, and although animations can be used for purposes other
than animating UI controls, this is extremely rare. So it is safe to
say that in 99% of cases you want the animation to stop once a some Node
is no longer showing. For both the mentioned buggy behaviors above, this
would be perfect. A spinner stops spinning when no longer showing, and
a scroll bar stops scrolling when no longer showing. It is also likely
to apply for many other uses of timelines and animations.
I therefore want to propose a new API, either on Node or Animation (or
both):
/**
* Creates a new timeline which is stopped automatically when this Node
* is no longer showing. Stopping timelines is essential as they
may refer
* nodes even after they are no longer used anywhere, preventing
them from
* being garbage collected.
*/
Node.createTimeline(); // and variants with the various Timeline
constructors
And/or:
/**
* Links this Animation to the given Node, and stops the animation
* automatically when the Node is no longer showing. Stopping
animations
* is essential as they may refer nodes even after they are no
longer used
* anywhere, preventing them from being garbage collected.
*/
void stopWhenHidden(Node node);
The above API for Animation could also be provided through another
constructor, which takes a Node which will it be linked to.
Alternatives:
- Be a lot more diligent about cleaning up animations and timelines
(essentially maintain the status quo which has led to above bugs)
- Use this lengthy code fragment below:
Timeline timeline = new Timeline();
someNode.sceneProperty()
.when(timeline.statusProperty().map(status -> status !=
Status.STOPPED))
.flatMap(Scene::windowProperty)
.flatMap(Window::showingProperty)
.orElse(false)
.subscribe(showing -> {
if (!showing) timeline.stop();
});
The `when` line ensures that the opposite problem (Nodes forever
referencing Timelines) doesn't occur if you are creating a new Timeline
for each use (not recommended, but nonetheless a common occurrence).
--John
Re: Proposed schedule for JavaFX 22
As a reminder, Rampdown Phase 1 (RDP1) for JavaFX 22 starts on Thursday, January 11, 2024 at 16:00 UTC (08:00 US/Pacific time). Given the upcoming holidays, that's a little over 2 working weeks from now. During rampdown of JavaFX 22, the "master" branch of the jfx repo will be open for JavaFX 23 fixes. Please allow sufficient time for any feature that needs a CSR. New features should be far enough along in the review process that you can finalize the CSR no later than Thursday, January 4, or it is likely to miss the window for this release, in which case it can be targeted for JavaFX 23. We will follow the same process as in previous releases for getting fixes into JavaFX 22 during rampdown. I'll send a message with detailed information when we fork, but candidates for fixing during RDP1 are P1-P3 bugs (as long as they are not risky) and test or doc bugs of any priority. Some small enhancements might be considered during RDP1, but they require explicit approval; the bar will be high for such requests. -- Kevin On 9/21/2023 7:41 AM, Kevin Rushforth wrote: Here is the proposed schedule for JavaFX 22. RDP1: Jan 11, 2024 (aka “feature freeze”) RDP2: Feb 1, 2024 Freeze: Feb 29, 2024 GA: Mar 19, 2024 We plan to fork a jfx22 stabilization branch at RDP1. The start of RDP1, the start of RDP2, and the code freeze will be 16:00 UTC on the respective dates. Please let Johan or me know if you have any questions. -- Kevin
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 11:19:18 GMT, Jose Pereda wrote: >> This PR replaces the deprecated `gdk_pointer_grab` with `gdk_seat_grab`, and >> `gdk_pointer_ungrab ` with `gdk_seat_ungrab`, using runtime checks and >> wrapped functions for GTK 3.20+ (so systems without it still run with GTK >> 3.8+), and fixes the dragging issue on Wayland. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Add compile-time checks to GdkSeat The addition of the compile-time flags looks OK. I did a build with GTK 3.22 (so it compiles the new code, does the dlsym, and does the runtime check) and verified that there are no regressions when running on an older system (Ubuntu 16.04). I then did a full test run on our headful test systems, and there is one new test failure -- it seems to be intermittent, although fails pretty consistently on our Ubuntu 22.04 and Ubuntu 20.04 test systems. I can reproduce it locally on a VM running Ubuntu 22.04, where it fails about 1/2 the time with this patch applied (it fails more often on our physical test systems). DatePickerTest > testDatePickerSceneChange FAILED java.lang.AssertionError: Timeout: Failed to receive onAction call. at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.util.Util.waitForLatch(Util.java:400) at test.robot.javafx.scene.DatePickerTest.clickDatePickerCalendarPopup(DatePickerTest.java:90) at test.robot.javafx.scene.DatePickerTest.testDatePickerSceneChange(DatePickerTest.java:123) Not sure what to make of this. I am not aware of any problems with this test, but it's possible that your fix has exposed a latent issue either in the test or somewhere else. - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1861759137
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:29:00 GMT, Thiago Milczarek Sayao wrote: >> Okay, that is unfortunate (GTK docs inaccurate), but makes sense. >> >> I'll add the compile-time checks to `wrapped.c`. But then, using `dlsym` >> seems redundant, as we can simply call the seat functions directly? > > The check is to allow compilation and test on Ubuntu 16.04 - When shipping > the final build it should be built on gtk 3.20+, so both dlsym and the check > makes sense. > > So when running on Ubuntu 16.04: > - For testing on the platform, the #ifdef will make it possible to compile; > - For running the released built (which should be built on gtk 3.20+) dlsym > will fail and fallback; That's only if we want to keep building working on 16.04. I think it makes easier to test on it. But, it's already failing for Platform preferences API code. - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429888067
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:34:02 GMT, Thiago Milczarek Sayao wrote: >> The check is to allow compilation and test on Ubuntu 16.04 - When shipping >> the final build it should be built on gtk 3.20+, so both dlsym and the check >> makes sense. >> >> So when running on Ubuntu 16.04: >> - For testing on the platform, the #ifdef will make it possible to compile; >> - For running the released built (which should be built on gtk 3.20+) dlsym >> will fail and fallback; > > That's only if we want to keep building working on 16.04. I think it makes > easier to test on it. > But, it's already failing for Platform preferences API code. In any case, if `GdkSeat` is available only since 3.20, then we need to add the compile-time checks anyway, since minimum supported is 3.8. - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429891858
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:20:35 GMT, Jose Pereda wrote: >> I think the docs are wrong, I probably exists since 3.20, so maybe check for >> `GTK_CHECK_VERSION(3, 20, 0);`. >> >> >> This is the compilation error on Ubuntu 16.04: >> `/home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c:200:34: >> error: unknown type name ‘GdkSeat’ >> ` > > Okay, that is unfortunate (GTK docs inaccurate), but makes sense. > > I'll add the compile-time checks to `wrapped.c`. But then, using `dlsym` > seems redundant, as we can simply call the seat functions directly? The check is to allow compilation and test on Ubuntu 16.04 - When shipping the final build it should be built on gtk 3.20+, so both dlsym and the check makes sense. So when running on Ubuntu 16.04: - For testing on the platform, the #ifdef will make it possible to compile; - For running the released built (which should be built on gtk 3.20+) dlsym will fail and fallback; - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429875706
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:09:29 GMT, Thiago Milczarek Sayao
wrote:
>> I take `GdkSeat` is available since GTK 3.0?
>> https://docs.gtk.org/gdk3/class.Seat.html
>>
>> But don't we have a minimum set on 3.8.0?
>>
>> Would this work?
>>
>> #if GTK_CHECK_VERSION(3, 0, 0)
>> static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
>> GdkSeat * wrapped_gdk_display_get_default_seat (GdkDisplay *display)
>> {...}
>> #endif
>>
>> ...
>>
>> #if GTK_CHECK_VERSION(3, 0, 0)
>> GdkSeat* seat =
>> wrapped_gdk_display_get_default_seat(gdk_window_get_display(window));
>> if (seat != NULL && _gdk_seat_grab != NULL) {
>> *status = ...
>> return TRUE;
>> }
>> #endif
>
> I think the docs are wrong, I probably exists since 3.20, so maybe check for
> `GTK_CHECK_VERSION(3, 20, 0);`.
>
>
> This is the compilation error on Ubuntu 16.04:
> `/home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c:200:34:
> error: unknown type name ‘GdkSeat’
> `
Okay, that is unfortunate (GTK docs inaccurate), but makes sense.
I'll add the compile-time checks to `wrapped.c`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429864963
Compiling Plataform Preferences on Ubuntu 16.04
Hi, I get some compilation errors when building JavaFX on Ubuntu 16.04, which seems related to the newly introduced Platform preferences API. Should we bother to make it work? In file included from /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp:45:0: /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.h:30:7: error: override controls (override/final) only available with -std=c++11 or -std=gnu++11 [-Werror] class PlatformSupport final ^ /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.h:35:47: error: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11 [-Werror] PlatformSupport(PlatformSupport const&) = delete; ^ /home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.h:36:58: error: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11 [-Werror] PlatformSupport& operator=(PlatformSupport const&) = delete; -- Thiago.
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 09:46:23 GMT, Jose Pereda wrote:
>> modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c line 197:
>>
>>> 195: return TRUE;
>>> 196: }
>>> 197: return FALSE;
>>
>> I did try to test on Ubuntu 16.04 and compilation failed (no surprise
>> because `GdkSeat` does not exists there). Suggestion to keep `#ifdef` here
>> and `return FALSE` on `#else` so it would still compile on Ubuntu 16.04 and
>> older systems. Will need to `#ifdef` all `GdkSeat` usage.
>
> I take `GdkSeat` is available since GTK 3.0?
> https://docs.gtk.org/gdk3/class.Seat.html
>
> But don't we have a minimum set on 3.8.0?
>
> Would this work?
>
> #if GTK_CHECK_VERSION(3, 0, 0)
> static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
> GdkSeat * wrapped_gdk_display_get_default_seat (GdkDisplay *display)
> {...}
> #endif
>
> ...
>
> #if GTK_CHECK_VERSION(3, 0, 0)
> GdkSeat* seat =
> wrapped_gdk_display_get_default_seat(gdk_window_get_display(window));
> if (seat != NULL && _gdk_seat_grab != NULL) {
> *status = ...
> return TRUE;
> }
> #endif
I think the docs are wrong, I probably exists since 3.20, so maybe check for
`GTK_CHECK_VERSION(3, 20, 0);`.
This is the compilation error on Ubuntu 16.04:
`/home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c:200:34:
error: unknown type name ‘GdkSeat’
`
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429852839
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 09:46:23 GMT, Jose Pereda wrote:
>> modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c line 197:
>>
>>> 195: return TRUE;
>>> 196: }
>>> 197: return FALSE;
>>
>> I did try to test on Ubuntu 16.04 and compilation failed (no surprise
>> because `GdkSeat` does not exists there). Suggestion to keep `#ifdef` here
>> and `return FALSE` on `#else` so it would still compile on Ubuntu 16.04 and
>> older systems. Will need to `#ifdef` all `GdkSeat` usage.
>
> I take `GdkSeat` is available since GTK 3.0?
> https://docs.gtk.org/gdk3/class.Seat.html
>
> But don't we have a minimum set on 3.8.0?
>
> Would this work?
>
> #if GTK_CHECK_VERSION(3, 0, 0)
> static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
> GdkSeat * wrapped_gdk_display_get_default_seat (GdkDisplay *display)
> {...}
> #endif
>
> ...
>
> #if GTK_CHECK_VERSION(3, 0, 0)
> GdkSeat* seat =
> wrapped_gdk_display_get_default_seat(gdk_window_get_display(window));
> if (seat != NULL && _gdk_seat_grab != NULL) {
> *status = ...
> return TRUE;
> }
> #endif
You're right. GdkSeat should be available. Maybe It needs an include?
/home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c:151:8:
error: unknown type name ‘GdkSeat’
static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
This is when compiling on Ubuntu 16.04.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429842823
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Sat, 16 Dec 2023 22:21:55 GMT, Thiago Milczarek Sayao
wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove compile-time if checks
>
> modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c line 197:
>
>> 195: return TRUE;
>> 196: }
>> 197: return FALSE;
>
> I did try to test on Ubuntu 16.04 and compilation failed (no surprise because
> `GdkSeat` does not exists there). Suggestion to keep `#ifdef` here and
> `return FALSE` on `#else` so it would still compile on Ubuntu 16.04 and older
> systems. Will need to `#ifdef` all `GdkSeat` usage.
I take `GdkSeat` is available since GTK 3.0?
https://docs.gtk.org/gdk3/class.Seat.html
But don't we have a minimum set on 3.8.0?
Would this work?
#if GTK_CHECK_VERSION(3, 0, 0)
static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
GdkSeat * wrapped_gdk_display_get_default_seat (GdkDisplay *display)
{...}
#endif
...
#if GTK_CHECK_VERSION(3, 0, 0)
GdkSeat* seat =
wrapped_gdk_display_get_default_seat(gdk_window_get_display(window));
if (seat != NULL && _gdk_seat_grab != NULL) {
*status = ...
return TRUE;
}
#endif
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429810712
