Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popup menus (including context menus) pop up briefly and disappear if another menu is already open #50367

Closed
Houkime opened this issue Jul 11, 2021 · 34 comments · Fixed by #50712

Comments

@Houkime
Copy link

Houkime commented Jul 11, 2021

Godot version

v4.0.dev.custom_build [a0d800e]

System information

Arch Linux (kernel 5.12.14-arch1-1, X11)

Issue description

The initial menu gets closed, but the new menu gets closed too, after a brief flicker of existence.

(flicker always happens, it is not always captured because of limited gif fps, but i do it multiple times in gifs to be sure.)
menus_bug
context_menus_bug

Expected behaviour

  • New menu pops up
  • Old menu gets closed
  • No flickering

Steps to reproduce

  • Create a new empty project and open it for edit.
  • Open any drop down menu (LMB at the top bar items or RMB for context menus).
  • Without closing it, try to open another menu.
  • After a brief display of the new menu, they both get closed.

Minimal reproduction project

Not applicable. (a new empty project is just as good as anything else)

@Houkime Houkime changed the title Drop down menus (including context menus) pop up briefly and disappear if another menu was already open Drop down menus (including context menus) pop up briefly and disappear if another menu is already open Jul 11, 2021
@Houkime Houkime changed the title Drop down menus (including context menus) pop up briefly and disappear if another menu is already open Popup menus (including context menus) pop up briefly and disappear if another menu is already open Jul 11, 2021
@Calinou Calinou added this to the 4.0 milestone Jul 11, 2021
@Calinou
Copy link
Member

Calinou commented Jul 11, 2021

See also #44997.

Can you reproduce this after enabling Single Window Mode in the Editor Settings?

@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

With single window mode, top bar menus behave normally (new menu opens, old closes), but context menus can no longer be opened if another popup menu (top bar OR context) is already open (the old one is not getting closed).

@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

This is how it looks chronologically without single window mode (I click on one top bar item, then click on another)

menu_button clicked 
 visibility set to true
[New Thread 0x7fffacff9640 (LWP 10023)]
menu_button clicked 
 visibility set to true
[New Thread 0x7fff9ffff640 (LWP 10024)]
 visibility set to false
[Thread 0x7fffacff9640 (LWP 10023) exited]
 visibility set to false
[Thread 0x7fff9ffff640 (LWP 10024) exited]

@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

I am not sure however what exactly closes them (sets visibility to false).
These hiding calls both originate from the main thread and gdb backtrace from one such event reads like below.
It seems like these calls were both injected into the message queue, but idk by what and why.

#0  Window::set_visible (this=0xe2a52b0, p_visible=<optimized out>) at scene/main/window.cpp:370
#1  0x00000000008c367a in call_with_variant_args_helper<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_args=<synthetic pointer>, r_error=..., p_method=<optimized out>, 
    p_instance=<optimized out>) at ./core/variant/binder_common.h:218
#2  call_with_variant_args_dv<__UnexistingClass> (default_values=..., r_error=..., p_argcount=<optimized out>, 
    p_args=<optimized out>, p_method=<optimized out>, p_instance=<optimized out>) at ./core/variant/binder_common.h:365
#3  MethodBindT<>::call(Object*, Variant const**, int, Callable::CallError&) (this=<optimized out>, p_object=<optimized out>, 
    p_args=<optimized out>, p_arg_count=<optimized out>, r_error=...) at ./core/object/method_bind.h:285
#4  0x0000000003af0b57 in Object::call (this=0xe2a52b0, p_method=..., p_args=0x0, p_argcount=0, r_error=...)
    at core/object/object.cpp:832
#5  0x00000000038eb2a2 in Callable::call (this=0x7ffff6647020, p_arguments=0x0, p_argcount=0, r_return_value=..., 
    r_call_error=...) at core/variant/callable.cpp:62
#6  0x0000000003aec462 in MessageQueue::_call_function (this=<optimized out>, p_callable=..., p_args=<optimized out>, 
    p_argcount=0, p_show_error=<optimized out>) at core/object/message_queue.cpp:258
#7  0x0000000003aec80c in MessageQueue::flush (this=0x6faeb30) at core/object/message_queue.cpp:304
#8  0x000000000238e393 in SceneTree::process (this=0x8bfc130, p_time=0.0125292214) at scene/main/scene_tree.cpp:441
#9  0x0000000000861c5e in Main::iteration () at main/main.cpp:2533
#10 0x000000000083a5b1 in OS_LinuxBSD::run (this=this@entry=0x7fffffffe130) at platform/linuxbsd/os_linuxbsd.cpp:278
#11 0x000000000082e802 in main (argc=3, argv=0x7fffffffe638) at platform/linuxbsd/godot_linuxbsd.cpp:58

@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

The Message does not have a sender or other origin info, so it is kind of an anonymous trolling at this point.

struct Message {
                Callable callable;
                int16_t type;
                union {
                        int16_t notification;
                        int16_t args;
                };
        };

@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

ok, i have dumped the method names of the calls being pushed into the message queue while this thing happens

menu_button clicked (the second click)

_update_callback
...
_update_callback
 visibility set to true
_update_child_controls
_update_minimum_size
_update_minimum_size
_update_callback
group flag call _viewports_process_picking
_update_minimum_size
_update_callback
...
_update_callback
_update_minimum_size
_update_callback
...
_update_callback
hide
_update_callback
group flag call _viewports_process_picking
 visibility set to false
_update_callback
...
_update_callback
hide
_update_callback
group flag call _viewports_process_picking
 visibility set to false
_update_callback

Now we know the exact method name that gets pushed, and this name is hide.
This gives a hope to catch an actual PUSH with a debugger to know what part of the code does this.

@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

and here is our trace

Thread 1 "godot.linuxbsd." hit Breakpoint 1, MessageQueue::push_call (this=0x6faeb30, p_id=..., p_method=..., 
    p_args=0x7fffffffda80, p_argcount=0, p_show_error=<optimized out>) at core/object/message_queue.cpp:48
48	        print_line("hide was pushed");
(gdb) backtrace
#0  MessageQueue::push_call (this=0x6faeb30, p_id=..., p_method=..., p_args=0x7fffffffda80, p_argcount=0, 
    p_show_error=<optimized out>) at core/object/message_queue.cpp:48
#1  0x0000000003aeed2b in MessageQueue::push_call (p_arg5=..., p_arg4=..., p_arg3=..., p_arg2=..., p_arg1=..., p_method=..., 
    p_id=..., this=<optimized out>) at core/object/message_queue.cpp:66
#2  MessageQueue::push_call (this=<optimized out>, p_object=p_object@entry=0xe2a5820, p_method=..., p_arg1=..., p_arg2=..., 
    p_arg3=..., p_arg4=..., p_arg5=...) at core/object/message_queue.cpp:124
#3  0x0000000003af2862 in Object::call_deferred (this=this@entry=0xe2a5820, p_method=..., p_arg1=..., p_arg2=..., p_arg3=..., 
    p_arg4=..., p_arg5=...) at core/object/object.cpp:1650
#4  0x000000000253586f in Popup::_close_pressed (this=0xe2a5820) at scene/gui/popup.cpp:106
#5  0x0000000003afef6a in Object::emit_signal (this=0x8c052d0, p_name=..., p_args=<optimized out>, p_argcount=<optimized out>)
    at core/object/object.cpp:1099
#6  0x0000000003affd90 in Object::emit_signal (this=this@entry=0x8c052d0, p_name=..., p_arg1=..., p_arg2=..., p_arg3=..., 
    p_arg4=..., p_arg5=...) at core/object/object.cpp:1154
#7  0x00000000023e1341 in Window::_event_callback (this=0x8c052d0, p_event=<optimized out>) at scene/main/window.cpp:328
#8  0x0000000000844c7b in DisplayServerX11::_send_window_event (this=<optimized out>, wd=..., p_event=4294958880)
    at platform/linuxbsd/display_server_x11.cpp:2681
#9  0x0000000000850395 in DisplayServerX11::process_events (this=0x70795d0) at platform/linuxbsd/display_server_x11.cpp:3048
#10 0x000000000083a5d0 in OS_LinuxBSD::run (this=this@entry=0x7fffffffe130) at platform/linuxbsd/os_linuxbsd.cpp:274
#11 0x000000000082e832 in main (argc=3, argv=0x7fffffffe638) at platform/linuxbsd/godot_linuxbsd.cpp:58

and for the second close (seems the same?)

Thread 1 "godot.linuxbsd." hit Breakpoint 1, MessageQueue::push_call (this=0x6faeb30, p_id=..., p_method=..., 
    p_args=0x7fffffffda80, p_argcount=0, p_show_error=<optimized out>) at core/object/message_queue.cpp:48
48	        print_line("hide was pushed");
(gdb) backtrace
#0  MessageQueue::push_call (this=0x6faeb30, p_id=..., p_method=..., p_args=0x7fffffffda80, p_argcount=0, 
    p_show_error=<optimized out>) at core/object/message_queue.cpp:48
#1  0x0000000003aeed2b in MessageQueue::push_call (p_arg5=..., p_arg4=..., p_arg3=..., p_arg2=..., p_arg1=..., p_method=..., 
    p_id=..., this=<optimized out>) at core/object/message_queue.cpp:66
#2  MessageQueue::push_call (this=<optimized out>, p_object=p_object@entry=0xe5c0df0, p_method=..., p_arg1=..., p_arg2=..., 
    p_arg3=..., p_arg4=..., p_arg5=...) at core/object/message_queue.cpp:124
#3  0x0000000003af2862 in Object::call_deferred (this=this@entry=0xe5c0df0, p_method=..., p_arg1=..., p_arg2=..., p_arg3=..., 
    p_arg4=..., p_arg5=...) at core/object/object.cpp:1650
#4  0x000000000253586f in Popup::_close_pressed (this=0xe5c0df0) at scene/gui/popup.cpp:106
#5  0x0000000003afef6a in Object::emit_signal (this=0x8c052d0, p_name=..., p_args=<optimized out>, p_argcount=<optimized out>)
    at core/object/object.cpp:1099
#6  0x0000000003affd90 in Object::emit_signal (this=this@entry=0x8c052d0, p_name=..., p_arg1=..., p_arg2=..., p_arg3=..., 
    p_arg4=..., p_arg5=...) at core/object/object.cpp:1154
#7  0x00000000023e1341 in Window::_event_callback (this=0x8c052d0, p_event=<optimized out>) at scene/main/window.cpp:328
#8  0x0000000000844c7b in DisplayServerX11::_send_window_event (this=<optimized out>, wd=..., p_event=4294958880)
    at platform/linuxbsd/display_server_x11.cpp:2681
#9  0x0000000000850395 in DisplayServerX11::process_events (this=0x70795d0) at platform/linuxbsd/display_server_x11.cpp:3048
#10 0x000000000083a5d0 in OS_LinuxBSD::run (this=this@entry=0x7fffffffe130) at platform/linuxbsd/os_linuxbsd.cpp:274
#11 0x000000000082e832 in main (argc=3, argv=0x7fffffffe638) at platform/linuxbsd/godot_linuxbsd.cpp:58

@fire fire added the confirmed label Jul 11, 2021
@Houkime
Copy link
Author

Houkime commented Jul 11, 2021

In BOTH cases DisplayServer sends WINDOW_EVENT_FOCUS_IN which translates by the Window to focus_entered which somehow triggers _close_pressed() on Popup which makes a deferred call to hide.

Also, one can notice that the window and the popup are different objects (they have different this) BUT the window is the same object for both popups.
So it is probably some kind of parent-children relation.

@Houkime
Copy link
Author

Houkime commented Jul 12, 2021

ok, noticed optimizing gets in the way too much, so here is a stack with an unoptimized build.
Now it is a bit clearer and indeed there is a problem that there is a parent window getting focused and (both of, being the children of the same parent) popup gets closed.

Thread 1 "godot.linuxbsd." hit Breakpoint 1, MessageQueue::push_call (this=0x98c1f50, p_id=..., p_method=..., 
    p_args=0x7fffffffd440, p_argcount=0, p_show_error=false) at core/object/message_queue.cpp:48
48	        print_line("hide was pushed");
(gdb) backtrace
#0  MessageQueue::push_call (this=0x98c1f50, p_id=..., p_method=..., p_args=0x7fffffffd440, p_argcount=0, p_show_error=false)
    at core/object/message_queue.cpp:48
#1  0x0000000005d296f3 in MessageQueue::push_call (this=0x98c1f50, p_id=..., p_method=..., p_arg1=..., p_arg2=..., 
    p_arg3=..., p_arg4=..., p_arg5=...) at core/object/message_queue.cpp:66
#2  0x0000000005d2a45b in MessageQueue::push_call (this=0x98c1f50, p_object=0x10bb81e0, p_method=..., p_arg1=..., p_arg2=..., 
    p_arg3=..., p_arg4=..., p_arg5=...) at core/object/message_queue.cpp:124
#3  0x0000000005d3bbea in Object::call_deferred (this=0x10bb81e0, p_method=..., p_arg1=..., p_arg2=..., p_arg3=..., 
    p_arg4=..., p_arg5=...) at core/object/object.cpp:1650
#4  0x0000000004622d18 in Popup::_close_pressed (this=0x10bb81e0) at scene/gui/popup.cpp:106
#5  0x0000000004622c3c in Popup::_parent_focused (this=0x10bb81e0) at scene/gui/popup.cpp:97
#6  0x0000000004625aba in call_with_variant_args_helper<Popup>(Popup*, void (Popup::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_instance=0x10bb81e0, p_method=(void (Popup::*)(Popup * const)) 0x4622c06 <Popup::_parent_focused()>, 
    p_args=0x7fffffffd9f0, r_error=...) at ./core/variant/binder_common.h:218
#7  0x0000000004625942 in call_with_variant_args<Popup> (p_instance=0x10bb81e0, 
    p_method=(void (Popup::*)(Popup * const)) 0x4622c06 <Popup::_parent_focused()>, p_args=0x7fffffffd9f0, p_argcount=0, 
    r_error=...) at ./core/variant/binder_common.h:332
#8  0x00000000046257b9 in CallableCustomMethodPointer<Popup>::call (this=0x1d3196b0, p_arguments=0x7fffffffd9f0, 
    p_argcount=0, r_return_value=..., r_call_error=...) at ./core/object/callable_method_pointer.h:96
#9  0x0000000005a999af in Callable::call (this=0xbc9c160, p_arguments=0x7fffffffd9f0, p_argcount=0, r_return_value=..., 
    r_call_error=...) at core/variant/callable.cpp:50
#10 0x0000000005d34706 in Object::emit_signal (this=0xb57d150, p_name=..., p_args=0x7fffffffd9f0, p_argcount=0)
    at core/object/object.cpp:1099
#11 0x0000000005d34e18 in Object::emit_signal (this=0xb57d150, p_name=..., p_arg1=..., p_arg2=..., p_arg3=..., p_arg4=..., 
    p_arg5=...) at core/object/object.cpp:1154
#12 0x0000000004498962 in Window::_event_callback (this=0xb57d150, p_event=DisplayServer::WINDOW_EVENT_FOCUS_IN)
    at scene/main/window.cpp:328
#13 0x00000000044b739b in call_with_variant_args_helper<Window, DisplayServer::WindowEvent, 0ul> (p_instance=0xb57d150, 
    p_method=
    (void (Window::*)(Window * const, DisplayServer::WindowEvent)) 0x4498606 <Window::_event_callback(DisplayServer::WindowEvent)>, p_args=0x7fffffffdd98, r_error=...) at ./core/variant/binder_common.h:218
#14 0x00000000044b4633 in call_with_variant_args<Window, DisplayServer::WindowEvent> (p_instance=0xb57d150, p_method=
    (void (Window::*)(Window * const, DisplayServer::WindowEvent)) 0x4498606 <Window::_event_callback(DisplayServer::WindowEvent)>, p_args=0x7fffffffdd98, p_argcount=1, r_error=...) at ./core/variant/binder_common.h:332
#15 0x00000000044b1063 in CallableCustomMethodPointer<Window, DisplayServer::WindowEvent>::call (this=0x1cff91c0, 
    p_arguments=0x7fffffffdd98, p_argcount=1, r_return_value=..., r_call_error=...)
    at ./core/object/callable_method_pointer.h:96
#16 0x0000000005a999af in Callable::call (this=0x9bcc5f8, p_arguments=0x7fffffffdd98, p_argcount=1, r_return_value=..., 
    r_call_error=...) at core/variant/callable.cpp:50
#17 0x000000000208d464 in DisplayServerX11::_send_window_event (this=0x998cca0, wd=..., 
    p_event=DisplayServer::WINDOW_EVENT_FOCUS_IN) at platform/linuxbsd/display_server_x11.cpp:2681
#18 0x000000000208eb9b in DisplayServerX11::process_events (this=0x998cca0) at platform/linuxbsd/display_server_x11.cpp:3048
#19 0x0000000002078870 in OS_LinuxBSD::run (this=0x7fffffffe140) at platform/linuxbsd/os_linuxbsd.cpp:274
#20 0x00000000020757a4 in main (argc=3, argv=0x7fffffffe638) at platform/linuxbsd/godot_linuxbsd.cpp:58

@Houkime
Copy link
Author

Houkime commented Jul 12, 2021

During gdb-mediated interrogation of the Window which gets focused (and hereby closes all popups which are set to close on parent focus), it transpires that this window has an empty title. Size and position-wise it spans the whole editor, so probably it is a root window of sorts.
It is not embedded, not exclusive, not transient, its' window id is 0 and it's parent rect is the whole screen. Ok, this should be a root window.
or rather, it is called Main window, and 0 is DisplayServer::MAIN_WINDOW_ID

@Houkime
Copy link
Author

Houkime commented Jul 12, 2021

This is how topbar works in 3.3.2
332_topbar

  • when you click one time, popup opens.
  • then if you with one popup open hover to another one - it opens, and the old one closes
  • on click, pop up closes

This is completely different and there probably was an overhaul a some point.
There is also no Single Window Mode option.
However, as mentioned, Single Window mode behavior in 4.0 and normal behavior on 3.3.2 are different.

@Calinou
Copy link
Member

Calinou commented Jul 12, 2021

This is completely different and there probably was an overhaul a some point.

Indeed. Unlike 3.x, the master branch has the DisplayServer refactor which allows multiple windows to be spawned by a single Godot process: https://godotengine.org/article/core-refactoring-progress-report-2

@Houkime
Copy link
Author

Houkime commented Jul 12, 2021

trying to bisect (master)

so far:
62efa30 bad
e3dd38c still bad
0819657 bad
79bbd8e bad
90601bb bad
f57ade8 bad
1956c7a bad
2a8531c bad(ironically, since it is named popup fixes)
bb30675 bad
5315bff bad (final result)
6d1ef8e good (but it is the thing that introduces the bug-critical logic)
7cc1e20 good
e968109 good
c75f4c0 good
efcc508 good (this demonstrates some other behavior, but arguably works)
4396e98 good (one of the commits that was in the refactoring thing, behaves like 3.3.2)

@Houkime
Copy link
Author

Houkime commented Jul 12, 2021

Ok, so the picture is now almost clear.

  • 6d1ef8e introduces closing popups on focus of non-direct parents (including main window)
  • 5315bff somehow leads to main window getting focus when we click with one popup already open

As a consequence, we click, main window gets focus (that it has previously lost?), and both popups get hidden because they are indirect children.
@pouleyKetchoupp?

@pouleyKetchoupp
Copy link
Contributor

Thanks for investigating!

It looks like there's something wrong with the other of events somehow. Even if the click gives focus to the main window and closes the previous popup, the new popup should still be created afterwards and not end up being closed.

6d1ef8e was needed to fix cases of popups that stayed open when clicking somewhere else in the editor (like property edit fields).

I think I've seen this issue or something similar when working on 5315bff and thought I fixed it. Which WM are you using? I had tested on Gnome, KDE and XFCE back then (on Ubuntu).

I'll try to check when I find some time, but if you'd like to investigate further, you can uncomment this preprocessor in the X11 display server to get more info about incoming X11 events and created/destroyed windows:

//#define DISPLAY_SERVER_X11_DEBUG_LOGS_ENABLED
#ifdef DISPLAY_SERVER_X11_DEBUG_LOGS_ENABLED
#define DEBUG_LOG_X11(...) printf(__VA_ARGS__)
#else
#define DEBUG_LOG_X11(...)
#endif

@Houkime
Copy link
Author

Houkime commented Jul 12, 2021

Thanks.
My usual WM is dwm.
This also happens with i3. (quick to install and test. Default shortcuts: https://i3wm.org/docs/refcard.html)
This does not seem to happen with LXQT(openbox).
It randomly happens with Wayfire (xwayland)

Random behaviour on Wayfire suggests a race condition. Which would probably depend on the speed of the window manager with dwm and i3 being the quickest around.

@Houkime
Copy link
Author

Houkime commented Jul 13, 2021

More detailed chronology

menu_button clicked (1st menu)
about_to_popup
 visibility set to true
 
(a long delay between me the human pressing buttons)

menu_button clicked (2nd menu)
about_to_popup
 visibility set to true
root window gets focus in event (this is not a delayed focus event from the first click, there was enough time between clicks!)
hide was pushed
 visibility set to false
root window gets focus in event
hide was pushed
 visibility set to false

This chronology indeed looks weird. So, first of all, popup happens earlier than focus signal gets to the root window. And secondly, the focus event on the root window happens twice.

@Houkime
Copy link
Author

Houkime commented Jul 13, 2021

For comparison, here is how this looks under LXQt

menu_button clicked
about_to_popup
 visibility set to true

(long delay between my clicks)

root window focused (It is processed BEFORE a button click! And it is single.)
hide was pushed 
menu_button clicked
about_to_popup
 visibility set to true
 visibility set to false

@Houkime
Copy link
Author

Houkime commented Jul 13, 2021

With x11 debug as proposed (patholgic example, dwm)

menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fff9ffff640 (LWP 2663)]
delete_sub_window: 8388743 (5) 
[Thread 0x7fffacff9640 (LWP 2662) exited]
[4042] MapNotify window=8388753 (6) 
[4042] VisibilityNotify window=8388753 (6), state=0 
[4042] VisibilityNotify window=8388743 (0), state=1 
[4042] Expose window=8388753 (6), count='0' 
[4042] Expose window=8388610 (0), count='1' 
[4042] Expose window=8388610 (0), count='0' 
[4043] FocusOut window=8388610 (0), mode='0' 
[4043] FocusIn window=8388753 (6), mode='0'
[4051] ButtonRelease window=8388610 (0), button_index=1 

(long pause between clicks)

[4642] ButtonPress window=8388610 (0), button_index=1 
menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fffacff9640 (LWP 2664)]
[4643] FocusOut window=8388753 (6), mode='0' 
[4643] FocusIn window=8388610 (0), mode='0' 
root window focused
hide was pushed
[4643] MapNotify window=8388763 (7) 
[4643] VisibilityNotify window=8388763 (7), state=0 
[4643] VisibilityNotify window=8388753 (6), state=1 
[4643] Expose window=8388763 (7), count='0' 
 visibility set to false
delete_sub_window: 8388753 (6) 
[Thread 0x7fff9ffff640 (LWP 2663) exited]
[4644] FocusOut window=8388610 (0), mode='0' 
[4644] FocusIn window=8388763 (7), mode='0' 
[4644] FocusOut window=8388763 (7), mode='0' 
[4644] FocusIn window=8388610 (0), mode='0' 
root window focused
hide was pushed
[4644] Expose window=8388610 (0), count='1' 
[4644] Expose window=8388610 (0), count='0' 
 visibility set to false
delete_sub_window: 8388763 (7) 
[Thread 0x7fffacff9640 (LWP 2664) exited]
[4645] VisibilityNotify window=8388610 (0), state=0 
[4645] Expose window=8388610 (0), count='0' 
[4651] ButtonRelease window=8388610 (0), button_index=1 

@Houkime
Copy link
Author

Houkime commented Jul 13, 2021

Button presses are pushed through via input event queue flushes.
The flushing of input happens AFTER the flushing of X11 events, and is called at the very end of process_events() function in x11 DisplayServer.
Apparently, such an arrangement does not guarantee that focus_in events are processed before the clicks they are associated with (this is an unoptimized build, compiler is probably innocent).

backtrace of a button click (pathologic)

Thread 1 "godot.linuxbsd." hit Breakpoint 1, MenuButton::pressed (this=0x10ed63b0) at scene/gui/menu_button.cpp:59
59	    print_line("menu_button clicked \n");
(gdb) backtrace
#0  MenuButton::pressed (this=0x10ed63b0) at scene/gui/menu_button.cpp:59
#1  0x00000000044bdb44 in BaseButton::_pressed (this=0x10ed63b0) at scene/gui/base_button.cpp:127
#2  0x00000000044be43b in BaseButton::on_action_event (this=0x10ed63b0, p_event=...) at scene/gui/base_button.cpp:159
#3  0x00000000044bd752 in BaseButton::_gui_input (this=0x10ed63b0, p_event=...) at scene/gui/base_button.cpp:67
#4  0x0000000004610ece in MenuButton::_gui_input (this=0x10ed63b0, p_event=...) at scene/gui/menu_button.cpp:74
#5  0x0000000003a1ab1a in call_with_variant_args_helper<__UnexistingClass, Ref<InputEvent>, 0ul> (p_instance=0x10ed63b0, 
    p_method=&virtual table offset 528, p_args=0x7fffffffd3e0, r_error=...) at ./core/variant/binder_common.h:218
#6  0x0000000003a19807 in call_with_variant_args_dv<__UnexistingClass, Ref<InputEvent> > (p_instance=0x10ed63b0, 
    p_method=&virtual table offset 528, p_args=0x7fffffffd4f0, p_argcount=1, r_error=..., default_values=...)
    at ./core/variant/binder_common.h:365
#7  0x0000000003a13b30 in MethodBindT<Ref<InputEvent> >::call (this=0xad04210, p_object=0x10ed63b0, p_args=0x7fffffffd4f0, 
    p_arg_count=1, r_error=...) at ./core/object/method_bind.h:285
#8  0x000000000445d613 in Viewport::_gui_call_input (this=0xb577280, p_control=0x10ed63b0, p_input=...)
    at scene/main/viewport.cpp:1656
#9  0x000000000445e60b in Viewport::_gui_input_event (this=0xb577280, p_event=...) at scene/main/viewport.cpp:1912
#10 0x000000000446530c in Viewport::input (this=0xb577280, p_event=..., p_local_coords=false) at scene/main/viewport.cpp:3062
#11 0x000000000449bf3c in Window::_window_input (this=0xb577280, p_ev=...) at scene/main/window.cpp:927
#12 0x00000000044b76bf in call_with_variant_args_helper<Window, Ref<InputEvent> const&, 0ul> (p_instance=0xb577280, p_method=
    (void (Window::*)(Window * const, const Ref<InputEvent> &)) 0x449bcbc <Window::_window_input(Ref<InputEvent> const&)>, 
    p_args=0x7fffffffdc08, r_error=...) at ./core/variant/binder_common.h:218
#13 0x00000000044b497f in call_with_variant_args<Window, Ref<InputEvent> const&> (p_instance=0xb577280, p_method=
    (void (Window::*)(Window * const, const Ref<InputEvent> &)) 0x449bcbc <Window::_window_input(Ref<InputEvent> const&)>, 
    p_args=0x7fffffffdc08, p_argcount=1, r_error=...) at ./core/variant/binder_common.h:332
#14 0x00000000044b0f31 in CallableCustomMethodPointer<Window, Ref<InputEvent> const&>::call (this=0x1cffc500, 
    p_arguments=0x7fffffffdc08, p_argcount=1, r_return_value=..., r_call_error=...)
    at ./core/object/callable_method_pointer.h:96
#15 0x0000000005a99daf in Callable::call (this=0x7fffffffdc30, p_arguments=0x7fffffffdc08, p_argcount=1, r_return_value=..., 
    r_call_error=...) at core/variant/callable.cpp:50
#16 0x000000000208d282 in DisplayServerX11::_dispatch_input_event (this=0x998cca0, p_event=...)
    at platform/linuxbsd/display_server_x11.cpp:2662
#17 0x000000000208d0bf in DisplayServerX11::_dispatch_input_events (p_event=...)
    at platform/linuxbsd/display_server_x11.cpp:2645
#18 0x0000000005a4ac8b in Input::_parse_input_event_impl (this=0x98f7270, p_event=..., p_is_emulated=false)
    at core/input/input.cpp:643
#19 0x0000000005a49b24 in Input::parse_input_event (this=0x98f7270, p_event=...) at core/input/input.cpp:465
#20 0x0000000005a4c068 in Input::flush_accumulated_events (this=0x98f7270) at core/input/input.cpp:858
#21 0x00000000020911be in DisplayServerX11::process_events (this=0x998cca0) at platform/linuxbsd/display_server_x11.cpp:3496
#22 0x0000000002078870 in OS_LinuxBSD::run (this=0x7fffffffe140) at platform/linuxbsd/os_linuxbsd.cpp:274
#23 0x00000000020757a4 in main (argc=3, argv=0x7fffffffe638) at platform/linuxbsd/godot_linuxbsd.cpp:58

@Houkime
Copy link
Author

Houkime commented Jul 13, 2021

This is what happens if one knocks out https://github.com/godotengine/godot/blob/master/platform/linuxbsd/display_server_x11.cpp#L3177
So, this line was the only reason we received a focus_in event at all.
In the previous log focus_in for 0 (main window) arrives the next frame after the click.
(both popups remain in place)

[1489] ButtonPress window=41943042 (0), button_index=1 
we now would have requested focus because of the click, but we won't
menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fffacff9640 (LWP 3592)]
[1490] MapNotify window=41943143 (1) 
[1490] VisibilityNotify window=41943143 (1), state=0 
[1490] VisibilityNotify window=41943042 (0), state=1 
[1490] Expose window=41943143 (1), count='0' 
[1491] FocusOut window=41943042 (0), mode='0' 
[1491] FocusIn window=41943143 (1), mode='0' 
[New Thread 0x7fff9ffff640 (LWP 3593)]
[Thread 0x7fff9ffff640 (LWP 3593) exited]
[1502] ButtonRelease window=41943042 (0), button_index=1 

long pause between my clicks

[1876] ButtonPress window=41943042 (0), button_index=1 
we now would have requested focus because of the click, but we won't
menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fff9ffff640 (LWP 3594)]
[1881] MapNotify window=41943153 (2) 
[1881] VisibilityNotify window=41943153 (2), state=0 
[1881] VisibilityNotify window=41943143 (1), state=1 
[1881] Expose window=41943153 (2), count='0' 
[1882] FocusOut window=41943143 (1), mode='0' 
[1882] FocusIn window=41943153 (2), mode='0' 
[1885] ButtonRelease window=41943042 (0), button_index=1 

@pouleyKetchoupp
Copy link
Contributor

Nice findings! Your interpretation of the problem makes sense.

This is what happens if one knocks out https://github.com/godotengine/godot/blob/master/platform/linuxbsd/display_server_x11.cpp#L3177
So, this line was the only reason we received a focus_in event at all.

If I remember correctly this was still important for focus to work correctly. When the line is disabled, does clicking on the main window still closes any previous popup correctly (if you don't open a new popup)?

The flushing of input happens AFTER the flushing of X11 events, and is called at the very end of process_events() function in x11 DisplayServer.
Apparently, such an arrangement does not guarantee that focus_in events are processed before the clicks they are associated with (this is an unoptimized build, compiler is probably innocent).

Yeah, I can see how that could be part of the problem. I'm not sure what the proper fix or design would be, but it might be something to investigate.

Although there's still something I don't fully understand in this chain of events:

[4642] ButtonPress window=8388610 (0), button_index=1 
menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fffacff9640 (LWP 2664)]
[4643] FocusOut window=8388753 (6), mode='0' 
[4643] FocusIn window=8388610 (0), mode='0' 
root window focused
hide was pushed
[4643] MapNotify window=8388763 (7) 
[4643] VisibilityNotify window=8388763 (7), state=0 
[4643] VisibilityNotify window=8388753 (6), state=1 
[4643] Expose window=8388763 (7), count='0' 
 visibility set to false
delete_sub_window: 8388753 (6) 
[Thread 0x7fff9ffff640 (LWP 2663) exited]
[4644] FocusOut window=8388610 (0), mode='0' 
[4644] FocusIn window=8388763 (7), mode='0' 
[4644] FocusOut window=8388763 (7), mode='0' 
[4644] FocusIn window=8388610 (0), mode='0' 

I understand the first change of focus from 6 to 0 is due to the mouse click, then the new window (7) is created and focused. But I wonder what is causing the last change of focus from 7 to 0 which is causing the new popup to close.

I wonder if it could be due to this when the previous popup (6) is closed somehow:

if (wd_window.menu_type && !wd_window.no_focus) {
if (!wd_parent.no_focus) {
XSetInputFocus(x11_display, wd_parent.x11_window, RevertToPointerRoot, CurrentTime);
}
}

Or maybe the use of RevertToPointerRoot is causing the closed window to reset the focus to 0. In this case, maybe we'll have to use RevertToPointerNone and find a different way to ensure the main window gains focus when all popups are closed.

@Houkime
Copy link
Author

Houkime commented Jul 14, 2021

If I remember correctly this was still important for focus to work correctly. When the line is disabled, does clicking on the main window still closes any previous popup correctly (if you don't open a new popup)?

No, the popup still remains open if this line is knocked out and if i click somewhere else.
However, this seems to happen only on dwm and i3.
LXQt and wayfire don't seem to be affected by the knockout.

I understand the first change of focus from 6 to 0 is due to the mouse click, then the new window (7) is created and focused. But I wonder what is causing the last change of focus from 7 to 0 which is causing the new popup to close.

As for the source of the second focus of the Main Window, you are right.
I have marked all places where the code could request a focus in, and it looks like this happens from window_set_transient() and the main window is supposed to be a transient parent.
One can notice that it is the FIRST popup that set_transient is being called on, and this call gets the root window focus, and this focus hides the second popup (and the second popup too fires its' set_transient() afterwards)

[700] ButtonPress window=25165826 (0), button_index=1 
requesting focus because of the click
menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fffacff9640 (LWP 5523)]
[701] MapNotify window=25165919 (1) 
requesting focus because menu window is started(MapNotify)
[701] VisibilityNotify window=25165919 (1), state=0 
[701] VisibilityNotify window=25165826 (0), state=1 
[701] Expose window=25165919 (1), count='0' 
[702] FocusOut window=25165826 (0), mode='0' 
[702] FocusIn window=25165919 (1), mode='0' 
[New Thread 0x7fff9ffff640 (LWP 5524)]
[Thread 0x7fff9ffff640 (LWP 5524) exited]
[715] ButtonRelease window=25165826 (0), button_index=1 

long pause between clicks

[1947] ButtonPress window=25165826 (0), button_index=1 
requesting focus because of the click
menu_button clicked 

about_to_popup
 visibility set to true
[New Thread 0x7fff9ffff640 (LWP 5525)]
[1948] FocusOut window=25165919 (1), mode='0' 
[1948] FocusIn window=25165826 (0), mode='0' 
root window focused
hide was pushed
[1948] MapNotify window=25165929 (2) 
requesting focus because menu window is started(MapNotify)
[1948] VisibilityNotify window=25165929 (2), state=0 
[1948] VisibilityNotify window=25165919 (1), state=1 
[1948] Expose window=25165929 (2), count='0' 
 visibility set to false
 from window_set_transient calling focus on (0) as a prev_parent of (1) 
delete_sub_window: 25165919 (1) 
[Thread 0x7fffacff9640 (LWP 5523) exited]
[1949] FocusOut window=25165826 (0), mode='0' 
[1949] FocusIn window=25165929 (2), mode='0' 
[1949] FocusOut window=25165929 (2), mode='0' 
[1949] FocusIn window=25165826 (0), mode='0' 
root window focused
hide was pushed
[1949] Expose window=25165826 (0), count='1' 
[1949] Expose window=25165826 (0), count='0' 
 visibility set to false
 from window_set_transient calling focus on (0) as a prev_parent of (2) 
delete_sub_window: 25165929 (2) 
[Thread 0x7fff9ffff640 (LWP 5525) exited]
[1950] VisibilityNotify window=25165826 (0), state=0 
[1950] Expose window=25165826 (0), count='0' 
[1956] ButtonRelease window=25165826 (0), button_index=1 

@Houkime
Copy link
Author

Houkime commented Jul 14, 2021

Or maybe the use of RevertToPointerRoot is causing the closed window to reset the focus to 0. In this case, maybe we'll have to use RevertToPointerNone and find a different way to ensure the main window gains focus when all popups are closed.

from man XSetInputFocus

The  specified focus window must be viewable at the time XSetInputFocus
       is called, or a BadMatch error results.  If the focus window later  be‐
       comes  not  viewable,  the X server evaluates the revert_to argument to
       determine the new focus window as follows:

       •    If revert_to is RevertToParent, the focus reverts  to  the  parent
            (or the closest viewable ancestor), and the new revert_to value is
            taken to be RevertToNone.

       •    If revert_to is RevertToPointerRoot or RevertToNone, the focus re‐
            verts  to  PointerRoot  or None, respectively.  When the focus re‐
            verts, the X server generates FocusIn and FocusOut events, but the
            last-focus-change time is not affected.

RevertTo kicks in only when the focus is lost due to the non-viewability of the current focus.
It does not act when the focus is actively transferred to something else. It is just a fallback.

Here, the problem is not RevertTo usage, but the fact that window_set_transient() actively sets the focus to the main window (main window is wd_parent in our case, it is a new focus).

In fact, if i just comment out this single line in window_set_transient(), everything just works, including submenus.

XSetInputFocus(x11_display, wd_parent.x11_window, RevertToPointerRoot, CurrentTime);

@Houkime
Copy link
Author

Houkime commented Jul 15, 2021

@pouleyKetchoupp I played with it for some time and i cannot seem to produce a situation where the program fails because this line in window_set_transient() is missing.
There is a comment there saying that this line is needed in a situation when both window and its' parent get destroyed, but even if such situations (like, clicking outside a submenu to close both it and the menu that spawmed it) nothing irregular seems to happen, at least with mouse.

I do run into some strange behaviour when using keyboard navigation, but i am not yet sure if it is because of this change, needs more testing.
BTW, this area of code is not under any sort of automatic test unfortunately.

@pouleyKetchoupp
Copy link
Contributor

@Houkime Great job on investigating and testing! I made that change as part of #41456, but unfortunately I didn't write a note about the exact case it was fixing in the PR and now I don't remember, my bad :/

It might be that it's needed for keyboard controls as you suggested, since in this case the WM or the focus set on mouse click events would not be able to do the work.

I'll do some tests on my side as well, on Gnome/KDE so we can have a clearer idea what this focus change in window_set_transient() fixes and what bugs it causes on different WMs.

Automatic tests would be great. On linux we would have to also test on different WMs to do it right so that could add to the complexity, but I totally agree it would help a lot to have it (even starting with something simple).

@Houkime
Copy link
Author

Houkime commented Jul 16, 2021

No, keyboard shenanigans are not related to the knocked out line. Or at least, they are not caused by the knockout (The line might still be necessary for them to actually work in the future).
And they are also not dwm/i3 specific
#50529

@Houkime
Copy link
Author

Houkime commented Jul 17, 2021

Ok, so i tried to make some test at least for this particular bug.
The main loop should be running for such x11 integration tests, so it was gdscript based.
Unfortunately GUT is not yet working with 4.0 version of gdscript, so it is an improvised plain gdscript version.

extends Node2D

func _ready():
	var but1 = MenuButton.new()
	prepare_popup_button(but1)
	but1.text = "button1"
	but1.rect_position = Vector2(100,0)
	
	var but2 = MenuButton.new()
	prepare_popup_button(but2)
	but2.text = "button2"
	but2.rect_position = Vector2(300,0)
	
	await wait_idle_frames(100)
	
	print("pressing 1st button")
	await realistic_lclick(center(but1))
	await wait_idle_frames(20)
	
	print("pressing 2nd button")
	await realistic_lclick(center(but2))
	await wait_idle_frames(20)
	
	assert(but1.get_popup().visible == false,\
				"first popup remains open. FAILURE")
	assert(but2.get_popup().visible == true,\
				"second popup gets hidden. FAILURE")

func center(control):
	return (control.get_rect().position + control.get_rect().end)/2

func realistic_lclick(vector2):
	
	var press_ev = InputEventMouseButton.new()
	press_ev.pressed = true
	press_ev.button_index = MOUSE_BUTTON_LEFT
	press_ev.button_mask = MOUSE_BUTTON_MASK_LEFT
	press_ev.position = vector2
	
	print("input - parsing press")
	Input.parse_input_event(press_ev)
	
	await wait_idle_frames(20)
	
	var release_ev = InputEventMouseButton.new()
	release_ev.pressed = false
	release_ev.button_index = MOUSE_BUTTON_LEFT
	release_ev.button_mask = MOUSE_BUTTON_MASK_LEFT
	release_ev.position = vector2
	
	print ("input - parsing release")
	Input.parse_input_event(release_ev)

func wait_idle_frames(n):
	for i in range(n):
		await get_tree().process_frame

func prepare_popup_button(popup_button):
	popup_button.text = "TEST BUTTON"
	var submenu = PopupMenu.new()
	submenu.set_name("submenu")
	submenu.add_item("submenu item")
	popup_button.get_popup().add_child(submenu)
	popup_button.get_popup().add_submenu_item("submenu label","submenu")
	add_child(popup_button)

It looks like this (first just editor, then i press F5, and the test runs, and then editor shows me the failed assert)
test_bug

This outputs (with debug things)

[1] ConfigureNotify window=16777218 (0), event=16777218, above=0, override_redirect=0 
pressing 1st button
input - parsing press
menu_button clicked 

about_to_popup
 visibility set to true
[101] MapNotify window=16777315 (1) 
requesting focus because menu window is started(MapNotify)
[101] VisibilityNotify window=16777315 (1), state=0 
[101] VisibilityNotify window=16777218 (0), state=1 
[101] Expose window=16777315 (1), count='0' 
[102] FocusOut window=16777218 (0), mode='0' 
[102] FocusIn window=16777315 (1), mode='0' 
input - parsing release
pressing 2nd button
input - parsing press
menu_button clicked 

about_to_popup
 visibility set to true
[141] MapNotify window=16777325 (2) 
requesting focus because menu window is started(MapNotify)
[141] VisibilityNotify window=16777325 (2), state=0 
[141] Expose window=16777325 (2), count='0' 
[142] FocusOut window=16777315 (1), mode='0' 
[142] FocusIn window=16777325 (2), mode='0' 
input - parsing release

And it fails. Unfortunately not because the second menu closes but because the first menu remains open.
And it remains open because our clicks are not real clicks and thus root window does not get focused when we click away from the first menu.
So, such testing is probably not really what is needed here (or one needs much deeper click emulation with gdscript)

A project if you want:
gdscript_for_tests_test_no_gut.zip

@Houkime
Copy link
Author

Houkime commented Jul 17, 2021

The idea of gdscript-based test came from here godotengine/godot-proposals#1760.
Unfortunately, as shown above, it does not really work as intended.

@Xrayez, this issue is a bit stuck because i cannot reliably test if everything would be ok if i delete one line of code that produces this bug, or refactor anything to make the focus handling design intentions clearer.
Testing manually seems complicated in this particular case (needs to be carried out on all WMs and also, there is no actual procedure for this that goes from the history of previous bugs).
If we could make some sort of testing, then one could just say that if the forgotten bug this line-to-be-deleted solves arises again, ok we fix it again and this time write a test for it. Progress.

You have previously tried to make this sort of automated testing for godot, do you have eny recommendations on how to proceed?
It needs to be an x11 integration test suite, so one either needs a real x11+WM and a running main loop, or a mock x11 injecting events and reacting in simple ways (mimicking quirks of different WMs) (and probably a running loop too).
The former can be achieved with gdscript-based testing, but gdscript input emulation is not deep enough for this particular purpose. The latter can be achieved by making a mock X11 environment, and polling it instead of a real one, but realism is a problem then. Though, one could probably separate core scenarios and replay them.

There are at least Qt and GTK who handle this integration reliably SOMEHOW, but i need to research on how exactly they are doing this. Also, they might be effectively single-window or sth.

@Houkime
Copy link
Author

Houkime commented Jul 17, 2021

@pouleyKetchoupp also one could entirely separate closing of popup menus from X11 specifics or focus-handling in general, leaving it entirely on the Input:: level. Which is probably the best solution if it can be achieved.
Or make focus-handling much stricter regulated and totally controlled by godot itself, with less assumptions about how x11 environment should behave.

@Calinou
Copy link
Member

Calinou commented Jul 17, 2021

It needs to be an x11 integration test suite, so one either needs a real x11+WM and a running main loop, or a mock x11 injecting events and reacting in simple ways (mimicking quirks of different WMs) (and probably a running loop too).

A desktop automation framework like Robot Framework could be suited here.

@Houkime
Copy link
Author

Houkime commented Jul 17, 2021

It needs to be an x11 integration test suite, so one either needs a real x11+WM and a running main loop, or a mock x11 injecting events and reacting in simple ways (mimicking quirks of different WMs) (and probably a running loop too).

A desktop automation framework like Robot Framework could be suited here.

Or one can just ask X11 nicely, since we are including the X11 headers anyway.

This is what xdotool does for mouseclicks faking. Notice how if we want to send to the current window, it is a very simple function (makes use of X11/extensions/XTest.h).
Unfortuunately, libxtst is not universally installed.
But as one can notice, the more precise branch does not use it and is still somewhat ok.
https://github.com/jordansissel/xdotool/blob/master/xdo.c#L802

int _xdo_mousebutton(const xdo_t *xdo, Window window, int button, int is_press) {
  int ret = 0;

  if (window == CURRENTWINDOW) {
    ret = XTestFakeButtonEvent(xdo->xdpy, button, is_press, CurrentTime);
    XFlush(xdo->xdpy);
    return _is_success("XTestFakeButtonEvent(down)", ret == 0, xdo);
  } else {
    /* Send to specific window */
    int screen = 0;
    XButtonEvent xbpe;
    charcodemap_t *active_mod;
    int active_mod_n;

    xdo_get_mouse_location(xdo, &xbpe.x_root, &xbpe.y_root, &screen);
    xdo_get_active_modifiers(xdo, &active_mod, &active_mod_n);

    xbpe.window = window;
    xbpe.button = button;
    xbpe.display = xdo->xdpy;
    xbpe.root = RootWindow(xdo->xdpy, screen);
    xbpe.same_screen = True; /* Should we detect if window is on the same
                                 screen as cursor? */
    xbpe.state = xdo_get_input_state(xdo);

    xbpe.subwindow = None;
    xbpe.time = CurrentTime;
    xbpe.type = (is_press ? ButtonPress : ButtonRelease);

    /* Get the coordinates of the cursor relative to xbpe.window and also find what
     * subwindow it might be on */
    XTranslateCoordinates(xdo->xdpy, xbpe.root, xbpe.window, 
                          xbpe.x_root, xbpe.y_root, &xbpe.x, &xbpe.y, &xbpe.subwindow);

    /* Normal behavior of 'mouse up' is that the modifier mask includes
     * 'ButtonNMotionMask' where N is the button being released. This works the
     * same way with keys, too. */
    if (!is_press) { /* is mouse up */
      switch(button) {
        case 1: xbpe.state |= Button1MotionMask; break;
        case 2: xbpe.state |= Button2MotionMask; break;
        case 3: xbpe.state |= Button3MotionMask; break;
        case 4: xbpe.state |= Button4MotionMask; break;
        case 5: xbpe.state |= Button5MotionMask; break;
      }
    }
    ret = XSendEvent(xdo->xdpy, window, True, ButtonPressMask, (XEvent *)&xbpe);
    XFlush(xdo->xdpy);
    free(active_mod);
    return _is_success("XSendEvent(mousedown)", ret == 0, xdo);
  }
}

@Xrayez
Copy link
Contributor

Xrayez commented Jul 17, 2021

You have previously tried to make this sort of automated testing for godot, do you have eny recommendations on how to proceed?

I'm not using Godot 4.x currently so I'm not really familiar with the new windows system in Godot to be honest. There's a fuzzer currently integrated into CI:

# Execute unit tests for the editor
- name: Unit Tests
run: |
./bin/godot.linuxbsd.tools.64s --test
# Download, unzip and setup SwiftShader library [d4550ab8d3f]
- name: Download SwiftShader
run: |
wget https://github.com/qarmin/gtk_library_store/releases/download/3.24.0/swiftshader.zip
unzip swiftshader.zip
rm swiftshader.zip
curr="$(pwd)/libvk_swiftshader.so"
sed -i "s|PATH_TO_CHANGE|$curr|" vk_swiftshader_icd.json
# Download and extract zip archive with project, folder is renamed to be able to easy change used project
- name: Download test project
run: |
wget https://github.com/qarmin/RegressionTestProject/archive/4.0.zip
unzip 4.0.zip
mv "RegressionTestProject-4.0" "test_project"
# Editor is quite complicated piece of software, so it is easy to introduce bug here
- name: Open and close editor
run: |
VK_ICD_FILENAMES=$(pwd)/vk_swiftshader_icd.json DRI_PRIME=0 xvfb-run bin/godot.linuxbsd.tools.64s --audio-driver Dummy -e -q --path test_project 2>&1 | tee sanitizers_log.txt || true
misc/scripts/check_ci_log.py sanitizers_log.txt
# Run test project
- name: Run project
run: |
VK_ICD_FILENAMES=$(pwd)/vk_swiftshader_icd.json DRI_PRIME=0 xvfb-run bin/godot.linuxbsd.tools.64s 40 --audio-driver Dummy --path test_project 2>&1 | tee sanitizers_log.txt || true
misc/scripts/check_ci_log.py sanitizers_log.txt

and AFAIK it does test GUI classes (basically all classes which are exposed in ClassDB).

But even if it does such testing, it would be difficult to reproduce. Testing this on a even deeper level may end up being too much work, or too much maintenance work, multiplied by differences in platform implementations (what about Windows platform)?

As I've previously said, GDScript works fine in single-windowed mode in 3.x, for instance I've recently created some tests for GridRect class in Goost: https://github.com/goostengine/goost/blob/2492e03adbf868fe7a673685a0a7d9dec9773300/tests/project/goost/scene/gui/grid_rect.gd. It does look complex, but a more clean framework could be written specifically for testing GUI stuff programmatically. If it doesn't work in other cases, then maybe it's not really worth it. If it's not currently possible to test in GDScript, perhaps more methods in DisplayServer could be exposed to help this (just as an idea).

X11 specific testing facilities could be integrated if they help the initial development I guess, but at the end of the day, it would make more sense to come up with something which does not require dealing with platform-specific functions directly, that's more implementation detail.


Having said that, I really don't know how to proceed with this specific issue, rendering/WMs is not my area of expertise/interest, just some food for thought!

@pouleyKetchoupp
Copy link
Contributor

@Houkime Sorry for the delayed answer. I was able to confirm the issue on Archlinux/i3 and opened #50712, please check if it's working for you. I also figured out the reason I had added this focus on parent in window_set_transient in the first place and updated the comment for the future. It was because otherwise, when a nested sub-menu closes, the whole popup would close (at least on Gnome).

Thanks again for all the investigation! I agree with the idea that we would be better off with proper regression tests for the editor ui system, especially on X11. I don't personally have enough bandwidth to spend too much time on Linux these days, but contributions on this topic are very welcome.

@pouleyKetchoupp also one could entirely separate closing of popup menus from X11 specifics or focus-handling in general, leaving it entirely on the Input:: level. Which is probably the best solution if it can be achieved.
Or make focus-handling much stricter regulated and totally controlled by godot itself, with less assumptions about how x11 environment should behave.

That sounds like an interesting idea too. I imagine it would have a to be a refactoring of all platform code and would need proper evaluation, but in the long run it does seem like it could make things easier (at least for X11, I don't know other platforms as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants