Improve height limit of PopupMenu of OptionButton/MenuButton

[floppyhammer]

Fixes #46545. The issue has been there for a while (since I started using Godot 4.0 actually). I solved the issue by adding an additional property extra_height_limit to PopupMenu (not exposed to users). I don't know if this is acceptable or if there are more appropriate solutions, so any suggestions are welcome.

Result:

Along the way I made this fix, I found another bug related to PopupMenu in Project Settings and Editor Settings when Single Window Mode is enabled. But that isn't caused by this PR.

[Riteo]

Along the way I made this fix, I found another bug related to PopupMenu in Project Settings and Editor Settings when Single Window Mode is enabled. But that isn't caused by this PR.

I think you're talking about #40227.

[floppyhammer]

@Riteo Yeah, you're right.

[Riteo]

Also, I think that there's no need for an extra property. After all, you have to compute its value still one way or the other. IMO we should change the way height_limit is computed in the first place, although I can't investigate right now.

[EricEzaM]

I don't really like the name "set extra height limit". For someone coming to this code in 1 year's time, they would need to look into where the extra_height_limit property was used to understand what it actually did.

"Height Limit" implies that if it was set to say, 50, the maximum height of the popup would be 50. But that is not what this does. This extra_height_limit is subtracted from the actual height limit (usable parent rect). Essentially what it does is set a maximum Y position (highest allowable point) for the popup menu.

As a result, perhaps changing the name to something like set_highest_allowable_point(int p_value) would represent it better. I still don't think that set_highest_allowable_point is necessarily the best name, but I just want to get the idea of a name change out there.

[floppyhammer]

Agree.

[EricEzaM]

If you put the MenuButton down really low on the screen, wouldn't it essentially make the popup miniscule and unusable?

[floppyhammer]

@EricEzaM We can make the menu go above the button in that case, just like what the auto-completion panel does in the script editor.

[floppyhammer]

@EricEzaM Or we leave height_limit as it is, and just make PopupMenu not respond to the mouse release too fast after the mouse press just activates it as it does in 3.2.

[floppyhammer]

@Riteo I also thought of changing how height_limit is calculated. However, that happens inside PopupMenu, so accessing the position of its parent (OptionButton or MenuButton) cannot be done in a clean way. But you are right, adding an extra property for this is not very desirable. I will think of a more reasonable way.

[floppyhammer]

I just realized adding one line of code height_limit -= get_position().y; is enough. Absolutely no need for a new property. Though the scenario mentioned by @EricEzaM in which OptionButton/MenuButton goes down really low on the screen still causes problems. That needs more consideration.

[Riteo]

@floppyhammer yeah I agree. It was discussed before (see #45201 (comment)) and I think that the popup should just try to fit the window the best it can before having to resize.

[floppyhammer]

[Riteo]

@floppyhammer you requested a review but the latest commit is 6 days old. I think you forgot to push

[floppyhammer]

@Riteo It says I pushed yesterday.

[Riteo]

@floppyhammer oh now it does. Since I looked at this PR from octodroid it probably messed up something on my side, sorry 😅

[KoBeWi]

MenuButton pressed and OptionButton pressed have lots of popup-related duplicate code that could probably be moved to Popup Menu class as a new method.

[floppyhammer]

@KoBeWi I agree. Should we do it in this PR or leave it to another one?

[KoBeWi]

Refactoring could be done in another PR.

[floppyhammer]

godot/scene/main/window.cpp

Lines 1095 to 1108 in c334989

	Rect2i Window::get_usable_parent_rect() const {
	ERR_FAIL_COND_V(!is_inside_tree(), Rect2());
	Rect2i parent;
	if (is_embedded()) {
	parent = _get_embedder()->get_visible_rect();
	} else {
	const Window *w = is_visible() ? this : get_parent_visible_window();
	//find a parent that can contain us
	ERR_FAIL_COND_V(!w, Rect2());
	parent = DisplayServer::get_singleton()->screen_get_usable_rect(DisplayServer::get_singleton()->window_get_current_screen(w->get_window_id()));
	}
	return parent;
	}

Is it designed to return the screen rect when calling Window::get_usable_parent_rect()? Isn't it more intuitive to return the parent window rect by the method's name?

[floppyhammer]

This PR has been devised to fix the menu item selection issue #46545. But since it has already been fixed, this PR seems quite unnecessary. Besides, it's flawed from the beginning anyway.