PopupMenu rework and enhancements.

[EricEzaM]

Initially inspired by this reddit post, which never got it's own issue submitted. This PR resolves a few existing issues with PopupMenu's which did have issue tickets, but also a few others which I found when making this PR.

• Popup menu size can now be limited, and content will scroll. Previously, if the popup menu had a lot of items, it would take up the entire window height and then scroll from there. Now, the size of popup menus can be limited by using get_popup()->set_max_size() (in C++) or get_popup().max_size (in GDScript). Of course, this works both in editor and in projects.
• This can be selectively used around the editor to help resolve ui issues such as Popup window to change language(and other similar too) should show first entry around mouse #22404. Examples in-editor shown below.
  
• Added margin container which improves consistency of look, as well as makes item positioning code simpler and easier to understand.
• Now uses a scroll container so users know it can be scrolled. This also allows the user to scroll without a scroll wheel by clicking and dragging the scrollbar. Previously users needed a scroll wheel to be able to scroll. (Closes Make possible to navigate overflowed 'PopupMenu's without a scroll wheel #29111)
  
• Changing item selection with arrow keys now scrolls to follow selected item, whereas previously it did not. (resolves this comment by @KoBeWi, which did not have a separate issue: Make possible to navigate overflowed 'PopupMenu's without a scroll wheel #29111 (comment))
• Scrolling with arrow keys stops at first or last item when scrolling up and down, respectively. Previously it looped around and kept on going.
  
• Fixed submenu autohide areas to match exactly the hover area. Previously they were slightly off. Also fixed selection of which item was hovered, as that was also slightly off. Previously the darker area which represented which item was hovered did not accurately represent the size of the actual item. As you can see, submenu's are correctly placed when scrolling is used. Note: I am not recommending that the project menu is made to scroll, this was only for submenu testing purposes.
  
• When search is enabled, selected item from search query will be scrolled to and highlighted. In the below case I type "obj" and then "res".
  
• Reduced code duplication by moving query of item icon size to a member function in the Item struct.
• General code clean up - comments where needed, and rewording variables to be more clear about what they are.

Bugsquad edit: Also fixes #41564.

[KoBeWi]

Would be cool if you added hysteresis too 🙃
I have an unfinished PR that attempted to implement it: #35487

[EricEzaM]

@KoBeWi That's a good feature, I'll see if I can add it in this PR. I did a search for PopupMenu issues and that one didnt come up :P

[groud]

It looks great ! :) I'll have a look to the code.

[EricEzaM]

@KoBeWi I have made some progress on adding that, but if I complete it fully I will put it in a separate PR so it can be reviewed separately from this PR, and not too many features/changes get muddled up in one PR.

[groud]

The code looks good to me, but i haven't tested locally.

[KoBeWi]

I did some testing and I noticed that some popups have maximum size that don't really make sense

Looks like it's calculated wrongly or something.

Also the new max_size property needs documentation. (I now noticed it comes from Window)

Other than that, all seems to work fine.

[EricEzaM]

Hmm yeah it looks like the height of the contents is calculated with an extra very small value, so the contents has to scroll a tiny bit. Will investigate in the morning

[EricEzaM]

Looks like there are a number of issues to resolve...

• 1. (This existed before this PR) Popups have height set to 100px even if they do not need it. See File -> Convert To.... The weird thing is that this doesn't seem to apply to all menus... See File -> Open Recent (when the list is empty), which does not have the same issue. Edit: Resolved

• 2. The issue Kobewi pointed out above, plus: the scroll bars do not focus automatically on what seems to be only these popup menus. When you hover the bar it does not highlight. Additionally, when you do click on the scrollbar then is highlights and you can click and scroll. Also, if you close it in it's highlighted state, it will stay that way when it comes up again. So it seems like the scrollbar is frozen to input or something? very weird. Edit: Resolved

• 3. When a max size is not set, and the popup menu minimum size returns something higher than the screen height, no scrollbar is visible and you cannot scroll. Edit: Resolved now.

[EricEzaM]

@KoBeWi ok, all done. I found some more stuff as in my previous comment which is now resolved.

The issue you found was really weird... It seems like the setting of the margin was not occurring properly on the margin container. So, the margins were 10px, but the content thought it was only 8px (which is what it should be). Therefore there was a 4px scroll (10px*2 - 8px*2). Anyway, moving the setting of the margins away from notification enter tree is what worked.

@groud if you care to re-review, the changes of the most recent commit (which were squashed into one) were:

popup.cpp

• Added check which ensures popup is not larger than parent, and also does an early exit is max size is not set (Lines 129 to 141). This is part of what solved #3 above

popup_menu.cpp

• Limited PopupMenu height to usable parent rect height. (lines 104 to 108) - this fixed #3 above
• Used set_as_minsize() to 'shrink' the borders of the submenu (line 200) - this is what fixed #1 above. Also did this in the PopupMenu::Popup() method, in case it is not a submenu. BTW the issue did not occur on "Recent Scripts" submenu since the code that opens that menu actually calls set_as_minsize before it opens.
• Moved setting of margin overrides for the margin container from ENTER_TREE to VISIBILITY_CHANGED (and after is_visible() check) - fixed Kobewi's issue and #2 above

[EricEzaM]

As a bonus, this PR also fixes the issue where popup menus were going off screen:

After:

[KoBeWi]

Looks fine now after brief testing.

[pouleyKetchoupp]

Great work! I've just commented on some details.

I can also confirm the latest changes fix #41564.

[akien-mga]

Thanks! Amazing work :)

[groud]

Awesome, thanks a lot !

[chiguireitor]

will this be backported to the v3 branch?

[jacksontriffon]

Is this obselete?

[Calinou]

Is this obselete?

This pull request was merged in the master branch (for 4.0.alpha), but it won't be cherry-picked to 3.x as it introduces compatibility-breaking changes.

It may be possibly to selectively backport features from this PR to 3.x, but someone needs to step up to do the required work 🙂

[rsubtil]

I'm interested in backporting these changes, but what are the mentioned "breaking changes"? And what features should I avoid backporting?

[EricEzaM]

I'm not too sure. @Calinou is it breaking change because of the fact that it changes the way the popup looks/behaves?

I don't think there was any breaking api changes by looking at the code. If you like, you can try backporting it and see how you go. Working through it will probably make any breaking changes more obvious.