On Sat, 15 Feb 2025 01:14:12 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> 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?
The word "inset" confused me. Could you use some other word here, if it is
referring to a dimension? Like maybe explain where exactly these are used?
>> 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.
Let me clarify:
I think the css properties must be listed in `cssref.html` as the component can
be styled via CSS. I don't see the `cssref.html` file modified in this PR.
>> 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).
should it have a different name then? `fixedPosition` or something like that?
>> 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.
my earlier PR was shut down under the rubric "we could do better" but "we"
never did.
>> 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.
ease of use for whom?
bloated code: a few bytes in extra methods
bloated data: thousand of bytes added per instance
Since we are adding properties to Window, it's probably ok because the number
of instances is relatively low (a real app won't have hundreds of windows).
Still, I would like to recommend investigating the alternative: the memory is
not an unlimited resource, and in my experience, it's worth thinking about it.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1960289720
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1960283225
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1960278499
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1960276809
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1960274585
