Fix that popup_centered didn't take the window's size into consideration

[Sauermann]

resolve #72949 (only tested it on linux)
alternative to #72954
regression from #62179

[akien-mga]

Thanks!

[Rindbee]

This PR may reopen #55894.

And why in that case, set the size first and then popup_centered()? why not call popup_centered() directly with the given size?

Perhaps the old size should be considered when calling popup_centered() with no arguments.

[Sauermann]

You are correct, that this PR has reopened #55894 on Windows.
So there are two different expectations of how popup_centered should work:

#55894: don't take the previous size into consideration
#72949: do take the previous size into consideration

We would need to decide on which variant and adjust the according places, that use popup_centered.
Or add an additional parameter like popup_centered(const Size2i &p_minsize, bool p_consider_previous_size).

[akien-mga]

I don't see what the use case would be for not taking the size into account? This would mean popping up centered with an unknown size, then setting the size, and then it's no longer centered.

[Rindbee]

I don't see what the use case would be for not taking the size into account? This would mean popping up centered with an unknown size, then setting the size, and then it's no longer centered.

Will popup at the smallest size when regardless of size. It is to consider the situation similar to #55894 to reuse the same popup window to display different content. #61982 and #62179 were created around the same time.

[Sauermann]

to reuse the same popup window to display different content

this use-case could be handled by setting the size before the popup_centered call.

How about the following approach?

• popup_centered() no longer takes any parameter and adjusts only the position and makes the popup visible.

I have the impression, that popup_centered does too much in the background. Perhaps it would be better to make this more explicit?

With Window::get_clamped_minimum_size, Window::get_contents_minimum_size, Window::get_contents_maximum_size there are functions available to calculate the desired size.

[Rindbee]

Well, I think the problem may be caused by _get_contents_minimum_size(). In #62179, as before, popup_centered() does not use size directly. But, I removed part of code inAcceptDialog::_get_contents_minimum_size().

[Rindbee]

I checked #62179. For the situation in #72949, the difference between before and after is mainly in Window::get_clamped_minimum_size(), that is, VisualShaderEditor::preview_window is expected to enable wrap_controls, but in fact it does not, resulting in the use of min_size instead of the return value of Window::_get_contents_minimum_size().

godot/scene/main/window.cpp

Lines 1557 to 1563 in 929ee61

	Size2 Window::get_clamped_minimum_size() const {
	if (!wrap_controls) {
	return min_size;
	}
	return min_size.max(get_contents_minimum_size());
	}

It is also a special case that Window::_get_contents_minimum_size() of preview_window can be changed with Window::size.

godot/editor/plugins/visual_shader_editor_plugin.cpp

Lines 4955 to 4957 in 929ee61

	void VisualShaderEditor::_preview_size_changed() {
	preview_vbox->set_custom_minimum_size(preview_window->get_size());
	}

But if wrap_controls is enabled there, there may be problems when resizing.

[YuriSizov]

• popup_centered() no longer takes any parameter and adjusts only the position and makes the popup visible.

There is no meaningful change in this other than inconveniencing users. There are 2 behaviors observed by users: popup at a fixed size, and popup at the previous size. For the former, the size can be passed to the method directly, even if it will simply be a shortcut for set_size + popup. For the latter, the size can be ignored, and then the method should rely on the existing value. If we need to do a sanity check before popping the window, it can be performed in either case in between setting the size (or taking the existing size) and doing the rest of the popup logic.

Same is true for any other popup method that doesn't rely on a ratio instead.

[Rindbee]

Same is true for any other popup method that doesn't rely on a ratio instead.

That is, the popup_center* called with the default size (0,0) will consider the current size value? Equivalent to calling popup_center*(get_size(), ...)?

[YuriSizov]

Yes, I think this makes the most sense. If you want the min size, you can pass it explicitly. Which is in fact what we normally do with these popups that are shared between different views.