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

Keep the editor help search dialog inside the screen #79814

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

Conversation

trollodel
Copy link
Contributor

@trollodel trollodel commented Jul 23, 2023

Fixes #79187 as described here

I can't test the conditions described in #79187, so testing appreciated.

@trollodel trollodel force-pushed the restrict_editor_help_search branch from 4a585e7 to 95de84e Compare July 23, 2023 09:32
@Sauermann
Copy link
Contributor

This might get made redundant by #79698 or #79617, both of which attempt to solve the inside-screen-problem for all windows and not just EditorHelpSearch.

@trollodel
Copy link
Contributor Author

trollodel commented Jul 23, 2023

I talked about that possibility here, but I didn't know that there are already PRs to fix it. Surely better than this PR.

@YuriSizov YuriSizov added this to the 4.2 milestone Jul 24, 2023
@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 24, 2023
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Nov 1, 2023
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 1, 2023
@YuriSizov
Copy link
Contributor

I don't entirely understand why having a different size would be a problem to begin with. Windows can exceed resolution bounds just fine. While the usability of such windows can be questioned, there shouldn't be a crash/freeze/other negative side effects. So that's one problem.

Another is why in case of #79187 the dialog appears on a wrong display to begin with. Was it placed there previously? Is it restored improperly? Do we also need to store the display index alongside the window rect? To be honest, I don't quite understand the steps from the OP and I don't have a second display to experiment myself. So if anyone could explain what's going on from the user perspective, I'd appreciate it.

What I can say for certain is that we indeed shouldn't fix this just for the editor help dialog. We should fix it for every dialog whose dialog_bounds we store and restore. We should unify the code responsible for restoring somewhere on EditorInterface and fix the issue there. I'm not entirely sure how a solution can be extended to Window itself, though, as the problems above do not appear that general to me. But if some helper method or setting can be added to Window, then by all means. I don't think it's #79617 though.

@trollodel
Copy link
Contributor Author

trollodel commented Dec 15, 2023

This is a general problem with multiple windows and multiple screens. Often, the editor code that manages window and size assumes implicitly that there is only one window and that the positioning and screen size doesn't change. This was guaranteed until 4.1.
This PR fixes some (but not all) scenarios when the user's screens layout changes, and only in one place. In the specific case, the bound goes out of screen because the screen was resized, but Godot doesn't know about that and tries to place in the old configuration.

What I can say for certain is that we indeed shouldn't fix this just for the editor help dialog. We should fix it for every dialog whose dialog_bounds we store and restore. We should unify the code responsible for restoring somewhere on EditorInterface and fix the issue there. I'm not entirely sure how a solution can be extended to Window itself, though, as the problems above do not appear that general to me.

This is true, code editors and docks do have a centralized and more complex fallbacks (in WindowWrapper) in case the screen layout changes, that can be made more generalized if desired.

Windows can exceed resolution bounds just fine.

No, it's not fine. If a resolution change put the window out of screen, and the window is modal, you can't recover your editor.

@YuriSizov
Copy link
Contributor

No, it's not fine.

Windows exceeding resolution in size is fine. Windows moving out of the visible space completely is not fine. I was referring to the former.

This PR fixes some (but not all) scenarios when the user's screens layout changes, and only in one place. In the specific case, the bound goes out of screen because the screen was resized, but Godot doesn't know about that and tries to place in the old configuration.

This doesn't explain a freeze that the user describes, since on their screenshots the help window is still visible and thus the editor is recoverable. Still, I think all restorable dialogs like that should be fixed together and anchored to their contextually relevant parent windows rather than arbitrary places on screen.

@akien-mga
Copy link
Member

Superseded by #79617.

@akien-mga akien-mga closed this Apr 30, 2024
@akien-mga akien-mga added archived and removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Apr 30, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Apr 30, 2024
@akien-mga
Copy link
Member

Superseded by #79617.

Or maybe not, based on #79187 (comment).

@akien-mga akien-mga reopened this Apr 30, 2024
@akien-mga akien-mga added this to the 4.x milestone Apr 30, 2024
@akien-mga akien-mga requested a review from bruvzg April 30, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detached script editor can become frozen with multiple, differing resolution monitors on Windows
5 participants