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 #99844

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 29, 2024

Same as #97009, but I made the dialog appear on currently focused window.

Fixes #96994
Fixes #90944

@JekSun97
Copy link
Contributor

I sometimes see window focus issues when opening a project, is it possible to carry this over to importing a project?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 29, 2024

Yes, it's already included.

@KoBeWi KoBeWi force-pushed the give_back_the_focus branch from 36bc47c to 3759100 Compare November 30, 2024 11:18
@KoBeWi KoBeWi force-pushed the give_back_the_focus branch 2 times, most recently from 839ea69 to 8d2778c Compare December 7, 2024 18:53
@Hilderin
Copy link
Contributor

Hilderin commented Jan 7, 2025

I'm using this PR for the last couple of days and it works great so far. It's very nice to be able to import assets in the background without Godot stealing the focus.

The only issue so far is the "Export Project..." button in the Export window. I need to do it twice for the exportation to run. Also, if I press only one time, select the file and close the window, the next time the Export window is opened, the exportation is executed.

@KoBeWi KoBeWi force-pushed the give_back_the_focus branch from 8d2778c to 215984c Compare January 7, 2025 23:10
@KoBeWi KoBeWi requested a review from a team as a code owner January 7, 2025 23:10
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 7, 2025

Should be fixed now, although I had to modify EditorFileDialog to emit file_selected before hiding. This might break some code.

@Hilderin
Copy link
Contributor

Hilderin commented Jan 8, 2025

Should be fixed now, although I had to modify EditorFileDialog to emit file_selected before hiding. This might break some code.

I'm really worry about this modification, that means that the editor file dialog will still be opened when saving a scene or when opening almost anything in the editor.

Just to be sure, I reverted locally the modifications in ProjectExportDialog and everything seems to work fine. What was the problem you encountered in this particular popup exactly?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 8, 2025

The file dialog did not return focus to export dialog and so the progress was appearing above the hidden file dialog, instead of the export dialog. Tested on Windows.

@KoBeWi KoBeWi force-pushed the give_back_the_focus branch from 215984c to 6fe259b Compare January 8, 2025 15:15
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 8, 2025

Ok I reverted the EditorExport/FileDialog changes. The issue I had before doesn't seem to be happening anymore.

Comment on lines +343 to +354
void EditorNode::input(const Ref<InputEvent> &p_event) {
// EditorNode::get_singleton()->set_process_input is set to true in ProgressDialog
// only when the progress dialog is visible.
// We need to discard all key events to disable all shortcuts while the progress
// dialog is displayed, simulating an exclusive popup. Mouse events are
// captured by a full-screen container in front of the EditorNode in ProgressDialog,
// allowing interaction with the actual dialog where a Cancel button may be visible.
Ref<InputEventKey> k = p_event;
if (k.is_valid()) {
get_tree()->get_root()->set_input_as_handled();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How does this work if you're trying to press the Cancel button of ProgressDialog with key events?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like you can't.
However canceling was broken before (at least during export: #93481) and now you can cancel with mouse, so it's still an improvement.

Copy link
Contributor

@Hilderin Hilderin Jan 8, 2025

Choose a reason for hiding this comment

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

It's working when the ProgressDialog is not in another popup because the progress dialog is parented to the root and not the EditorNode. I guess a possible fix could be to accept space and enter key in EditorNode::input. The set_input_as_handled is really only used for shortcuts.

@akien-mga akien-mga requested a review from bruvzg January 8, 2025 17:03
@Hilderin
Copy link
Contributor

Hilderin commented Jan 9, 2025

I detected another wierd side effect. When a sub scene is used in a main scene and both scenes are opened, when switching back to the main scene, the progress "Updating scene" appears and after that the first right click displays and context menu of the scene tab. My guess is that get_tree()->get_root()->set_input_as_handled(); causes the focus out of the scene tab from being triggered.
I tested on Godot master without this PR and I'm not able to reproduce.

godot.windows.editor.x86_64_wcKb0h8kXG.mp4

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 9, 2025

No idea what causes it. Hiding the dialog and removing all input/processing overrides does not fix the issue. It's like it's caused by the existence of the dialog alone.

@Hilderin
Copy link
Contributor

The issue is caused because Viewport::_gui_input_event does not process the button release and gui.mouse_focus is never reset to null.
I'm not sure why, but when I comment the line DisplayServer::get_singleton()->process_events(); in ProgressDialog::_update_ui is issue is fixed. Is that line still needed? I'm not sure but this line seems to drop events.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 10, 2025

It's part of the code that manually processes SceneTree. It's probably supposed to keep the dialog responsive and allow to cancel during processing.
...Except it doesn't work. I don't know what changed, but #93481 stays unresolved (it was being fixed in my previous tests).

I'll just remove this line.

@KoBeWi KoBeWi force-pushed the give_back_the_focus branch from 6fe259b to d0fe15e Compare January 10, 2025 12:46
@Hilderin
Copy link
Contributor

Nice, it fixes the issue. I'll continue working in Godot using this PR to see if I spot other side effects.

@Hilderin
Copy link
Contributor

Unfortunately, the last modification brings a "semi-bad" side effect. The editor window is unresponsive while the progress dialog is displayed. I retested after readding the DisplayServer::get_singleton()->process_events(); and the editor is responsive with the line.
image

@Hilderin
Copy link
Contributor

Ok, I found a workaround... The issue is that gui.mouse_focus is still set in the ViewPort. I saw that there's a private method: Viewport::_drop_mouse_focus() that resets it.
And Viewport::_drop_mouse_focus() is called in set_disable_input.

So adding these lines in ProgressDialog::_popup fixes the issue of the right click after the save of a used scene and changing to the main scene:

...
reparent(current_window);

bool window_is_input_disabled = current_window->is_input_disabled();
current_window->set_disable_input(!window_is_input_disabled);
current_window->set_disable_input(window_is_input_disabled);

show();

Note that there's probably a more elegant way to fix the issue but it works.

Co-authored-by: Hilderin <81109165+Hilderin@users.noreply.github.com>
@KoBeWi KoBeWi force-pushed the give_back_the_focus branch from d0fe15e to 77d18d1 Compare January 14, 2025 00:37
@akien-mga akien-mga merged commit 8cf6061 into godotengine:master Jan 14, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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