On Wed, 4 Jun 2025 15:00:44 GMT, Kevin Rushforth <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> enable preview feature system properties for tests
>
> build.gradle line 2186:
>
>> 2184: systemProperty
>> 'junit.jupiter.execution.timeout.lifecycle.method.default',
>> JUNIT_LIFECYCLE_TIMEOUT
>> 2185: systemProperty 'javafx.enablePreview', 'true'
>> 2186: systemProperty 'javafx.suppressPreviewWarning', 'true'
>
> This seems a good approach.
>
> it might make it a little more difficult if we wanted to test the logic that
> throws an exception when `javafx.enablePreview` is not `"true"`, but that
> would still be possible by having that test explicitly set the property to
> the empty string in an `@Before` method, and then restoring it in an `@After`
> method. Probably not worth doing anyway.
Yes, this seems like a good solution waiting for a problem. I agree that we can
do that when we discover that problem.
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 1403:
>
>> 1401: }
>> 1402:
>> 1403: private class UndecoratedMoveResizeHelper {
>
> Have you verified that removing this doesn't result in any loss of
> functionality?
Yes, one of the first things that I discovered in this project was that
`UndecoratedMoveResizeHelper` was effectively unused.
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkWindow.java
> line 43:
>
>> 41:
>> 42: if (isExtendedWindow()) {
>> 43:
>> prefHeaderButtonHeightProperty().subscribe(this::onPrefHeaderButtonHeightChanged);
>
> This subscription is never canceled. Could this result in a leak?
The `prefHeaderButtonHeight` property is a field on the same class. We only
subscribe to this property within the context of this class, so it won't
prevent `GtkWindow` from being eligible for GC.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2127169914
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2127166757
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2127161222
