On Thu, 13 Feb 2025 19:10:15 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Implementation of >> [`EXTENDED`](https://gist.github.com/mstr2/0befc541ee7297b6db2865cc5e4dbd09) >> and `EXTENDED_UTILITY` stage style. > > Michael Strauß has updated the pull request incrementally with two additional > commits since the last revision: > > - typo > - update copyright headers modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line 715: > 713: > 714: protected abstract boolean _supportsExtendedWindows(); > 715: public final boolean supportsExtendedWindows() { missing newline after L714? modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonMetrics.java line 39: > 37: * @param minHeight the minimum height of the window buttons > 38: */ > 39: public record HeaderButtonMetrics(Dimension2D leftInset, Dimension2D > rightInset, double minHeight) { would it make more sense to use Insets instead of Dimension2D? What if the app calls for asymmetrical padding? modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 153: > 151: > 152: private static final CssMetaData<HeaderButtonOverlay, Number> > BUTTON_DEFAULT_HEIGHT_METADATA = > 153: new CssMetaData<>("-fx-button-default-height", > StyleConverter.getSizeConverter()) { The new styleables need to be documented in cssref.html as a part of this PR, right? (The javadoc for this class will not be visible in the API specification). modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 209: > 207: > 208: private static final List<CssMetaData<?, ?>> METADATA = > 209: Stream.concat(getClassCssMetaData().stream(), `RichUtils.combine()` L419 avoids creating intermediary objects. modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 283: > 281: */ > 282: private final StyleableBooleanProperty allowRtl = > 283: new SimpleStyleableBooleanProperty(ALLOW_RTL_METADATA, this, > "allowRtl", true) { what is the rationale for this property? Instead, I think, it should look at the `Node::getEffectiveNodeOrientation()` which either inherits the value from the parent or allows the app to override for a specific component (in other words, we already have this functionality for free). Or am I missing something? modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 328: > 326: > 327: var updateStylesheetSubscription = sceneProperty() > 328: .flatMap(Scene::fillProperty) will this work if the value of scene is `null`? modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 405: > 403: } : null; > 404: > 405: if (type == MouseEvent.ENTER || type == MouseEvent.MOVE || type > == MouseEvent.DRAG) { minor: would it be clearer to use a `switch(type)` here instead? modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 406: > 404: > 405: if (type == MouseEvent.ENTER || type == MouseEvent.MOVE || type > == MouseEvent.DRAG) { > 406: handleMouseOver(node); could there be a situation when node is null but we pass it on to the method? also L410, L412 modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 586: > 584: totalHeight = snapSizeY(getEffectiveButtonHeight()); > 585: } else { > 586: totalHeight = snapSizeY(Math.max(button1Height, > Math.max(button2Height, button3Height))); +1 for correct snapping! modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 632: > 630: } > 631: > 632: private static double boundedSize(double min, double pref, double > max) { minor: it would be nice to move this to some common utilities class (there is already such a method in c.s.j.scene.control.skin.Utils class). along with two previous methods. modules/javafx.graphics/src/main/java/com/sun/glass/ui/HeaderButtonOverlay.java line 678: > 676: orderedButtons.add(this); > 677: buttonOrder.set(order); > 678: glyph.getStyleClass().setAll("glyph"); "glyph": though apt, the name might be somewhat confusing. maybe "icon"? modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 299: > 297: } > 298: > 299: /** since the majority of windows is expected to be DECORATED, would it make more sense to move these properties into a separate container akin to `Node.getMiscProperties()` ? modules/javafx.graphics/src/main/java/javafx/application/ConditionalFeature.java line 163: > 161: */ > 162: EXTENDED_WINDOW, > 163: Is this PR blocked by the Preview Feature Support #1359? modules/javafx.graphics/src/main/java/javafx/scene/layout/HeaderBar.java line 323: > 321: minHeightProperty(); > 322: > 323: ObservableValue<Stage> stage = sceneProperty() will this handle `null` scene? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956349123 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956353096 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956357334 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956364194 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956372516 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956374877 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956464424 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956462896 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956484613 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956488129 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956491008 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956495596 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956505679 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956507647
