On Tue, 15 Apr 2025 10:56:31 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Implementation of >> [`StageStyle.EXTENDED`](https://gist.github.com/mstr2/0befc541ee7297b6db2865cc5e4dbd09). > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > documentation I did some testing on windows 11 and macOS 15.4 and did a full review of the code of this PR to help moving it forward. The things I noted are all pretty minor, this is in very good shape. Things I noticed while testing: - Placing Labels and Buttons in the header bar and styling it works as intended. - When fiddling with Windows screen scaling settings, the default header bar buttons look good. I once managed to see the shape of maximize button rendered with slightly blurry top and bottom lines of the square shape. I couldn't reproduce this, unfortunately. I wonder if having pre-rendered button shapes for all allowed scaling factors would guarantee this cannot happen, or if it would even make things worse. It's not a real problem either way. - When placing a MenuBar in the HeaderBar, dragging the window by clicking on the menu will open the menu and drag the window away from the opened window, which looks broken, and ComboBox and TextField don't work at all. While the fix is documented well (setting HeaderBar.draggable to false for those controls), maybe this should be set by default for all `Control` `Node`s? This would need additional documentation though.  - On Windows, when resizing the window horizontally to small sizes, the header button symbols are drawn over any other content in the header bar's center region, which looks broken. Can the layout be made stricter to prevent this? Or always draw the background behind these symbols?  modules/javafx.graphics/src/main/java/com/sun/glass/ui/View.java line 70: > 68: return false; > 69: } > 70: public boolean handleMenuEvent(View view, int x, int y, int xAbs, It may be worth adding code documentation describing what is returned here modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/HeaderButtonBehavior.java line 90: > 88: > 89: switch (type) { > 90: case CLOSE -> getStage().ifPresent(Stage::close); The getStage() calls could be pulled before the switch and return out of the method if null, instead of the ifPresent closures. 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? modules/javafx.graphics/src/main/java/com/sun/javafx/tk/TKSceneListener.java line 87: > 85: boolean _direct, boolean _inertia); > 86: > 87: public boolean menuEvent(double x, double y, double xAbs, double yAbs, since not all xyEvent methods return something, it would be good to add code docs explaining the semantics of this returned boolean value. 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? 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. 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. 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. 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? ------------- PR Review: https://git.openjdk.org/jfx/pull/1605#pullrequestreview-2778375918 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2050513166 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057822368 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057811120 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057829027 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057800013 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057803486 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057806868 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057831626 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r2057817035
