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

Sat Feb 15 01:19:10 UTC 2025

On Fri, 14 Feb 2025 15:43:38 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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?

Yeah, doesn't look nice, but I wanted to match the style of the surrounding methods.

> 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?

This class is not API, so it doesn't matter here. However, it matters in `HeaderBar.leftSystemInset/rightSystemInset`, which are both of type `Dimension2D`. The system insets are not arbitrary rectangles, they are always aligned with the top-left and top-right edges of the window. As such, the only necessary information is their spatial extent, which is best represented as a `Dimension2D`.
I'm not sure what you mean by asymmetrical padding. Can you elaborate?

> 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).

`HeaderButtonOverlay` is not API, it's an internal implementation detail. The javadoc is just informational for future JavaFX developers.

> 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.

Needs to wait until we have something like this in an accessible place...

> 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?

`HeaderButtonOverlay` inherits its node orientation from the scene, which is under the control of the application developer. On the other hand, `allowRtl` is under the control of JavaFX developers (that's us), specifying whether the selected header button implementation supports RTL layouts at all. Currently, all header button implementations do.

The property is there to make all aspects of `HeaderButtonOverlay` styleable, so that future JavaFX developers can add other header button styles that may have fixed locations (i.e. don't move from one side to the other in RTL mode).

> 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`?

Yes!

> 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?

I've tried, but the additional conditions make it end up more verbose and funny-looking.

> 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

Yes, that's a very common situation. It happens when the cursor is on the header bar, but not on a header button.

> 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.

We can also consider exposing this as API (not with this PR, though). If it's useful for us, it might also be useful for application developers.

> 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()` ?

Sure, but the downside is that everything gets more bloated for little benefit. Right now, these properties are accessed in derived classes without any checks, since they are final and non-null. In this case, I think ease of use is more important.

> 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?

Yes, it is. Once we get preview features in, I'll annotate the new API elements.

> 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?

Yes, it will.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968295
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968317
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968351
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968405
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968436
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968452
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968485
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968472
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968540
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968558
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968567
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968572