popup deferred hide suppressed if reopened on Windows platform #61001

derammo · 2022-05-13T13:36:38Z

Summary

These changes address two problems with the way deferred close of popups is implemented on the Windows platform.

Problem 1: call_deferred("hide") on popups causes a duplicate call_deferred("hide")

This is addressed by having the relevant notification code in DisplayServerWindows not notify the same window that is the source of the close request.

This problem most likely affects other platforms, but there don't seem to be any issues reported on other platforms, so maybe it does no harm there. At least the X11 display manager appears to operate the same way.

Problem 2: popup requests deferred hide of itself, but is reopened before that hide operation is executed

This happens due to the sequence of operations in the main loop. Deferred calls and buffered input are on separate queues and therefore get out of order. To work around this, popup will now be aware of its popped_up state even when single window mode is not used. If a request to hide is processed after the same popup window is reopened, it won't hide.

Note that several other places in the code base also use deferred hide to close the current window, in order to pop out of the stack and process messages correctly. The resulting reordering probably cannot be corrected centrally in the Window code, because it would change how Window works in a basic way, breaking many things. I tried some different ways to fix it centrally, but will just create a discussion instead. This fix just for the Popup() class addressed the existing issues.

Problem sequence of calls

This shows the sequence of calls that happens during a right mouse click that closes a popup in one location and opens it in the other. This figure is showing the slow case, where even with delayed input the (second) hide is processed after the reopen. There are also timings where the input and the (first) hide happen in the same main loop iteration, but the same problems occur.

OS_Windows::run
    DisplayServerWindow::process_events()
        MouseProc right click
            _send_window_event WINDOW_EVENT_CLOSE_REQUEST
                Window::_propagate_window_notification NOTIFICATION_WM_CLOSE_REQUEST
                    PopupMenu::_notification
                        Popup::_notification
                            Popup::_close_pressed
                                call_deferred this->Window::hide
    Main::iteration
        Scene_tree::physics_process
            flush dispatches Window::hide
                Window::set_visible (false)
                    Window::_clear_window
                        DisplayServerWindows::delete_sub_window
                            DisplayServerWindows::popup_close
                                _send_window_event WINDOW_EVENT_CLOSE_REQUEST
                                    Window::_propagate_window_notification NOTIFICATION_WM_CLOSE_REQUEST
                                        PopupMenu::_notification
                                            Popup::_notification
                                                Popup::_close_pressed
                                                    call_deferred this->Window::hide

OS_Windows::run
    DisplayServerWindow::process_events()
        WndProc right click
            Input::parse_input_event
                buffered_events.push_back
    Main::iteration
        Input::flush_buffered_events
            long way through the gui ...
                PopupMenu::popup
                    Window::popup
                        Window::set_visible (true)
                            window set visible in new location
        Scene_tree::physics_process
            flush dispatches Window::hide from previous sequence
                Window::set_visible (false)
                    window was reused in the time we were async, close it again

After the changes in this PR, the sequences are intended to be as follows:

Fixed case 1: reopened before hide executes -> don't hide at all

OS_Windows::run
    DisplayServerWindow::process_events()
        MouseProc right click
            _send_window_event WINDOW_EVENT_CLOSE_REQUEST
                Window::_propagate_window_notification NOTIFICATION_WM_CLOSE_REQUEST
                    PopupMenu::_notification
                        Popup::_notification
                            Popup::_close_pressed
                                popped_up = false
                                call_deferred this->Popup::_popup_conditional_hide
        WndProc right click
            Input::parse_input_event
                buffered_events.push_back
    Main::iteration
        Input::flush_buffered_events
            long way through the gui ...
                PopupMenu::popup
                    Window::popup
                        Window::set_visible (true)
                            does nothing since already visible
                        Popup::_post_popup
                            popped_up = true
        Scene_tree::physics_process
            flush dispatches Popup::_popup_conditional_hide
                popped_up == true, do nothing

Fixed case2: not reopened before hide executes -> execute hide, but only once

OS_Windows::run
    DisplayServerWindow::process_events()
        MouseProc right click
            _send_window_event WINDOW_EVENT_CLOSE_REQUEST
                Window::_propagate_window_notification NOTIFICATION_WM_CLOSE_REQUEST
                    PopupMenu::_notification
                        Popup::_notification
                            Popup::_close_pressed
                                popped_up = false
                                call_deferred this->Popup::_popup_conditional_hide
        WndProc right click
            Input::parse_input_event
                buffered_events.push_back
    Main::iteration
        Scene_tree::physics_process
            flush dispatches Popup::_popup_conditional_hide
                popped_up == false
                Window::set_visible (false)
                    Window::_clear_window
                        DisplayServerWindows::delete_sub_window
                            DisplayServerWindows::popup_close
                                don't send close request to self, only other popups

Limitations

This fix does not change the behavior of --single-window mode. In this mode, popups already don't automatically close when you click else where. This is unchanged from the current 4.0 master, and I do not know what the intended behavior is in this mode.

popup no longer tries to close itself a second time popup no longer closes after having been reopened fixed bug in RenameDialog not calling base (by inspection) fixes godotengine#59181 fixes godotengine#60921 reverts godotengine#59287

bruvzg

The changes seem good to me, I'll fully test it tomorrow.

bruvzg · 2022-05-13T13:42:29Z

platform/windows/display_server_windows.cpp

+		if (win_id != p_window) {
+			// Only request close on related windows, not this window.  We are already processing it.
+			_send_window_event(windows[win_id], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
+		}


This is likely relevant for X11 and macOS as well (need testing, code is almost same).

I just put that in the notes above, since I am not set up to test Linux (yet) unfortunately. Is there something else I could have done to make sure this PR gets to the right "other platforms" people? Obviously you caught it this time, just wondering about future.

derammo · 2022-05-16T22:42:18Z

@bruvzg do we need someone from X11 and MacOS before this can advance?

bruvzg

Seems to be working fine on Windows, since neither of the mentioned bugs is reproducible on Linux/macOS there's no need to add anything else to the same PR.

akien-mga · 2022-05-17T06:30:45Z

scene/gui/popup.cpp

-	call_deferred(SNAME("hide"));
+	// Hide after returning to process events, but only if we don't
+	// get popped up in the interim.
+	call_deferred(SNAME("_popup_conditional_hide"));


I think you could remove the need for binding this method in _bind_methods (which makes it accessible from scripting, even if hidden by default due to the leading _) by using Callables instead of StringNames:

Suggested change

call_deferred(SNAME("_popup_conditional_hide"));

callable_mp(this, &Popup::_popup_conditional_hide).call_deferred({}, 0);

CC @KoBeWi to confirm, currently we seem to only use Callable::call_deferred on a callable_mp once:

scene/3d/collision_object_3d.cpp 322: callable_mp(this, &CollisionObject3D::_update_debug_shapes).call_deferred({}, 0);

Yes, thanks! That's exactly what I wanted. I will wait for confirmation.

I have to admit though, if it is only used in one place right now, that seems a bit high on the risk/reward scale? We already tested as written, including bruvzg's time.

Well Callables and callable_mp are new in 4.0 and while a big part of the codebase has been ported where it was trivial, I don't think we've really looked at deferred calls yet. But it should work fine, and that should be the "good" approach going forwards. The less we need to rely on strings, the better.

As we don't agree on this, I'll do it myself in a follow up.

No argument there. I just wouldn't put that on this PR at this point in time. I await your decision after discussion off-line.

After further discussion, it was made clear that converting remaining uses of call_deferred(StringName, VarArgs...) is not that trivial and requires further discussion to define how we want to do this exactly (if at all). So it was indeed well outside the scope of this PR.

Here's an example conversion in a case where there's actually an argument to pass to call_deferred and how it requires getting a const Variant ** to pass:

diff --git a/modules/openxr/editor/openxr_action_map_editor.cpp b/modules/openxr/editor/openxr_action_map_editor.cpp index 6e9a2e1b61..3d40e4853b 100644 --- a/modules/openxr/editor/openxr_action_map_editor.cpp +++ b/modules/openxr/editor/openxr_action_map_editor.cpp @@ -183,7 +183,8 @@ void OpenXRActionMapEditor::_on_add_action_set() { // Make sure our action set is the current tab tabs->set_current_tab(0); - call_deferred("_set_focus_on_action_set", new_action_set_editor); + const Variant *argp = &new_action_set_editor; + callable_mp(this, &OpenXRActionMapEditor::_set_focus_on_action_set).call_deferred(&argp, 1); } void OpenXRActionMapEditor::_set_focus_on_action_set(OpenXRActionSetEditor *p_action_set_editor) {

Which is arguably more bothersome than the original code, even if it makes it possible to unbind the method.

akien-mga · 2022-05-17T10:56:57Z

Thanks!

popup deferred hide suppressed if reopened

47d0dc8

popup no longer tries to close itself a second time popup no longer closes after having been reopened fixed bug in RenameDialog not calling base (by inspection) fixes godotengine#59181 fixes godotengine#60921 reverts godotengine#59287

derammo requested review from a team as code owners May 13, 2022 13:36

KoBeWi added bug platform:windows topic:editor labels May 13, 2022

KoBeWi added this to the 4.0 milestone May 13, 2022

bruvzg reviewed May 13, 2022

View reviewed changes

bruvzg approved these changes May 17, 2022

View reviewed changes

akien-mga reviewed May 17, 2022

View reviewed changes

akien-mga merged commit ccdd85d into godotengine:master May 17, 2022

derammo deleted the derammo_popup_conditional_hide branch September 15, 2022 10:39

bruvzg mentioned this pull request May 28, 2024

[macOS, X11] Fix duplicate window close requests. #92458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

popup deferred hide suppressed if reopened on Windows platform #61001

popup deferred hide suppressed if reopened on Windows platform #61001

derammo commented May 13, 2022

bruvzg left a comment

bruvzg May 13, 2022

derammo May 13, 2022

derammo commented May 16, 2022

bruvzg left a comment

akien-mga May 17, 2022

derammo May 17, 2022

derammo May 17, 2022

akien-mga May 17, 2022

akien-mga May 17, 2022

derammo May 17, 2022

akien-mga May 17, 2022 •

edited

Loading

akien-mga commented May 17, 2022

	call_deferred(SNAME("_popup_conditional_hide"));
	callable_mp(this, &Popup::_popup_conditional_hide).call_deferred({}, 0);

popup deferred hide suppressed if reopened on Windows platform #61001

popup deferred hide suppressed if reopened on Windows platform #61001

Conversation

derammo commented May 13, 2022

Summary

Problem 1: call_deferred("hide") on popups causes a duplicate call_deferred("hide")

Problem 2: popup requests deferred hide of itself, but is reopened before that hide operation is executed

Problem sequence of calls

Fixed case 1: reopened before hide executes -> don't hide at all

Fixed case2: not reopened before hide executes -> execute hide, but only once

Limitations

bruvzg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derammo commented May 16, 2022

bruvzg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga May 17, 2022 • edited Loading

Choose a reason for hiding this comment

akien-mga commented May 17, 2022

akien-mga May 17, 2022 •

edited

Loading