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

Don't use saved editor dialog size in single-window-mode. #75141

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Mar 20, 2023

Fixes #75084.

If single_window_mode is enabled, trying to place editor dialogs with a saved size
often looks wrong if the main editor window has been resized.
This can result in windows being partially (or mostly) off-screen,
or just off-centered.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 20, 2023

It might be nice to re-position the dialog if the parent window size changes, but I checked godot3 and it doesn't do that either, so at least that part isn't a regression.

@rcorre rcorre changed the title Don't use saved size in single-window-mode. Don't use saved editor dialog size in single-window-mode. Mar 20, 2023
@YuriSizov YuriSizov added this to the 4.x milestone Mar 20, 2023
@YuriSizov
Copy link
Contributor

I don't think this improves the situation meaningfully. If a dialog has a big enough size by default, it will still slide off of the screen. I think we should address this properly, instead of restoring a behavior to be similar to 3.x.

Dialogs need to respect the size of their parent window when embedded, and treat it as their max size. This should likely be an option, not a default behavior, as the opposite may still be useful in games and apps made with Godot.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 21, 2023

Thanks for the feedback @YuriSizov. I agree this isn't ideal, but I'm not quite following you. Are you saying that if popup is called with bounds that do not fit within the parent, it should adjust the bounds so they do? That is, we'd either (A) clamp the offending bounds or (B) shift the bounds?

example

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 22, 2023

I think you follow me quite well :) IMO we should try both. If the size is within the parent's size, just slide it over. If it's bigger, then resize it to fit.

But again, this should probably be a flag on the Window node, and not the default behavior. We need it for the editor, but it may still be desirable for other projects to spawn windows out of frame.

Or it can be a new method that takes a rect2 and fits it into the Window's area. So you can use this feature dynamically instead, whenever you need to.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 23, 2023

Aha, it looks like my issue was a dupe of #37792.
8be16e0 added the necessary setting, but it wasn't actually set on any of the editor dialogs.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 23, 2023

NVM. 8be16e0 was just about being able to drag windows outside the editor bounds. clamp_to_embedder only applies to drag events. Should clamp_to_embedder also work on popup and parent window resize events, or should that be a separate setting?

@YuriSizov
Copy link
Contributor

I think it should apply here, yes. If you can't drag a window outside, then I think it's safe to assume you don't want to spawn it outside either.

@rcorre rcorre requested a review from a team as a code owner March 25, 2023 01:15
@rcorre
Copy link
Contributor Author

rcorre commented Mar 25, 2023

I updated it so windows that are clamped to their embedder are now automatically shifted/resized on popup, and when the parent is resized.

In the recording, there's a lag when I resize the parent. That only happens when I'm recording, so I'm guessing it's something weird with X11 and SSR? It seems reponsive when I'm not recording the screen.

ssr-2023-03-24_21.12.37.mp4

@YuriSizov YuriSizov requested review from a team and removed request for a team March 27, 2023 10:35
@YuriSizov YuriSizov modified the milestones: 4.x, 4.1 Mar 27, 2023
scene/main/viewport.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

This looks great to me! Will wait a bit for another review, but this should be good to merge.

@YuriSizov YuriSizov requested a review from a team March 27, 2023 10:42
Fixes godotengine#75084.

The clamp_to_embedder setting was added in 8be16e0,
but was not set on any of the in-editor dialogs.

This patch sets `clamp_to_embedder` on editor dialogs so they cannot be dragged out of the frame.
This also modifies `clamp_to_embedder` so a window is clamped to the bounds of an embedder when
it pops up and when the parent is resized.
@YuriSizov YuriSizov merged commit f818d2b into godotengine:master Mar 28, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@rcorre rcorre deleted the rrc/dialog-pos branch August 20, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants