RFR: 8313424: JavaFX controls in the title bar [v49]

Andy Goryachev angorya at openjdk.org
Fri Feb 14 17:43:28 UTC 2025


On Thu, 13 Feb 2025 19:10:15 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Implementation of [`EXTENDED`](https://gist.github.com/mstr2/0befc541ee7297b6db2865cc5e4dbd09) and `EXTENDED_UTILITY` stage style.
>
> Michael Strauß has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - typo
>  - update copyright headers

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 715:

> 713: 
> 714:     protected abstract boolean _supportsExtendedWindows();
> 715:     public final boolean supportsExtendedWindows() {

missing newline after L714?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonMetrics.java line 39:

> 37:  * @param minHeight the minimum height of the window buttons
> 38:  */
> 39: public record HeaderButtonMetrics(Dimension2D leftInset, Dimension2D rightInset, double minHeight) {

would it make more sense to use Insets instead of Dimension2D?  What if the app calls for asymmetrical padding?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 153:

> 151: 
> 152:     private static final CssMetaData<HeaderButtonOverlay, Number> BUTTON_DEFAULT_HEIGHT_METADATA =
> 153:         new CssMetaData<>("-fx-button-default-height", StyleConverter.getSizeConverter()) {

The new styleables need to be documented in cssref.html as a part of this PR, right?
(The javadoc for this class will not be visible in the API specification).

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 209:

> 207: 
> 208:     private static final List<CssMetaData<?, ?>> METADATA =
> 209:         Stream.concat(getClassCssMetaData().stream(),

`RichUtils.combine()` L419 avoids creating intermediary objects.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 283:

> 281:      */
> 282:     private final StyleableBooleanProperty allowRtl =
> 283:         new SimpleStyleableBooleanProperty(ALLOW_RTL_METADATA, this, "allowRtl", true) {

what is the rationale for this property?
Instead, I think, it should look at the `Node::getEffectiveNodeOrientation()` which either inherits the value from the parent or allows the app to override for a specific component (in other words, we already have this functionality for free).  

Or am I missing something?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 328:

> 326: 
> 327:         var updateStylesheetSubscription = sceneProperty()
> 328:             .flatMap(Scene::fillProperty)

will this work if the value of scene is `null`?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 405:

> 403:         } : null;
> 404: 
> 405:         if (type == MouseEvent.ENTER || type == MouseEvent.MOVE || type == MouseEvent.DRAG) {

minor: would it be clearer to use a `switch(type)` here instead?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 406:

> 404: 
> 405:         if (type == MouseEvent.ENTER || type == MouseEvent.MOVE || type == MouseEvent.DRAG) {
> 406:             handleMouseOver(node);

could there be a situation when node is null but we pass it on to the method?  also L410, L412

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 586:

> 584:             totalHeight = snapSizeY(getEffectiveButtonHeight());
> 585:         } else {
> 586:             totalHeight = snapSizeY(Math.max(button1Height, Math.max(button2Height, button3Height)));

+1 for correct snapping!

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 632:

> 630:     }
> 631: 
> 632:     private static double boundedSize(double min, double pref, double max) {

minor:
it would be nice to move this to some common utilities class (there is already such a method in c.s.j.scene.control.skin.Utils class).

along with two previous methods.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 678:

> 676:             orderedButtons.add(this);
> 677:             buttonOrder.set(order);
> 678:             glyph.getStyleClass().setAll("glyph");

"glyph": though apt, the name might be somewhat confusing.  maybe "icon"?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 299:

> 297:     }
> 298: 
> 299:     /**

since the majority of windows is expected to be DECORATED, would it make more sense to move these properties into a separate container akin to `Node.getMiscProperties()` ?

modules/javafx.graphics/src/main/java/javafx/application/ConditionalFeature.java line 163:

> 161:      */
> 162:     EXTENDED_WINDOW,
> 163: 

Is this PR blocked by the Preview Feature Support #1359?

modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 323:

> 321:         minHeightProperty();
> 322: 
> 323:         ObservableValue<Stage> stage = sceneProperty()

will this handle `null` scene?

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956349123
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956353096
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956357334
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956364194
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956372516
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956374877
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956464424
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956462896
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956484613
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956488129
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956491008
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956495596
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956505679
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956507647


More information about the openjfx-dev mailing list