On Tue, 5 Nov 2024 01:32:16 GMT, Michael Strauß <mstra...@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 incrementally with one additional > commit since the last revision: > > stylistic changes second batch of comments modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 118: > 116: > 117: private static final String ALIGNMENT = "headerbar-alignment"; > 118: private static final String MARGIN = "headerbar-margin"; Q: should these be a String or an Object? String.equals() are more expensive. (I guess we use Strings elsewhere, so probably ok). modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 170: > 168: // Inflate the minHeight property. This is important so that we > can track whether a stylesheet or > 169: // user code changes the property value before we set it to the > height of the native title bar. > 170: minHeightProperty(); if that's the case, why not initialize it in L1150 ? modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 262: > 260: @Override > 261: protected double computeMinWidth(double height) { > 262: Node leading = getLeading(), center = getCenter(), trailing = > getTrailing(); I think it's a bad practice to have multiple statements on one line. modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBarBase.java line 51: > 49: * can specify draggable content nodes of the {@code HeaderBarBase} with > the {@link #setDraggable} method. > 50: * > 51: * @apiNote Most application developers should use the {@link HeaderBar} > implementation instead of just curious: what's the reason to split the HB into the two classes? Can you give an example? modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBarBase.java line 138: > 136: private void updateInsets() { > 137: if (currentFullScreen || currentMetrics == null) { > 138: leftSystemInset.set(new Dimension2D(0, 0)); Q: would it make sense to declare a `static final Dimension2D EMPTY = new Dimension2D(0, 0)` and use it everywhere? modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBarBase.java line 190: > 188: * independent of layout orientation. > 189: */ > 190: private final ReadOnlyObjectWrapper<Dimension2D> rightSystemInset = Q: would it make sense to create a class with requestLayout() ? this will save two class objects... modules/javafx.graphics/src/main/java/javafx/stage/StageStyle.java line 83: > 81: * <p> > 82: * An extended window has the default window buttons (minimize, > maximize, close), but no system-provided > 83: * draggable header bar. Applications need to provide their own > header bar by placing a single {@link HeaderBar} I would like to see this expanded just a little bit, to give an example of what is (system menu, buttons) and is not rendered (rounded corners, app title), similarly to the the table in the JEP. I would prefer a table or an `<ul>`, and I specifically want to include the window title. Window title is a property, and I think it deserves a mention (will it be shown in the tray? in the "running applications" os widget?) I don't know whether the best place to include that is here or in the `HeaderBar`. modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1336: > 1334: } > 1335: > 1336: - (NSEvent*)lastEvent could there be side effects with exposing this field? concurrency issues? modules/javafx.graphics/src/main/resources/com/sun/glass/ui/gtk/WindowDecorationGnome.css line 26: > 24: */ > 25: > 26: .window-button-container { is it possible for the app dev to alter the styling with an external style sheet? modules/javafx.graphics/src/main/resources/com/sun/glass/ui/gtk/WindowDecorationGnome.css line 125: > 123: > 124: .close-button > .glyph { > 125: -fx-shape: "m 8.1464844,8.1464844 a 0.5,0.5 0 0 0 0,0.7070312 L > 11.292969,12 8.1464844,15.146484 a 0.5,0.5 0 0 0 0,0.707032 0.5,0.5 0 0 0 > 0.7070312,0 L 12,12.707031 l 3.146484,3.146485 a 0.5,0.5 0 0 0 0.707032,0 > 0.5,0.5 0 0 0 0,-0.707032 L 12.707031,12 15.853516,8.8535156 a 0.5,0.5 0 0 0 > 0,-0.7070312 0.5,0.5 0 0 0 -0.707032,0 L 12,11.292969 8.8535156,8.1464844 a > 0.5,0.5 0 0 0 -0.7070312,0 z"; minor: we could probably minimize the path by rounding to 2 or 3 decimal points (same comment for all .css changes) modules/javafx.graphics/src/main/resources/com/sun/glass/ui/win/WindowDecoration.css line 107: > 105: > 106: .maximize-button.restore > .glyph { > 107: -fx-shape: "m 7.6491699,2.5192871 q 0,-0.3444824 > -0.1369629,-0.6495361 Q 7.3752441,1.5646973 7.1407471,1.338501 > 6.90625,1.1123047 6.5970459,0.98156738 6.2878418,0.85083008 > 5.9475098,0.85083008 H 1.7722168 Q 1.838623,0.65991211 1.9589844,0.50219727 > 2.0793457,0.34448242 2.2370605,0.23242188 2.3947754,0.12036133 > 2.5836182,0.06018066 2.7724609,0 2.9758301,0 H 5.9475098 Q 6.4746094,0 > 6.9394531,0.20129395 7.4042969,0.40258789 7.7508545,0.74707031 > 8.0974121,1.0915527 8.2987061,1.5563965 8.5,2.0212402 8.5,2.5483398 V > 5.5241699 Q 8.5,5.7275391 8.4398193,5.9163818 8.3796387,6.1052246 > 8.2675781,6.2629395 8.1555176,6.4206543 7.9978027,6.5410156 > 7.8400879,6.661377 7.6491699,6.7277832 Z M 1.253418,8.5 Q 1.0043945,8.5 > 0.77612305,8.3983154 0.54785156,8.2966309 0.37561035,8.1243896 > 0.20336914,7.9521484 0.10168457,7.723877 0,7.4956055 0,7.246582 V 2.9550781 Q > 0,2.7019043 0.10168457,2.475708 0.20336914,2.2495117 0.37561035,2.0772705 > 0.54785156,1.9050293 0.77404785,1.8033447 1.0002441,1.70166 02 1.253418,1.7016602 h 4.2915039 q 0.2531738,0 0.4814453,0.1016845 0.2282715,0.1016846 0.3984375,0.2718506 0.170166,0.170166 0.2718506,0.3984375 0.1016845,0.2282715 0.1016845,0.4814453 V 7.246582 q 0,0.2531739 -0.1016845,0.4793701 Q 6.5949707,7.9521484 6.4227295,8.1243896 6.2504883,8.2966309 6.024292,8.3983154 5.7980957,8.5 5.5449219,8.5 Z M 5.5241699,7.6491699 q 0.087158,0 0.1639405,-0.033203 0.076782,-0.033203 0.1369628,-0.091309 0.060181,-0.058105 0.093384,-0.1348877 0.033203,-0.076782 0.033203,-0.1639404 v -4.25 q 0,-0.087158 -0.033203,-0.1660156 Q 5.8852539,2.730957 5.8271484,2.6728516 5.769043,2.6147461 5.6901855,2.581543 5.6113281,2.5483398 5.5241699,2.5483398 h -4.25 q -0.087158,0 -0.1639404,0.033203 -0.076782,0.033203 -0.1348877,0.093384 -0.0581055,0.060181 -0.0913086,0.1369628 -0.0332031,0.076782 -0.0332031,0.1639405 v 4.25 q 0,0.087158 0.0332031,0.1639404 0.0332031,0.076782 0.0913086,0.1348877 0.0581055,0.058105 0.1348877,0.091309 0.076782,0.033203 0.1639404,0.033203 z"; ... especially here modules/javafx.graphics/src/test/java/test/javafx/scene/layout/HeaderBarTest.java line 70: > 68: "BOTTOM_CENTER, 10, 10, 100, 80", > 69: "BOTTOM_RIGHT, 10, 10, 100, 80" > 70: }) these tests can be simplified to avoid parsing with @CsvSource and default conversion, example: @ParameterizedTest @CsvSource(value = {"yo,true,1", "bah,true,1.1", "meh,false,2.2"}) void test(String candidate, boolean expected, double a) { System.out.println("candidate=" + candidate + " expected=" + expected + " a=" +a ); } ------------- PR Review: https://git.openjdk.org/jfx/pull/1605#pullrequestreview-2416113079 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829669611 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829671987 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829674239 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829687841 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829847752 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829849333 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829857742 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829868608 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829874022 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829875372 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829876147 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1829926610
