On Mon, 4 Nov 2024 23:21:22 GMT, Andy Goryachev <ango...@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
