RFR: 8313424: JavaFX controls in the title bar [v19]
Andy Goryachev
angorya at openjdk.org
Tue Nov 5 00:22:43 UTC 2024
On Fri, 1 Nov 2024 23:38:52 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
>> This PR is a new take on a highly requested feature: JavaFX controls in the header bar (see also #594 for an earlier iteration).
>>
>> This is a feature with many possible ways to skin the cat, and it has taken quite a bit of effort to come up with a good user model. In contrast to the previous iteration, the focus has shifted from providing an entirely undecorated window to providing a window with a user-configurable header bar.
>>
>> The customizable header bar is a new layout container: `javafx.scene.layout.HeaderBar`. It has three areas that accept child nodes: leading, center, and trailing. `HeaderBar` also automatically adjusts for the placement of the default window buttons (minimize, maximize, close) on the left or right side of the window.
>>
>> The customizable header bar is combined with a new `EXTENDED` stage style, which extends the client area into the header bar area. The new extended stage style is supported on Windows, macOS, and Linux. For platforms that don't support this stage style, it automatically downgrades to `DECORATED`.
>>
>> This is how it looks like on each of the three operating systems:
>>
>> 
>>
>> The window buttons (minimize, maximize, close) are provided by JavaFX, not by the application developer. This makes it easier to get basic window functionality without recreating the entirety of the window controls for all platforms.
>>
>> ## Usage
>> This is a minimal example that uses a custom header bar with a `TextField` in the center area. `HeaderBar` is usually placed in the top area of a `BorderPane` root container:
>>
>> public class MyApp extends Application {
>> @Override
>> public void start(Stage stage) {
>> var headerBar = new HeaderBar();
>> headerBar.setCenter(new TextField());
>>
>> var root = new BorderPane();
>> root.setTop(headerBar);
>>
>> stage.setScene(new Scene(root));
>> stage.initStyle(StageStyle.EXTENDED);
>> stage.show();
>> }
>> }
>>
>> To learn more about the details of the API, refer to the documentation of `StageStyle.EXTENDED` and `HeaderBar`.
>>
>> ## Platform integration
>> The implementation varies per platform, and ranges from pretty easy to quite involved:
>> 1. **macOS**: The window buttons are provided by macOS, we just leave an empty area where the window buttons will appear. The client area is extended to cover the entire window by setting the `NSW...
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits:
>
> - Merge branch 'master' into feature/extended-window
> - Merge branch 'master' into feature/extended-window
> - macOS: dynamically adapt toolbar style to headerbar height
> - fix header bar height flicker
> - NPE
> - fix peer access outside of synchronizer
> - improve title text documentation
> - macOS: hide window title
> - better documentation
> - set minHeight to native height of title bar
> - ... and 13 more: https://git.openjdk.org/jfx/compare/58cd76a8...9b63892d
first batch of comments up until and including HeaderBar.
to be continued...
buildSrc/win.gradle line 329:
> 327: "winmm.lib", "imm32.lib", "shell32.lib", "Uiautomationcore.lib", "dwmapi.lib", "uxtheme.lib",
> 328: "/DELAYLOAD:user32.dll", "/DELAYLOAD:urlmon.dll", "/DELAYLOAD:winmm.dll", "/DELAYLOAD:shell32.dll",
> 329: "/DELAYLOAD:Uiautomationcore.dll", "/DELAYLOAD:dwmapi.dll", "/DELAYLOAD:uxtheme.dll"]).flatten()
minor suggestion: placing each entry on separate line might simplify maintenance and reduce merge conflicts.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 170:
> 168: * Indicates that the window is modal which affects whether the window is minimizable.
> 169: */
> 170: @Native public static final int MODAL = 1 << 10;
minor: would it be better to add EXTENDED after the MODAL to minimize the change? Do the actual values matter?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 267:
> 265: if ((styleMask & UNIFIED) != 0 && (styleMask & EXTENDED) != 0) {
> 266: throw new RuntimeException("UNIFIED and EXTENDED cannot be combined");
> 267: }
is it even possible to create a window with both `UNIFIED` and `EXTENDED` set?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 74:
> 72: * <li>{@link Region} — {@code window-button}, {@code maximize-button}
> 73: * <li>{@link Region} — {@code window-button}, {@code close-button}
> 74: * </ul>
1. I think this part (css ref) is rather important and maybe should be a part of this PR. It would be nice to be able to review the CSS reference as a part of this PR.
2. if you don't kind, could you please link your JEP to the PR and the ticket?
3. could you please include the "application title" row in the table in the JEP?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 78:
> 76: *
> 77: * <table style="white-space: nowrap">
> 78: * <caption>CSS properties of {@code window-button-container}</caption>
The new CSS properties need to be included in `cssref.html`, along with the description of platform availability.
Should this be done as part of this PR? (that would be my preference)
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 104:
> 102: * </thead>
> 103: * <tbody><tr>
> 104: * <th>-fx-button-order</th><td><integer></td><td>0/1/2</td>
what happens when CSS sets all buttons to the same value?
should this be something that user has control over vs. defined by the platform?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 199:
> 197: */
> 198: private final StyleableBooleanProperty allowRtl =
> 199: new SimpleStyleableBooleanProperty(ALLOW_RTL_METADATA, this, "allowRtl", true) {
why is `allowRtl` a property and not a simple flag?
why is it allowed to be changed via CSS via `-fx-allow-rtl`?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 273:
> 271: public ButtonType buttonAt(double x, double y) {
> 272: for (var button : orderedButtons) {
> 273: if (button.isVisible() && button.getBoundsInParent().contains(x, y)) {
are we using the right coordinates? x,y seems to be in the "window" coordinates, while button.parent is... not. Shouldn't you do a proper conversion here?
(also notice my earlier comment about the Close button in RTL mode)
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 286:
> 284: * @param type the event type
> 285: * @param button the button type
> 286: * @param x the X coordinate, in pixels relative to the window
this is an internal class, but is it correct to refer to the "window coordinates"? what are these? platform window coordinates?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 339:
> 337:
> 338: private void handleMouseUp(Node node, ButtonType buttonType) {
> 339: boolean releasedOnButton = buttonAtMouseDown == node;
very minor stylistic suggestion:
boolean releasedOnButton = (buttonAtMouseDown == node);
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 354:
> 352: case MINIMIZE -> stage.setIconified(true);
> 353: case MAXIMIZE -> stage.setMaximized(!stage.isMaximized());
> 354: case CLOSE -> stage.close();
should this `switch` have a default case?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 360:
> 358:
> 359: private void onFocusedChanged(boolean focused) {
> 360: for (var node : new Node[] {minimizeButton, maximizeButton, closeButton}) {
how is this better than
minimizeButton.pseudoClassStateChanged(ACTIVE_PSEUDOCLASS, focused);
maximizeButton.pseudoClassStateChanged(ACTIVE_PSEUDOCLASS, focused);
closeButton.pseudoClassStateChanged(ACTIVE_PSEUDOCLASS, focused);
?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 414:
> 412:
> 413: double width = getWidth();
> 414: double button1Width = boundedWidth(button1);
Q: do we want to snap to pixel every time we use Node.[min/pref/max] values to avoid jitter and fuzzy edges?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 490:
> 488:
> 489: private static final List<CssMetaData<?, ?>> METADATA =
> 490: Stream.concat(getClassCssMetaData().stream(), Stream.of(BUTTON_ORDER_METADATA)).toList();
[unrelated] this malloc galore deserves a utility method
https://bugs.openjdk.org/browse/JDK-8320796
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkWindow.java line 278:
> 276: }
> 277:
> 278: View.EventHandler eventHandler = view.getEventHandler();
should this check be moved before L271?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinView.java line 115:
> 113:
> 114: EventHandler eventHandler = getEventHandler();
> 115: if (eventHandler != null && eventHandler.pickDragAreaNode(wx, wy) != null) {
computing `wx, wy` may not be necessary if `eventHandler != null` check is moved before L111
modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinWindow.java line 385:
> 383: * Classifies the window region at the specified physical coordinate.
> 384: * <p>
> 385: * This method is called from native code.
is it called from the fx application thread?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinWindow.java line 390:
> 388: * @param y the Y coordinate in physical pixels
> 389: */
> 390: @SuppressWarnings("unused")
tangentially related conversation starter:
should we have a couple of annotations for code called from JNI/native and reflection?
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/ViewScene.java line 206:
> 204: @Override public String toString() {
> 205: View view = getPlatformView();
> 206: return (" scene: " + hashCode() + " @ (" + view.getWidth() + "," + view.getHeight() + ")");
leading space in `toString()` might be a _bad idea_
suggestion (JSON-like):
`return "ViewScene" + hashCode() + "{width=" + view.getWidth() + ", height=" + view.getHeight() + "}";`
modules/javafx.graphics/src/main/java/javafx/application/ConditionalFeature.java line 156:
> 154: * Indicates that a system supports {@link javafx.stage.StageStyle#EXTENDED}.
> 155: * <p>
> 156: * This feature is currently supported on Windows, Linux, and macOS.
would it make sense to add more information similarly to UNIFIED_WINDOW on L151?
maybe pointing to different levels of support?
I don't know whether it makes sense to bring the whole table from the JEP here, but maybe somewhere (HeaderBar)?
modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 53:
> 51: * account for the default window buttons (minimize, maximize, close). If a child is configured to be
> 52: * centered in the {@code center} area, it is laid out with respect to the stage, and not with respect
> 53: * to the {@code center} area. This ensures that the child will appear centered in the stage regardless
should there be more fine tuned control for growing/shrinking?
can the application just add a single container like GridPane that handles the resizing?
modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 59:
> 57: * {@code HeaderBar} honors the minimum, preferred, and maximum sizes of its children. If the child's
> 58: * resizable range prevents it from be resized to fit within its position, it will be vertically centered
> 59: * relative to the available space; this alignment can be customized with a layout constraint.
what if the components are too large to fit, will clipping occur? should this behavior be mentioned?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1605#pullrequestreview-2414057652
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828345233
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828349822
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828354349
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828510506
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828377118
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828502773
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828528087
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828531686
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828533592
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828534701
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828535399
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828536256
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828538043
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828543247
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828545452
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828548458
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828549557
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828550177
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828553092
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828555928
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828560450
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828558349
More information about the openjfx-dev
mailing list