On Fri, 14 Feb 2025 15:43:38 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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?
Yeah, doesn't look nice, but I wanted to match the style of the surrounding
methods.
> 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?
This class is not API, so it doesn't matter here. However, it matters in
`HeaderBar.leftSystemInset/rightSystemInset`, which are both of type
`Dimension2D`. The system insets are not arbitrary rectangles, they are always
aligned with the top-left and top-right edges of the window. As such, the only
necessary information is their spatial extent, which is best represented as a
`Dimension2D`.
I'm not sure what you mean by asymmetrical padding. Can you elaborate?
> 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).
`HeaderButtonOverlay` is not API, it's an internal implementation detail. The
javadoc is just informational for future JavaFX developers.
> 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.
Needs to wait until we have something like this in an accessible place...
> 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?
`HeaderButtonOverlay` inherits its node orientation from the scene, which is
under the control of the application developer. On the other hand, `allowRtl`
is under the control of JavaFX developers (that's us), specifying whether the
selected header button implementation supports RTL layouts at all. Currently,
all header button implementations do.
The property is there to make all aspects of `HeaderButtonOverlay` styleable,
so that future JavaFX developers can add other header button styles that may
have fixed locations (i.e. don't move from one side to the other in RTL mode).
> 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`?
Yes!
> 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?
I've tried, but the additional conditions make it end up more verbose and
funny-looking.
> 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
Yes, that's a very common situation. It happens when the cursor is on the
header bar, but not on a header button.
> 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.
We can also consider exposing this as API (not with this PR, though). If it's
useful for us, it might also be useful for application developers.
> 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()` ?
Sure, but the downside is that everything gets more bloated for little benefit.
Right now, these properties are accessed in derived classes without any checks,
since they are final and non-null. In this case, I think ease of use is more
important.
> 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?
Yes, it is. Once we get preview features in, I'll annotate the new API elements.
> 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?
Yes, it will.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968295
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968317
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968351
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968405
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968436
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968452
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968485
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968472
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968540
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968558
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968567
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1956968572
