RFR: 8313424: JavaFX controls in the title bar [v19]

[Michael Strauß]

On Mon, 4 Nov 2024 23:21:22 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits:
>> 
>>  - Merge branch 'master' into feature/extended-window
>>  - Merge branch 'master' into feature/extended-window
>>  - macOS: dynamically adapt toolbar style to headerbar height
>>  - fix header bar height flicker
>>  - NPE
>>  - fix peer access outside of synchronizer
>>  - improve title text documentation
>>  - macOS: hide window title
>>  - better documentation
>>  - set minHeight to native height of title bar
>>  - ... and 13 more: https://git.openjdk.org/jfx/compare/58cd76a8...9b63892d
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 199:
> 
>> 197:      */
>> 198:     private final StyleableBooleanProperty allowRtl =
>> 199:         new SimpleStyleableBooleanProperty(ALLOW_RTL_METADATA, this, "allowRtl", true) {
> 
> why is `allowRtl` a property and not a simple flag?
> why is it allowed to be changed via CSS via `-fx-allow-rtl`?

The purpose is to allow JavaFX developers (that is us, not users of JavaFX) to define the entire appearance of `WindowControlsOverlay` with a CSS file. This should make it easier to maintain it, since we won't have to keep looking in two places to change the appearance.

> modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 273:
> 
>> 271:     public ButtonType buttonAt(double x, double y) {
>> 272:         for (var button : orderedButtons) {
>> 273:             if (button.isVisible() && button.getBoundsInParent().contains(x, y)) {
> 
> are we using the right coordinates?  x,y seems to be in the "window" coordinates, while button.parent is... not.  Shouldn't you do a proper conversion here?
> 
> (also notice my earlier comment about the Close button in RTL mode)

`WindowControlsOverlay` is not part of the JavaFX scene graph. It is a resizeable _overlay_ control that always stretches the entire window, and with that in mind, `button.getBoundsInParent()` will translate the button bounds to window coordinates.

> modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 286:
> 
>> 284:      * @param type the event type
>> 285:      * @param button the button type
>> 286:      * @param x the X coordinate, in pixels relative to the window
> 
> this is an internal class, but is it correct to refer to the "window coordinates"?  what are these?  platform window coordinates?

JavaFX window coordinates, i.e. scaled coordinates. For coordinates coming from the native toolkit, I have used the term "physical pixels". Physical pixels are always converted at the Java-native boundary, and are not used in JavaFX internals.

> modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java line 354:
> 
>> 352:                 case MINIMIZE -> stage.setIconified(true);
>> 353:                 case MAXIMIZE -> stage.setMaximized(!stage.isMaximized());
>> 354:                 case CLOSE -> stage.close();
> 
> should this `switch` have a default case?

No, because we never have more than three window buttons. If we ever had, we would want to change the code instead of running into a default clause.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828597391
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828600111
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828601573
PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1828602372