RFR: 8313424: JavaFX controls in the title bar [v49]
Andy Goryachev
angorya at openjdk.org
Fri Feb 14 17:43:28 UTC 2025
On Thu, 13 Feb 2025 19:10:15 GMT, Michael Strauß <mstrauss at 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
More information about the openjfx-dev
mailing list