On Fri, 17 Oct 2025 02:22:04 GMT, Michael Strauß <[email protected]> wrote:
>> The `HeaderBar` control currently has three areas: `leading`, `center`, and
>> `trailing`. Additionally, there's `leftSystemInset` and `rightSystemInset`,
>> which are not RTL adjusted. I've come to the understanding that there is no
>> particularly good reason for this, because every time you would want to use
>> this information for layout purposes, it should also be adjusted for RTL.
>>
>> With this in mind, there are three changes for the `HeaderBar` control:
>> 1. Rename `leading` to `left`, and `trailing` to `right`, which aligns the
>> terminology with `BorderPane`.
>> 2. Adjust `leftSystemInset` and `rightSystemInset` for RTL.
>> 3. Make `leftSystemInset`, `rightSystemInset`, and `minSystemHeight`
>> attached properties for `Stage`.
>>
>> With this change, the `HeaderBar` control is more semantically consistent
>> and easier to use, and the renamed `left` and `right` areas now show its
>> close relationship with `BorderPane`.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make leftSystemInset/rightSystemInset/minSystemHeight attached properties
The API changes look good. I left a couple inline comments and questions.
Your proposal to add attached Properties seems reasonable and natural. My only
question is whether the property name should be prefixes with the
fully-qualified class name rather than just the unqualified name.
modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line
287:
> 285: *
> 286: * @param stage the {@code Stage}
> 287: * @return the {@code leftSystemInset} property
Needs `@since 26` javadoc tag (for all new methods).
modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line
429:
> 427: * The left area of the {@code HeaderBar}.
> 428: *
> 429: * @defaultValue {@code null}
Needs `@since 26` javadoc tag here and all other renamed methods (if the method
name or signature changes then it is a new method in 26).
modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line
888:
> 886: this.leftSystemInset = new ReadOnlyObjectWrapper<>(stage,
> "HeaderBar.leftSystemInset", EMPTY);
> 887: this.rightSystemInset = new ReadOnlyObjectWrapper<>(stage,
> "HeaderBar.rightSystemInset", EMPTY);
> 888: this.minSystemHeight = new ReadOnlyDoubleWrapper(stage,
> "HeaderBar.minSystemHeight");
Should the classname prefix in the property `name` be fully qualified? If it
were, then a utility could find the corresponding method(s) from the bean using
the convention that if a name contains a `.`, it is an attached property with
the bean being an argument to the named (static) method.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1936#pullrequestreview-3371685356
PR Review Comment: https://git.openjdk.org/jfx/pull/1936#discussion_r2456487752
PR Review Comment: https://git.openjdk.org/jfx/pull/1936#discussion_r2456509801
PR Review Comment: https://git.openjdk.org/jfx/pull/1936#discussion_r2456522184
