8369836: Update HeaderBar API #1936
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mstr2 this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please update the title of this pull request to just the issue ID. |
Webrevs
|
|
Mailing list message from Andy Goryachev on openjfx-dev: Can you clarify what you mean by "aligning with" BorderPane? Does it mean we are trying to propagate somewhat misleading terminology that was used by the BorderPane (setLeft() in RTL mode results in the added node on the right side, so it should really be named something like setLeading() instead of setLeft()). Thanks, From: openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of Michael Strau? <mstrauss at openjdk.org> 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 two changes for the `HeaderBar` control: 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`. ------------- Commit messages: Changes: https://git.openjdk.org/jfx/pull/1936/files PR: https://git.openjdk.org/jfx/pull/1936 |
|
Mailing list message from Michael Strauß on openjfx-dev: Yes, exactly. "Left" and "right" is used in several places in JavaFX On Tue, Oct 14, 2025 at 6:52?PM Andy Goryachev
|
|
Mailing list message from Andy Goryachev on openjfx-dev: I would rather clarify the incorrect labels in the existing components. We obviously cannot change setLeft() - perhaps we should add explanation to the corresponding javadoc explaining RTL behavior. I would like to avoid making the same mistake going forward. -andy From: Michael Strau? <michaelstrau2 at gmail.com> Yes, exactly. "Left" and "right" is used in several places in JavaFX On Tue, Oct 14, 2025 at 6:52?PM Andy Goryachev
-------------- next part -------------- |
|
Mailing list message from Michael Strauß on openjfx-dev: Considering that the "left" and "right" terminology is deeply On Tue, Oct 14, 2025 at 7:08?PM Andy Goryachev
|
|
Reviewers: @andy-goryachev-oracle @kevinrushforth |
I think this is a good choice from a consistency point of view. |
|
There's another thing that I think will improve the The We already have an attached property of this kind: It should be noted that this will be the first instance of an observable attached property in JavaFX. So while regular attached properties have a getter/setter pair like this... class HBox {
static Insets getMargin(Node);
static void setMargin(Node, Insets);
}...an observable attached property will have an observable property getter: class HeaderBar {
static ReadOnlyObjectProperty<Dimension2D> leftSystemInsetProperty(Stage);
static Dimension2D getLeftSystemInset(Stage);
}The form of the property getter shouldn't be controversial, as it follows the existing getter/setter form for attached properties. What needs to be clarified, however, is what For the |
There was a problem hiding this comment.
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.
| * is an empty {@code Dimension2D}. | ||
| * | ||
| * @param stage the {@code Stage} | ||
| * @return the {@code leftSystemInset} property |
There was a problem hiding this comment.
Needs @since 26 javadoc tag (for all new methods).
| * in a right-to-left layout. | ||
| * The left area of the {@code HeaderBar}. | ||
| * | ||
| * @defaultValue {@code null} |
There was a problem hiding this comment.
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).
| this.leftSystemInset = new ReadOnlyObjectWrapper<>(stage, "HeaderBar.leftSystemInset", EMPTY); | ||
| this.rightSystemInset = new ReadOnlyObjectWrapper<>(stage, "HeaderBar.rightSystemInset", EMPTY); | ||
| this.minSystemHeight = new ReadOnlyDoubleWrapper(stage, "HeaderBar.minSystemHeight"); |
There was a problem hiding this comment.
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.
2 participants
The
HeaderBarcontrol currently has three areas:leading,center, andtrailing. Additionally, there'sleftSystemInsetandrightSystemInset, 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
HeaderBarcontrol:leadingtoleft, andtrailingtoright, which aligns the terminology withBorderPane.leftSystemInsetandrightSystemInsetfor RTL.leftSystemInset,rightSystemInset, andminSystemHeightattached properties forStage.With this change, the
HeaderBarcontrol is more semantically consistent and easier to use, and the renamedleftandrightareas now show its close relationship withBorderPane./csr
/reviewers 2
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1936/head:pull/1936$ git checkout pull/1936Update a local copy of the PR:
$ git checkout pull/1936$ git pull https://git.openjdk.org/jfx.git pull/1936/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1936View PR using the GUI difftool:
$ git pr show -t 1936Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1936.diff
Using Webrev
Link to Webrev Comment