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

Fix progress dialog steals focus #97009

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Sep 14, 2024

This should resolve the issue where the Progress Dialog steals focus while the Godot Editor is not the active window.

Initially, I tried using different window options, but it’s simply not possible to display a new popup without it appearing in front of other windows or taking focus, at least on Windows.

The solution was to replace the use of a PopupPanel with a simple PanelContainer. The advantage here is that it should be faster to show or hide since no new window creation is involved. Additionally, I’ve seen suggestions in other issues/proposals to make the progress dialog less intrusive. With this approach, it could easily be moved to a corner, resized, etc., in the future, or even made non-exclusive.

At first, I thought using a PanelContainer might cause problems since it’s not exclusive, but since the Progress Dialog is always used on the main thread, the user cannot interact with the editor while it’s displayed. Still, I used a transparent full screen PanelContainer to prevent interactions with the mouse. The only potential issue I noticed is that a user could enter shortcuts while the progress dialog is visible, and when it closes, all queued shortcuts would be executed. I'm not sure if that could be prevented.

There’s also a slight visual difference with the default theme: the progress dialog now has a shadow around it. I don’t think this is a significant issue.

Lastly, I removed a line in WindowWrapper::WindowWrapper that I didn’t fully understand. I assume it was intended to display the ProgressDialog centered on the current window, but I’m not sure it was functioning properly. In my tests with the ScriptEditor, even in a floating window, the progress dialog was displayed in the middle of the main editor window.

Old progress dialog:
image

New progress dialog:
image

@Hilderin Hilderin added this to the 4.4 milestone Sep 14, 2024
@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch 2 times, most recently from fff6c76 to b0ac658 Compare September 14, 2024 19:45
@KoBeWi
Copy link
Member

KoBeWi commented Sep 14, 2024

Still, I used a transparent full screen PanelContainer to prevent interactions with the mouse.

Why PanelContainer, not a Control, or MarginContainer if you needed container?

@Hilderin
Copy link
Contributor Author

That's a really good point! The only reason I didn't consider it is because I don't have much experience with the GUI in Godot, and it didn’t cross my mind. I’ll make the adjustment :)

@KoBeWi
Copy link
Member

KoBeWi commented Sep 14, 2024

Also that hack doesn't work, the editor still takes input:

godot.windows.editor.dev.x86_64_ka0ROBQKOb.mp4

Also2, if a dialog appears while loading (e.g. plugin is disabled due to script errors), it will cover the progress "dialog".

editor/progress_dialog.cpp Outdated Show resolved Hide resolved
@bruvzg
Copy link
Member

bruvzg commented Sep 14, 2024

Initially, I tried using different window options, but it’s simply not possible to display a new popup without it appearing in front of other windows or taking focus, at least on Windows.

It should not take focus if NO_FOCUS flag was set before calling show_window. We probably should split DisplayServers show_window to separate show_window, order_window and focus_window to have more control.

@Hilderin
Copy link
Contributor Author

Hilderin commented Sep 14, 2024

It should not take focus if NO_FOCUS flag was set before calling show_window. We probably should split DisplayServers show_window to separate show_window, order_window and focus_window to have more control.

If we use NO_FOCUS, that works, it does not steal to focus but calling SetWindowPos even with SWP_NOACTIVATE still focus Godot because it's a child window. If we remove completely the call to SetWindowPos, it works, but the progress dialog is displayed in front of the current application. Maybe there's something I missed?!?!

@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch from b0ac658 to 08ea351 Compare September 14, 2024 21:05
@Hilderin
Copy link
Contributor Author

Why PanelContainer, not a Control, or MarginContainer if you needed a container?

I used a CenterContainer, so I could remove the code that positioned the dialog in the center of the screen.

Also that hack doesn't work, the editor still takes input

I fixed it by adjusting the node order to ensure the ProgressDialog is the last node.

If a dialog appears while loading (e.g., a plugin is disabled due to script errors), it will cover the progress "dialog".

That’s a limitation of this solution. I don’t think it’s possible to avoid it without using an actual window. One think I did for the "Load errors" in #96830 was to display popup only after progress is gone. Is it really a problem? Maybe that's a way to do it but it could be difficult to find every possible dialog.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 14, 2024

Now mouse input is blocked properly, but as you noted, you can still use shortcuts. Maybe it can be solved by a node that intercepts all input and sets it as handled?

@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch 2 times, most recently from a08a410 to 1be429a Compare September 15, 2024 00:24
@Hilderin
Copy link
Contributor Author

Following your suggestion I overrided the input method in EditorNode and when the ProcessDialog is visible. That seems to fix the problem with the shortcuts.

@WhalesState
Copy link
Contributor

WhalesState commented Sep 15, 2024

void EditorNode::input(const Ref<InputEvent> &p_event) {
	Ref<InputEventKey> k = p_event;
	// Return early to avoid checking if `progress_dialog` is visible on every event, like mouse motion events. We also can return if it's an echo event, but i guess it may cause issues if the editor nodes are processing the echo events too.
	// Maybe we also can use this method to block mouse input instead of using a `Control` node. Just handle them without checking if it's a key input.
	if (k.is_null()) {
		return;
	}
	// Prevent all shortcuts while the progress dialog is displayed.
	// To simulate an exclusive popup.
	if (progress_dialog->is_visible()) {
		get_tree()->get_root()->set_input_as_handled();
	}
}

@Hilderin
Copy link
Contributor Author

Hilderin commented Sep 15, 2024

// Maybe we also can use this method to block mouse input instead of using a Control node. Just handle them without checking if it's a key input.

I thought about that, but since the ProgressDialog can have a Cancel button, I needed to allow mouse events to enable clicking on it. A validation to check if the mouse coordinates are inside the progress dialog could be done, but I found it a bit complex compared to using a full-size control. On the other hand, it would eliminate the need to keep the ProgressDialog as the last node. I'm not sure which approach is best, honestly. What do you think?

@WhalesState
Copy link
Contributor

WhalesState commented Sep 15, 2024

You can disable the input completly for the Editor Window when the ProgressDialog is visible, something like get_tree()->get_root()->set_gui_disable_input(true) and enable it back when the ProgressDialog becomes hidden.

@Hilderin
Copy link
Contributor Author

You can disable the input completly for the Editor Window when the ProgressDialog is show, something like get_tree()->get_root()->set_gui_disable_input(true) and enable it back when the ProgressDialog becomes hidden.

That would be smart :) I'll try it a bit later, thanks!

@WhalesState
Copy link
Contributor

You can disable the input completly for the Editor Window when the ProgressDialog is show, something like get_tree()->get_root()->set_gui_disable_input(true) and enable it back when the ProgressDialog becomes hidden.

That would be smart :) I'll try it a bit later, thanks!

Let's just hope that disabling the Input for the the root Window doesn't affect all it's Window children, we also can try to pause the editor and we make the ProgressDialog always process, I was thinking also about disabling the input processing for the EditorNode but this will also disable it for the ProgressDialog, unless we make them sibilings.

@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch from 1be429a to 14ae9ac Compare September 15, 2024 23:29
@Hilderin
Copy link
Contributor Author

After some testing, set_gui_disable_input and set_disable_input is only available at the Viewport level, so disabling inputs at the root prevent using the Cancel button in the ProgressDialog. Also, the idea to pause the EditorNode is not a bad one, but the problem is that a lot of system processing is done into EditorNode and uses the ProgressDialog. I fear that if the node is paused, it could lead to some weird situations or deadlocks.

For the moment, I only applied some of the suggested refactoring in EditorNode::input. By the way, the EditorNode::input method will be called only when the ProgressDialog is visible because of the call to EditorNode::get_singleton()->set_process_input when the dialog opens and closes.

editor/editor_node.cpp Outdated Show resolved Hide resolved
@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch from 14ae9ac to 18d64b8 Compare September 16, 2024 00:30
@WhalesState
Copy link
Contributor

WhalesState commented Sep 16, 2024

disable_gui_input can be accessed with a cast, since it's a public function set_disable_input.

EditorNode::get_singleton()->get_viewport()->set_disable_input(true);

Tested with gdscript and it only disables it for the Viewport and all it's children including other SubViewport nodes, but not for other Window children. Which should work the same for this issue.

I have used a SubViewport instead of the root since I can't access disable_gui_input in gdscript. disabling it's input doesn't call any input for all it's children, but the child Window was an exception and all it's input works since it's getting the pure input from the DisplayServer and are not propagated from the parent Viewport.
image

extends Control

func _ready() -> void:
	$"%Button".pressed.connect(func():
		$"%Window".popup_centered()
		$"%SubViewport".gui_disable_input = true
	)
	$"%Cancel".pressed.connect(func():
		$"%Window".hide()
		$"%SubViewport".gui_disable_input = false
	)
Window2.mp4

And this was the scene setup, the SubViewportContainer fills the screen and stretch = true.
image

Try to replace both lines EditorNode::get_singleton()->set_process_input(false/true); with:

EditorNode::get_singleton()->get_viewport()->set_disable_input(true/false);

And just comment the lines inside the EditorNode cpp _input() and the Control mouse blocker show() and hide() to test it fast without rebuilding the whole editor.

@WhalesState
Copy link
Contributor

WhalesState commented Sep 16, 2024

Tested with EditorNode::get_singleton()->get_viewport()->set_disable_input(true); inside ProgressDialog::_popup and EditorNode::get_singleton()->get_viewport()->set_disable_input(false); inside ProgressDialog::end_task after hiding it.

I didn't make any other changes to the 4.3 branch.

IT WORKS! 😄

I also have forced the cancel button to show, just to make sure I can press it.

There's a lag in the video, the buttons are just hovered when the window is not focused, but when i try to press them nothing happens and they can't be hovered again after focusing the window and the shortcut input doesn't work.

Input.mp4

@Hilderin
Copy link
Contributor Author

You tried directly from master? It's normal that it worked because the ProgressDialog in master is another Viewport, it's another Window. You should try the same thing with the code from the PR, when I did the same test, the cancel button did not work because now the ProgressDialog and the EditorNode are in the same Viewport. Which is the hole point to prevent a popup to steal the focus.

@WhalesState

This comment was marked as resolved.

@WhalesState
Copy link
Contributor

WhalesState commented Sep 16, 2024

Instead of changing it to a CenterContainer, you can make the Window unfocusable, set_flag(Window::FLAG_NO_FOCUS, true) inside ProgressDialog::ProgressDialog().
This will not focus the ProgressDialog when we use popup(), unless we interact with the window title with mouse clicks or we resize the window, which can't happen because the ProgressDialog is borderless and not resizable.

window-focus.mp4

@WhalesState
Copy link
Contributor

WhalesState commented Sep 16, 2024

I also have found another limitation.
The ProgressDialog before was hostable, for example all the window wrappers like ScriptEditor, EditorDocks, ShaderEditor are registered as hosts, and when the ProgressDialog is about to popup, it search for the focused window to appear exclusive to it, like if the ScriptEditor is the current focused window, it will appear exclusive to it. After removing this code, the ProgressDialog will always appear on top of the EditorInterface only.

Also you will have to disable the input for all the window wrappers too, because if one of them is visible while the ProgressDialog is visible and loading, we will be able to interact with it.

To reproduce this issue, make the script editor floating and then remove all the .import files inside the project folder, and return back to the script window (backup the project first to not lose any data), it will not show that the ProgressDialog is importing and also you will be able to change the script while it's reimporting.

The solution is to keep the ProgressDialog as a PopupPanel since it will require alot of work to simulate a fake exclusive Window and to mimic the same old behavior.

Maybe just this line can solve both issues without causing any regression later.

image

@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch from 18d64b8 to a7d63a5 Compare September 24, 2024 10:03
@Hilderin
Copy link
Contributor Author

Also you will have to disable the input for all the window wrappers too, because if one of them is visible while the ProgressDialog is visible and loading, we will be able to interact with it.

This should be fixed now. I added a set_process_mode(PROCESS_MODE_DISABLED) on all windows in WindowWrapper.

The solution is to keep the ProgressDialog as a PopupPanel since it will require alot of work to simulate a fake exclusive Window and to mimic the same old behavior.

Check: #97009 (comment)

@WhalesState
Copy link
Contributor

WhalesState commented Sep 24, 2024

Sorry for the noise, and thanks for fixing this.

I have been using it with Window::FLAG_NO_FOCUS for a week now, and yes it was annoying that it appears on top of other applications, the only way to fix this and yet we make it appears exclusive to the current window is to add a new flag FLAG_EMBEDDED to Window so it forces embedding a Window to another Window or Viewport. Or maybe later we can move the Panel to the current godot focused window to be noticed.

main->set_offset(SIDE_TOP, style->get_margin(SIDE_TOP));
main->set_offset(SIDE_BOTTOM, -style->get_margin(SIDE_BOTTOM));
// Be sure it's always the very last node to prevent user interaction while the dialog is visible.
get_parent()->move_child(this, get_parent()->get_child_count() - 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
get_parent()->move_child(this, get_parent()->get_child_count() - 1);
get_parent()->move_child(this, -1);

@KoBeWi
Copy link
Member

KoBeWi commented Sep 24, 2024

Found a new problem 🙃
When you export project, the progress dialog will appear behind export dialog.

editor/progress_dialog.h Outdated Show resolved Hide resolved
@Hilderin Hilderin force-pushed the fix-progress-dialog-steals-focus branch from a7d63a5 to 2fa29db Compare September 24, 2024 22:47
@Hilderin
Copy link
Contributor Author

When you export a project, the progress dialog appears behind the export dialog.

As WhalesState suggested, the solution could be to display the ProgressDialog within the dialog when a dialog is focused, rather than in the main window. I checked the codebase, and only a few dialogs use the ProgressDialog. One way to achieve this could be to add a check in EditorProgress::EditorProgress to determine if the current window is a dialog, and dynamically add a ProgressDialog control into the dialog at runtime, removing it in the EditorProgress::EditorProgress destructor.

Another idea is to have two types of ProgressDialog: the old modal dialog and the "new" panel version. When progress occurs in a dialog, we could use the modal dialog version as before. The advantage of this approach is that we could reuse the old dialog in most cases except during startup and reimporting assets, which could also reduce potential side effects.

Do you think this is a good idea?

@WhalesState
Copy link
Contributor

WhalesState commented Sep 25, 2024

A hack for this which may save us from adding a Control mouse blocker and accepting the key input is to popup a fake borderless and unfocusable Window that has a size of (1, 1) to the current active window, similar to how the ColorPicker makes one when picking a screen color, and we will be able to move the Panel freely to any active window without disabling the process or caring about mouse or key input, then we hide the fake window when the progress is finished.

Maybe we will still need the host code for the ProgressDialog to loop over the window wrappers and we find our target, but the fake window will also block the cancel button 😞, can't we just listen to the Escape key when a progress can be canceled? and we show a label 'Press "Escape" key to cancel" instead of showing the Cancel button?

Yet the Android editor will need to show the cancel button but we can make this an exceptional case when the window is not embedded.

This is really complicated, I don't remember i have ever seen the Cancel button before or pressing it 😄.

@WhalesState
Copy link
Contributor

Viewport::set_embedding_subwindows: Can't change "gui_embed_subwindows" while a child window is displayed. Consider hiding all child windows before changing this value.

We can disable gui_embedd_subwindows just before showing the ProgressDialog then we re-enable it after hiding the progress. This can be applied to the main editor window if no WindowWrappers are visible, but sadly they all can be visible but not focused 😭 and they will just block this hack. I'm running out of solutions now 😴.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 25, 2024

Another idea is to have two types of ProgressDialog: the old modal dialog and the "new" panel version. When progress occurs in a dialog, we could use the modal dialog version as before. The advantage of this approach is that we could reuse the old dialog in most cases except during startup and reimporting assets, which could also reduce potential side effects.

Does it mean having two classes? Sounds like lots of code duplication.

As WhalesState suggested, the solution could be to display the ProgressDialog within the dialog when a dialog is focused, rather than in the main window. I checked the codebase, and only a few dialogs use the ProgressDialog. One way to achieve this could be to add a check in EditorProgress::EditorProgress to determine if the current window is a dialog, and dynamically add a ProgressDialog control into the dialog at runtime, removing it in the EditorProgress::EditorProgress destructor.

tbh the dialog check is unnecessary, just reparent to current Window. Though it won't look good if the parent dialog is too small to show progress (but I think that can't normally happen).

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