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

[Michael Strauß]

On Thu, 24 Apr 2025 08:12:57 GMT, Markus Mack <mmack at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   documentation
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/TKScene.java line 89:
> 
>> 87:     public TKClipboard createDragboard(boolean isDragSource);
>> 88: 
>> 89:     default void processOverlayCSS() {}
> 
> This seems to be almost the only place where the quantum toolkit is made aware of CSS and layout. Could the ViewSceneOverlay have been directly added to the javafx.scene.Scene instead of the toolkit scene?

Yes, possibly. The reason why it's done like this is because the implementation is partially lifted from the existing full-screen overlay to minimize total changes. Maybe this would be a good candidate for a future refactor?

> modules/javafx.graphics/src/main/java/javafx/stage/StageStyle.java line 84:
> 
>> 82:      * bar area of the stage.
>> 83:      * <p>
>> 84:      * This is a conditional feature, to check if it is supported see {@link Platform#isSupported(ConditionalFeature)}.
> 
> When someone who's planning on adopting the new title bar feature reads this, they might ask themselves which platforms are the supported ones, and if there are plans to support missing ones. Is there any place outside the codebase where this is actually explained? Can we link to it?

It is actually documented in `ConditionalFeature.EXTENDED_WINDOW`.

> modules/javafx.graphics/src/main/java/javafx/stage/StageStyle.java line 113:
> 
>> 111:      * of the {@code Scene} to remain easily recognizable. Applications should set the scene fill to a color
>> 112:      * that matches the brightness of the user interface, even if the scene fill is not visible because it
>> 113:      * is obscured by other controls.
> 
> Can we add the reasoning for this? When deciding on the actual color, it would be useful to know when exactly it may be shown.

How a scene background is shown is already described in `Scene.fill`. Note that if we get #1655, we might want to change this part of the specification such that the color scheme of the buttons adjusts to the color scheme of the scene (i.e. `Scene.Preferences.colorScheme`) and not to its background fill.

> modules/javafx.graphics/src/main/java/javafx/stage/StageStyle.java line 117:
> 
>> 115:      * <h4>Custom header buttons</h4>
>> 116:      * If more control over the header buttons is desired, applications can opt out of the default header buttons
>> 117:      * by setting {@link HeaderBar#setPrefButtonHeight(Stage, double)} to zero and providing custom header buttons
> 
> Setting height to zero to disable something is unfortunately often misused without actually disabling anything. Even though implemented correctly here, I'd prefer a more semantic way of doing this, e.g. by adding a "showHeaderButtons" property.

I agree when it actually makes a semantic difference. For example, there's a difference between setting `Node.opacity` to zero, and setting `Node.visible` to `false`. In the first case, the node will still receive events, while in the second case it won't. But what's the semantic difference between non-existing header buttons, and header buttons with zero height?

One argument against setting `prefButtonHeight` to zero could be that it special-cases one specific value (or two, because there's also the special value `USE_DEFAULT_SIZE`). In general, the property only describes a _preferred_ height, which the toolkit is free to honor (in any way it sees fit) or to ignore entirely. However, the two special values 0 and `USE_DEFAULT_SIZE` are specified to be honored by all toolkits in all cases.

> modules/javafx.graphics/src/main/native-glass/mac/GlassWindow.h line 53:
> 
>> 51:     BOOL                isDecorated;
>> 52:     BOOL                isResizable;
>> 53:     BOOL                isStandardButtonsVisible;
> 
> showStandardButtons? The surrounding code does not seem to follow a strict "isXyz" naming convention for bools, so we could improve grammar here.

Except for `suppressWindowMoveEvent` and `suppressWindowResizeEvent`, the code does seem to follow the "isFoo" convention...

> modules/javafx.graphics/src/main/native-glass/win/GlassWindow.cpp line 617:
> 
>> 615:         // Since DefWindowProc() is not called, call the mouse menu handler directly
>> 616:         HandleViewMenuEvent(GetHWND(), WM_CONTEXTMENU, (WPARAM) GetHWND(), ::GetMessagePos ());
>> 617:         //::DefWindowProc(GetHWND(), msg, wParam, lParam);
> 
> Space after GetMessagePos, leftover code comment?

I think the comment is there to drive the point home that `DefWindowProc` is not called? It's pre-existing code that I've just moved around.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2058604759
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2058604353
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2058604510
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2058604597
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2058605178
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2058604780