Expose getters for the currently focused Window

[Sauermann]

Make it easier to get the focused Window-node be exposing Viewport and DisplayServer functions, that return the focused window.
Currently it is necessary to traverse the scene-tree recursively in order to check every Node, if it is a Window, that has focus.

Here is a MRP to showcase how the exposed functions can be used to find the focused Control-node, that receives Key-InputEvents during the GUI-Input-stage.
GetFocusedControl.zip (updated 2023-07-16)

Press F1 to print the currently focused Control in the console.

resolve godotengine/godot-proposals#7242 by making it easier to find the focused Control-node, that receives Key-InputEvents during the GUI-Input-stage

Updated 2023-08-09: Fix merge conflict
Updated 2023-08-12: Fix merge conflict
Updated 2023-08-22: Fix merge conflict
Updated 2024-01-19: Fix merge conflict

[rsubtil]

The changes look good to me, and I can confirm this works well by applying this patch in my project!

[bruvzg]

When there's no popups, _get_focused_window_or_popup is using the last focused window, not the current focused window, so it won't work if non of the windows is focused. But this probably should be fixed in the _get_focused_window_or_popup implementations.

[Sauermann]

Thank you for this information. I didn't test this case.
I have updated the PR and reset the variable to INVALID_WINDOW_ID, when a window loses focus (on Linux, Windows and macOS)

Since this is a change in behavior, I would consider this PR now as experimental, especially because I can't test the implementation on macOS.

[Riteo]

I think that this is indeed a fine feature to have.

That said, I don't really like the name get_focused_window_or_popup because it doesn't completely reflect the return type.

The DisplayServer method returns a WindowID, so I would expect something closer to get_focused_window_id, which should also be more in line with the naming of the rest of DisplayServer's API.

Regarding Viewport, I think it would be better to remove the _or_popup suffix. It makes me feel like Popups are a completely unrelated class from Window and that the return type might even change, while it's the exact opposite (and the latter is not a feature of GDScript/C++).

[Sauermann]

The current implementation of get_focused_window_or_popup for DisplayServer does the following:

1. If there is a popup, then return the topmost Popup.
2. if there is no popup, then return the focused Window.

Instead of get_focused_window_id, get_focused_window_or_popup_id would be less ambiguous, because the function returns in some cases not the focused Window.

My understanding (might be wrong about that) is, that this was implemented, in order that Popups don't steal the focus of the main window for things like Menus, so that the main window appears in the Window Manager as still having focus.

For the embedded case, this PR currently just returns the focused window. From my recent experiences with bugs related to focused embedded windows, I believe it is necessary, to also track popup-windows in the embedded case, like the DisplayServer implementations do. So I would like to keep the Viewport-function-name in sync with the Display-Server function-name. If there is a better name available, I am happy to change it, but I am not convinced, that removing the _or_popup suffix would be beneficial.

[Riteo]

@Sauermann thanks for the explanation, the context explains a lot. I didn't understand from the name that the logic had actually a separate case for popups and I didn't know that this logic existed before internally (it did with the same name too) so I parsed the name in a different way, which is unfortunate, but it indeed makes sense.

I just guess that the "or" cuts the meaning of the sentence too cleanly, especially considering the order of operation. Perhaps a clearer, yet more verbose, name could be "get_top_popup_or_focused_window". No idea if it would fit with the API but eh, this is really a nitpick considering the rest of the windowing API with all of its unavoidable quirks.

I'm not sure why you want to keep the Viewport name in sync with DisplayServer, especially considering that they return different things. Normally I'd make the distinction. Is there an already established pattern or is there some other reason?

[Sauermann]

get_top_popup_or_focused_window sounds good to me.

I'm not sure why you want to keep the Viewport name in sync with DisplayServer, especially considering that they return different things

My long-term goal is to change the Viewport-implementation, so that it returns conceptionally the same as the DisplayServer-implementation.
I believe, that this is necessary, because currently there are several edge-cases which do not work correctly for focus of embedded Windows. (See for example #78398 or #78346)

[RandomShaper]

Could maybe the following alternative design work?:

• _get_focused_window_or_popup() is made public (with the underscore removed).
• A default, no-opish, implementation is added to DisplayServer.
  Con:
• Scritps have to call window_get_attached_instance_id() themselves on the returned WindowID.
  Pro:
• The various, almost identical get_top_popup_or_focused_window() can be removed.

Alternative 2:
_get_focused_window_or_popup() is made protected and virtual , with the same pro as the approach described above.

[Sauermann]

Combining the two approaches would look like this:

• _get_focused_window_or_popup is made virtual and also no-opish implemented in DisplayServer.
• get_top_popup_or_focused_window is only added to DisplayServer, but not to each of the plaform-specific implementations.

[RandomShaper]

Soudns good!

[Riteo]

@Sauermann

My long-term goal is to change the Viewport-implementation, so that it returns conceptionally the same as the DisplayServer-implementation.

Makes sense. IMO a refactoring is needed in the long term, especially for the need to account client-side decorations. I'd love to help after the Wayland mess settles down.

[trollodel]

I was working on #78672 (comment) using this PR, but it crashes on startup.

System info:
Godot v4.2.dev (ec62b8a + this PR) - KDE neon 5.27 22.04 - X11 - Vulkan (Forward+) - dedicated AMD Radeon Graphics (RADV NAVI23) () - AMD Ryzen 5 5600 6-Core Processor (12 Threads)

Stacktrace (using the OP's project):

[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7f562cc42520] (??:0)
[2] /usr/lib/x86_64-linux-gnu/libvulkan_radeon.so(+0x18703b) [0x7f5623f8703b] (??:0)
[3] VulkanContext::command_begin_label(VkCommandBuffer_T*, String, Color) (/godot/./core/templates/cowdata.h:415)
[4] RenderingDeviceVulkan::draw_command_begin_label(String, Color) (/godot/./core/templates/cowdata.h:415)
[5] RendererSceneRenderImplementation::RenderForwardClustered::_render_scene(RenderDataRD*, Color const&) (/godot/./core/templates/cowdata.h:415 (discriminator 3))
[6] RendererSceneRenderRD::render_scene(Ref<RenderSceneBuffers> const&, RendererSceneRender::CameraData const*, RendererSceneRender::CameraData const*, PagedArray<RenderGeometryInstance*> const&, PagedArray<RID> const&, PagedArray<RID> const&, PagedArray<RID> const&, PagedArray<RID> const&, PagedArray<RID> const&, PagedArray<RID> const&, RID, RID, RID, RID, RID, RID, int, float, RendererSceneRender::RenderShadowData const*, int, RendererSceneRender::RenderSDFGIData const*, int, RendererSceneRender::RenderSDFGIUpdateData const*, RenderingMethod::RenderInfo*) (/godot/./core/templates/paged_array.h:146)
[7] RendererSceneCull::_render_scene(RendererSceneRender::CameraData const*, Ref<RenderSceneBuffers> const&, RID, RID, unsigned int, RID, RID, RID, RID, int, float, bool, RenderingMethod::RenderInfo*) (/godot/servers/rendering/renderer_scene_cull.cpp:3343 (discriminator 4))
[8] RendererSceneCull::render_camera(Ref<RenderSceneBuffers> const&, RID, RID, RID, Vector2, unsigned int, float, RID, Ref<XRInterface>&, RenderingMethod::RenderInfo*) (/godot/./servers/rendering/renderer_scene_render.h:266)
[9] RendererViewport::_draw_3d(RendererViewport::Viewport*) (/godot/servers/rendering/renderer_viewport.cpp:244)
[10] RendererViewport::_draw_viewport(RendererViewport::Viewport*) (/godot/servers/rendering/renderer_viewport.cpp:309)
[11] RendererViewport::draw_viewports() (/godot/servers/rendering/renderer_viewport.cpp:759 (discriminator 4))
[12] RenderingServerDefault::_draw(bool, double) (/godot/servers/rendering/rendering_server_default.cpp:92)
[13] Main::iteration() (/godot/main/main.cpp:3589)
[14] ProgressDialog::task_step(String const&, String const&, int, bool) (/godot/editor/progress_dialog.cpp:212)
[15] EditorFileSystem::reimport_files(Vector<String> const&) (/godot/./core/templates/cowdata.h:415)
[16] EditorFileSystem::_update_scan_actions() (/godot/editor/editor_file_system.cpp:689)
[17] EditorFileSystem::_notification(int) (/godot/editor/editor_file_system.cpp:1621)
[18] Object::notification(int, bool) (/godot/core/object/object.cpp:832)
[19] SceneTree::_process_group(SceneTree::ProcessGroup*, bool) (/godot/scene/main/scene_tree.cpp:951)
[20] SceneTree::_process(bool) (/godot/scene/main/scene_tree.cpp:1028)
[21] SceneTree::process(double) (/godot/scene/main/scene_tree.cpp:510)
[22] Main::iteration() (/godot/main/main.cpp:3576)
[23] OS_LinuxBSD::run() (/godot/platform/linuxbsd/os_linuxbsd.cpp:933)
[24] /godot/bin/godot.linuxbsd.editor.dev.x86_64(main+0x13e) [0x564bbe05cfbe] (/godot/platform/linuxbsd/godot_linuxbsd.cpp:74)
[25] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f562cc29d90] (??:0)
[26] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f562cc29e40] (??:0)
[27] /godot/bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x564bbe068465] (??:?)

EDIT: It was a false alarm (sorry for that), I was fooled by scons.

[Sauermann]

Thanks for the report. I find it noticeable, that the stacktrace doesn't contain any files, that were changed by this PR.