On Thu, 24 Apr 2025 08:12:57 GMT, Markus Mack <[email protected]> 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
