RFR: 8260528: Clean glass-gtk sizing and positioning code [v27]

[Thiago Milczarek Sayao]

On Thu, 17 Nov 2022 15:42:29 GMT, Johan Vos <jvos at openjdk.org> wrote:

>> Thiago Milczarek Sayao has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>> 
>>   8260528: Clean glass-gtk sizing and positioning code
>
> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 713:
> 
>> 711: 
>> 712: static int geometry_get_window_width(const WindowGeometry *windowGeometry) {
>> 713:      return (windowGeometry->final_width.type != BOUNDSTYPE_WINDOW)
> 
> minor: wouldn't it be more readable (here and below) if the test was reversed? ( `(windowGeometry->final_width.type != BOUNDSTYPE_WINDOW`)

Those functions already existed. Do you prefer to keep as it was or to invert the `if`?

> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 891:
> 
>> 889: 
>> 890:     if (get_frame_extents_property(&top, &left, &bottom, &right)) {
>> 891:         if ((top + right + bottom + left) != 0) {
> 
> Are we 100% sure the extents can never be negative values?

I'm 99.99% sure :)

> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 1047:
> 
>> 1045: 
>> 1046:     if (moved) {
>> 1047:         geometry_set_window_x(&geometry, x);
> 
> this will set `windowGeometry->refx` where 2 lines below you set `windowGeometry.current_x`. Unless there is gravity involved, both refer to the same value, one being `int` and the other being `float` though. Is there another conceptual difference, or why do we have 2 values here?

I think you're right, I added `current_x/y` to check if a window moved, but `ref_x/ref_y` might contain the same value (different type).

> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 1269:
> 
>> 1267: }
>> 1268: 
>> 1269: void WindowContextTop::to_front() {
> 
> Do you use `raise` and `lower` here because the sibiling parameter is always `NULL` ?

Yes. But also `gdk_window_raise` and `gdk_window_lower` seems to better match the `toFront()` and `toBack()` description.

See:
https://tronche.com/gui/x/xlib/window/XRestackWindows.html
https://tronche.com/gui/x/xlib/window/XRaiseWindow.html

gdk functions calls those Xlib functions internally.

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

PR: https://git.openjdk.org/jfx/pull/915