On Mon, 4 Nov 2024 20:33:42 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 > > buildSrc/win.gradle line 329: > >> 327: "winmm.lib", "imm32.lib", "shell32.lib", >> "Uiautomationcore.lib", "dwmapi.lib", "uxtheme.lib", >> 328: "/DELAYLOAD:user32.dll", "/DELAYLOAD:urlmon.dll", >> "/DELAYLOAD:winmm.dll", "/DELAYLOAD:shell32.dll", >> 329: "/DELAYLOAD:Uiautomationcore.dll", "/DELAYLOAD:dwmapi.dll", >> "/DELAYLOAD:uxtheme.dll"]).flatten() > > minor suggestion: placing each entry on separate line might simplify > maintenance and reduce merge conflicts. I tried that, but this will add another 14 lines to the script. I'm inclined to not change it at this point. > modules/javafx.graphics/src/main/java/com/sun/glass/ui/WindowControlsOverlay.java > line 490: > >> 488: >> 489: private static final List<CssMetaData<?, ?>> METADATA = >> 490: Stream.concat(getClassCssMetaData().stream(), >> Stream.of(BUTTON_ORDER_METADATA)).toList(); > > [unrelated] this malloc galore deserves a utility method > > https://bugs.openjdk.org/browse/JDK-8320796 I'd be in favor of getting rid of this "static inheritance" anti-pattern entirely. We should resurrect that discussion at some point. > modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkWindow.java > line 278: > >> 276: } >> 277: >> 278: View.EventHandler eventHandler = view.getEventHandler(); > > should this check be moved before L271? Could be, but it really doesn't matter since `eventHandler` is basically never `null`. > modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinView.java line > 115: > >> 113: >> 114: EventHandler eventHandler = getEventHandler(); >> 115: if (eventHandler != null && >> eventHandler.pickDragAreaNode(wx, wy) != null) { > > computing `wx, wy` may not be necessary if `eventHandler != null` check is > moved before L111 True, but this is a menu event handler, i.e. it is only invoked if you right-click on a window title bar. There's no need to elide a double division here, especially given that `eventHandler` is basically never `null`. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1831992702 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1831994096 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1831994855 PR Review Comment: https://git.openjdk.org/jfx/pull/1605#discussion_r1831996006
