On Sat, 4 Jul 2026 21:05:02 GMT, Marius Hanl <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> lazy properties
>
> modules/javafx.controls/src/main/java/javafx/scene/control/DialogPane.java
> line 451:
>
>> 449: if (headerBar == null) {
>> 450: headerBar = new SimpleObjectProperty<>(this, "headerBar") {
>> 451: WeakReference<HeaderBar> wref = new
>> WeakReference<>(null);
>
> `oldHeaderBar` might be a bit better and follows the other properties in
> other controls
I'd say it's not the old value, but the current value of the property. Changed
it to `currentValue`.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderDragType.java
> line 99:
>
>> 97: * In contrast to {@link #DRAGGABLE}, which positively identifies a
>> node as a draggable part of the
>> 98: * {@code HeaderBar}, this option excludes a node from
>> draggable-area hit testing: the header bar
>> 99: * behaves as if the node was not present and continues hit testing
>> unimpeded.
>
> `TRANSPARENT` was the the hardest for me to understand. I think you did a
> good job already explaining it - But to fully understand, one more sentence
> what it means that the hit testing continues would help a lot to clarify IMO.
I've added a sentence that hit-testing continues with the next eligible node
under the transparent node.
> modules/javafx.graphics/src/main/java/javafx/stage/StageStyle.java line 115:
>
>> 113: * }</pre>
>> 114: *
>> 115: * <h4>Title text</h4>
>
> Not just the title text but also the application icon in case of Windows and
> probably most of Linux. Probably good to mention as well? Since for both,
> clicking on the icon gives a `ContextMenu` which is also lost - so good to
> know for the developer.
I've added language that an application icon is not provided for free. However,
I didn't include any mention of a context menu: this is subsumed by
"Applications [...] need to provide their own implementation", as this
specification is not the right place to enumerate platform-specific behaviors.
> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/HeaderBarTest.java
> line 103:
>
>> 101: }
>> 102:
>> 103: <T> T getAttachedProperty(String name) {
>
> Should this be called `getExtendedProperties` as well?
I'd say no, because this method specifically retrieves a single attached
property from the extended stage.
> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/HeaderBarTest.java
> line 167:
>
>> 165: @Test
>> 166: void attachedPropertyPublishesExplicitValue() {
>> 167: var colorScheme = new ColorScheme[1];
>
> Minor: `AtomicReference` could be used. I personally like it a bit more
I don't like that one: `AtomicReference` is an atomic reference, not a
general-purpose mutable reference wrapper.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2188#discussion_r3524196586
PR Review Comment: https://git.openjdk.org/jfx/pull/2188#discussion_r3524196752
PR Review Comment: https://git.openjdk.org/jfx/pull/2188#discussion_r3524196856
PR Review Comment: https://git.openjdk.org/jfx/pull/2188#discussion_r3524196918
PR Review Comment: https://git.openjdk.org/jfx/pull/2188#discussion_r3524196876
