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

[Michael Strauß]

On Mon, 4 Nov 2024 20:33:42 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
>
> 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