Improve editor popups with multiple screens

[giarve]

Hopefully this PR will be able to fix multi-screen support.

Fixes #37320
Fixes #38591

[kuruk-mm]

I saw your comment on IRC, I was unable to fix two screen bugs because hardware limitation (I'm trapped on Brazil because Coronavirus, and only have my notebook with limited hardware).

Set the position and size with XSizeHints is obsolete (https://tronche.com/gui/x/xlib/ICC/client-to-window-manager/wm-normal-hints.html), to set the position.

A better aproach is with https://tronche.com/gui/x/xlib/window/configure.html
But I was unable to do that for my condition. If you need to talk something you can write here or send me a IRC Private message.

[HaSa1002]

Oh sorry. I didn't saw you are working on it...

[KoBeWi]

I tested this PR and #37320 doesn't happen anymore.

[giarve]

X11 comments:

Cannot continue due to window position checks returning (0,0) sometimes and sometimes not. I have tried combining XGetWindowAttributes with XQueryTree and XTranslateCoordinates but it doesn't fix the problem. It may be the window manager being insane, but I still don't know.
Having initial coordinates at (0,0) is a problem if the function Point2i DisplayServerX11::window_get_position(WindowID p_window) is called before getting the real ones so that function could be replace with one that recomputes the coordinates using a working method. That race could happen in DisplayServerX11::window_set_current_screen(int p_screen, WindowID p_window).

I have tried setting XConfigureWindow as @kuruk-mm (I hope you are well) suggested on the create window function but I didn't get different results. I will be pushing that structure so he won't have to make these changes.

To set a new screen for the window, the trick could be taking the current coordinates and subtracting the current screen offset (screen_get_position). Then getting the new screen offset and adding it to the previous subtraction. But we require the window position to be the real one.

X11 in conclusion: XGetWindowAttributes or XTranslateCoordinates sometimes returns incorrect positions when the editor is not in the default/primary screen. Watch the print_line() in _window_changed and the print_line() in _create_window while randomly clicking "Scene, Project, Debug, Editor and Help" dropdown menus in the editor. This must be fixed to allow moving windows within screens reliably.

Due to being a problem that a lot of people will encounter I have decided to simply adapt this PR to be merged so that it is not tried to be solved multiple times.
@HaSa1002 if you're able to test your PR after this one gets merged as I tried your fix but it didn't work on Linux so you will be able to merge your changes on the gui tooltip_popup (I didn't try the tooltip tho).

EDIT: This comment is outdated from the current changes (last push).

[bruvzg]

Some other places with global_position/global_transform used for pop-up:

godot/editor/animation_bezier_editor.cpp

Lines 654 to 667 in 6366332

	Vector2 popup_pos = get_global_transform().xform(mb->get_position());
	menu->clear();
	menu->add_icon_item(bezier_icon, TTR("Insert Key Here"), MENU_KEY_INSERT);
	if (selection.size()) {
	menu->add_separator();
	menu->add_icon_item(get_theme_icon("Duplicate", "EditorIcons"), TTR("Duplicate Selected Key(s)"), MENU_KEY_DUPLICATE);
	menu->add_separator();
	menu->add_icon_item(get_theme_icon("Remove", "EditorIcons"), TTR("Delete Selected Key(s)"), MENU_KEY_DELETE);
	}
	menu->set_as_minsize();
	menu->set_position(popup_pos);
	menu->popup();

godot/editor/connections_dialog.cpp

Line 828 in 6366332

Vector2 global_position = tree->get_global_position() + position;

godot/editor/dependency_editor.cpp

Line 281 in 6366332

file_options->set_position(owners->get_global_position() + p_pos);

godot/editor/editor_audio_buses.cpp

Lines 506 to 507 in 6366332

	effect_options->set_position(effects->get_global_position() + area.position + Vector2(0, area.size.y));
	effect_options->popup();

godot/editor/editor_audio_buses.cpp

Lines 561 to 562 in 6366332

	bus_popup->set_position(get_global_position() + pos);
	bus_popup->popup();

godot/editor/editor_file_dialog.cpp

Lines 605 to 606 in 6366332

	item_menu->set_position(item_list->get_global_position() + p_pos);
	item_menu->popup();

godot/editor/editor_file_dialog.cpp

Lines 627 to 628 in 6366332

	item_menu->set_position(item_list->get_global_position() + p_pos);
	item_menu->popup();

godot/editor/editor_node.cpp

Lines 4760 to 4761 in 6366332

	scene_tabs_context_menu->set_position(mb->get_global_position());
	scene_tabs_context_menu->popup();

godot/editor/filesystem_dock.cpp

Lines 2267 to 2268 in 6366332

	tree_popup->set_position(tree->get_global_position() + p_pos);
	tree_popup->popup();

godot/editor/filesystem_dock.cpp

Lines 2281 to 2282 in 6366332

	tree_popup->set_position(tree->get_global_position() + p_pos);
	tree_popup->popup();

godot/editor/filesystem_dock.cpp

Lines 2307 to 2308 in 6366332

	file_list_popup->set_position(files->get_global_position() + p_pos);
	file_list_popup->popup();

godot/editor/filesystem_dock.cpp

Lines 2326 to 2327 in 6366332

	file_list_popup->set_position(files->get_global_position() + p_pos);
	file_list_popup->popup();

godot/editor/project_settings_editor.cpp

Lines 628 to 634 in 6366332

	Point2 ofs = input_editor->get_global_position();
	Rect2 ir = input_editor->get_item_rect(ti);
	ir.position.y -= input_editor->get_scroll().y;
	ofs += ir.position + ir.size;
	ofs.x -= 100;
	popup_add->set_position(ofs);
	popup_add->popup();

godot/editor/debugger/script_editor_debugger.cpp

Lines 1396 to 1397 in 6366332

	item_menu->set_position(error_tree->get_global_position() + p_pos);
	item_menu->popup();

godot/editor/plugins/curve_editor_plugin.cpp

Line 121 in 6366332

open_context_menu(get_global_transform().xform(mpos));

godot/editor/plugins/visual_shader_editor_plugin.cpp

Line 1657 in 6366332

members_dialog->set_position(graph->get_global_position() + Point2(5 * EDSCALE, 65 * EDSCALE));

These callbacks arguments probably aren't in the screen coordinates either:

godot/editor/scene_tree_dock.cpp

Line 2387 in 6366332

void SceneTreeDock::_tree_rmb(const Vector2 &p_menu_pos) {

godot/editor/plugins/animation_blend_tree_editor_plugin.cpp

Line 452 in 6366332

void AnimationNodeBlendTreeEditor::_popup_request(const Vector2 &p_position) {

get_global_mouse_position() → get_screen_transform().xform(get_local_mouse_position():

godot/editor/editor_audio_buses.cpp

Lines 752 to 753 in 6366332

	delete_effect_popup->set_position(get_global_mouse_position());
	delete_effect_popup->popup();

And unlikely I have found all places.

[bruvzg]

Notes for the future improvemet:

• Special Fullscreen case probably should be ported to Windows and macOS as well.
• In case current window position (relative to the current screen) is bigger than target screen size, window will be moved to the wrong screen (not handled on any platform).

e.g. with the following display config, moving window to screen 1 will move it to screen 2 instead:

[giarve]

In X11, we're currently saving the coordinates in the _create_window() function to the rect provided as argument to the function but after making a XMoveResizeWindow() call. The problem of this is that we may not be able to set the first window position (the WM will probably place it somewhere else, like would happen with tiling WMs but it doesn't have to be tiled to ignore your placement). XCreateWindow() coordinates are sometimes ignored too. As a consequence, the saved coordinates may not be correct and up to date with the real position when making the window (this should only happen once).

I would recommend to use ConfigureNotify event as a source of truth for the real position of the window, which means that only _window_changed() function should modify and save coordinates of the window structure.

Having said that, XSync only makes sure that the command is sent, received and processed by the X server which means that it still has to give us the REAL coordinates back sending the client a ConfigureNotify event.

In conclusion, I have not found any other method to get real coordinates other than waiting for ConfigureNotify event and updating the saved coordinates accordingly (without using any type of transformation as coordinates should be global like happens in MacOS and should happen in Windows).

Future work could concentrate on waiting for the first ConfigureNotify event inside the _create_window() function or searching for another way to retrieve real coordinates (which I don't recommend if you're not an expert with X11).

EDIT: I have tried adding this:

static Bool _predicate_wait_position(Display *dpy, XEvent *event, XPointer win_arg) {
	return (event->xany.window == (Window)win_arg) && (event->type == ConfigureNotify);
}

XEvent xevent;
XIfEvent(x11_display, &xevent, _predicate_wait_position, (XPointer)wd.x11_window); // blocks until event is found
_window_changed(&xevent);

instead of setting coordinates manually when creating the window but no success, windows are warped to the corner somewhere and I have not checked if they're updated properly (it could still be that some of the popups are not properly fixed).

I propose to keep it as it is for now until it stops working for someone (if it ever does).

[Anutrix]

Requesting rebase.

[groud]

Sorry for the late review. It seems that we have now another PR to solve this problem: #52280. But it looks like this one might solve more situation. Would you mind a rebase?

This PR will likely need a review from @reduz, as I am not familiar enough with windows positioning.

[jmb462]

Changes from this PR have been included in the other PR except changes to:
scene/main/window.cpp related to set current screen
platform/linuxbsd/display_server_x11.cpp cause I don't understand this part

[giarve]

As this PR has 10 conflicting files (which would require me to download, compile and test the code after changes or patch the code from the open PRs only and add the code changes that have been left out) I would suggest merging #52280 and #42117.

#41395 mitigated tooltip positioning and with #40922 it is possible that X11 may work correctly after these two are merged.

However, if after merging #52280 and #42117, someone finds that in X11 windows (tooltips will be probably be already fixed) are still placed in the first screen and not in the screen where the editor (which is the parent window) is running, create a new PR adding the changes from the scene/main/window.cpp file from this PR.

If in X11 anyone still has problems after merging these two PR mentioned above, compile the engine with both the changes from scene/main/window.cpp and platform/linuxbsd/display_server_x11.cpp and if it works, create a new PR with only these changes.

Be aware that when this PR was made, the code here was tested on at least 2 KDE desktops and an Ubuntu one (Gnome or Unity, I don't remember), working fine (tiling WMs worked but the windows or tooltips ocuppied a big screen area and I don't know if they were supposed to).

[mhilbrunner]

Alright, closing as per the above comment. :)