Fix popup positioning with multiple monitors #37528
Conversation
|
Could you amend the commit log to be more explicit? "Fixed popups" doesn't tell us much. See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-logs-with-readability-in-mind |
HaSa1002
force-pushed
the
fix_windows_wrong_set_screen
branch
2 times, most recently
from
685b61d to
7779bd4
Compare
Done |
HaSa1002
force-pushed
the
fix_windows_wrong_set_screen
branch
from
7779bd4 to
5e290c6
Compare
HaSa1002
force-pushed
the
fix_windows_wrong_set_screen
branch
from
5e290c6 to
d0da44d
Compare
HaSa1002
changed the title
HaSa1002
force-pushed
the
fix_windows_wrong_set_screen
branch
from
d0da44d to
a524c10
Compare
There was a problem hiding this comment.
Setting current_screen in the Window::popup_* variations is arguably redundant as they all call Window::popup which sets it regardless.
I approve of removing the positioning logic from Viewport::_gui_show_tooltip, as that's already handled by Popup which TooltipPanel inherits from.
| if (!is_embedded()) { | ||
| current_screen = DisplayServer::get_singleton()->window_get_current_screen(get_parent_visible_window()->get_window_id()); | ||
| } |
There was a problem hiding this comment.
current_screen is already set in this method a little further down. Is there any reason to set it here again?
There was a problem hiding this comment.
Sorry, but where? I may be blind, but I don't see a current_screen in this method
There was a problem hiding this comment.
oh I see. Well I fix the logic below then
There was a problem hiding this comment.
There was a problem hiding this comment.
Now current_screen gets set right before the popup position is changed, which might result in current_window being set, only for the window to be immediately moved to another screen.
Looking at the DisplayServer implementations, it seems that whenever a window's bounds are changed, the window gets notified of that through Window::_rect_changed_callback which should arguably be the place where the window's current_screen should actually be updated. This would make the value of current_screen more consistent and also allow get_current_screen to use the cached value instead of querying the DisplayServer every time.
There was a problem hiding this comment.
I tested the following cases:
- current_screen set as in master (in popup) -> windows, like add node, will always beeing placed on main monitor
- current_screen queried from current_screen in popup -> windows and popups open on the monitor of the active parent window
- not setting current_screen in popup (only on resize) -> all windows and popups open on first screen. _rect_changed_callback does not provides desired effect, since the current screen is set and the position is updated based on the current_screen. Therefore setting it once in popup with the screen of the active parent window appears to be the only correct fix.
I want to emphasize this last point. You want your windows in your application on the same screen as the window you came from. The application should not just position it where it feels it is useful (i.e. the main screen)
There was a problem hiding this comment.
Thinking about it a little more, changing current_screen only in _rect_changed_callback would also create the problem of it only being updated after the DisplayServer processes its incoming events, which might be considered a weird behavior to the user who might expect current_screen to be updated as soon they call set_position(), so perhaps that shouldn't be the only place current_screen is set.
That said, are you sure that this isn't fixed by #40922 (which got merged today) or #37506 (which isn't merged yet)? As far as I can tell, Viewport::_gui_show_tooltip nor Window::popup and the methods it calls don't rely on current_screen at any point, unless I'm missing something. How does setting current_screen fix this issue?
HaSa1002
force-pushed
the
fix_windows_wrong_set_screen
branch
2 times, most recently
from
95cb1b5 to
3ae6f30
Compare
Chaosus
force-pushed
the
fix_windows_wrong_set_screen
branch
from
3ae6f30 to
4f180de
Compare
|
Fixed conflicts & rebased. |
HaSa1002
force-pushed
the
fix_windows_wrong_set_screen
branch
from
4f180de to
23ad4a6
Compare
|
Rebased again. PR can be closed. However, we could strip out all code, except for the removal of the custom popup code and use the default popup code instead. |
Alright then, closing in favor of #37506.
If this is desired, we can either update and re-open this PR, or open a new one. In the meantime, this seems to be at least partially superseded and at least partially abandoned, so we can close now and re-assess later if needed. |
fixes #37320
I missed #37506...