Skip to content

Fix OptionButton PopupMenu not shrinking after item changes#114806

Merged
akien-mga merged 1 commit intogodotengine:masterfrom
Clubhouse1661:fix-option-button-popup-shrinking
Jan 10, 2026
Merged

Fix OptionButton PopupMenu not shrinking after item changes#114806
akien-mga merged 1 commit intogodotengine:masterfrom
Clubhouse1661:fix-option-button-popup-shrinking

Conversation

@Clubhouse1661
Copy link
Contributor

Fixes #114786.

Updates OptionButton::show_popup() to correctly handle PopupMenu resizing when content changes from larger to smaller items.

  • Calls popup->set_min_size(Size2(0, 0)) before opening. This is crucial because Window::popup(rect) clamps the size to the current min_size. Without resetting it, the window refuses to shrink to the new smaller content size because it's still constrained by the previous items' minimum size.
  • Explicitly enables popup->set_shrink_height(true) to ensure the height is recalculated to fit the new content.

Verified with the MRP from the issue. The PopupMenu now immediately shrinks to the correct size when switching from long items to short items.

Before_bug.mp4
After_bug.mp4

Copilot AI review requested due to automatic review settings January 9, 2026 19:19
@Clubhouse1661 Clubhouse1661 requested a review from a team as a code owner January 9, 2026 19:19

This comment was marked as spam.

@AThousandShips
Copy link
Member

Please turn off the copilot feature for contributing to Godot, it creates a lot of noise (and uses CI resources) and risks including bad suggestions that you then have to fix because it didn't understand how we write code and do things in the engine

@akien-mga
Copy link
Member

See also #114760.

CC @bruvzg @YeldhamDev

@Clubhouse1661
Copy link
Contributor Author

Understood, I've disabled the Copilot auto-review feature.

@YeldhamDev
Copy link
Member

@akien-mga This PR doesn't affect the bug I fixed, because it doesn't use an OptionButton. 😉

In my PR however, I moved popup->set_shrink_width(false) line to the constructor, as it doesn't make sense to set it every time it pops up. The same could be done here instead.

@Clubhouse1661 Clubhouse1661 force-pushed the fix-option-button-popup-shrinking branch from ce46750 to dfc945e Compare January 9, 2026 20:07
@Clubhouse1661
Copy link
Contributor Author

Agreed. Updated to follow @YeldhamDev's suggestion from #114760:

Moved popup->set_shrink_width(false) from show_popup() to the constructor since it's a one-time property

@YeldhamDev
Copy link
Member

YeldhamDev commented Jan 9, 2026

popup->set_shrink_height(true) should follow suit (unless the fix needs it to be called every popup).

@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2026

popup->set_shrink_height(true)

This is default, so not needed at all.

Fixes godotengine#114786. Reset min_size before popup to prevent clamping to old size. Move shrink_width to constructor following PR godotengine#114760 style.
@Clubhouse1661 Clubhouse1661 force-pushed the fix-option-button-popup-shrinking branch from dfc945e to 72ed25e Compare January 9, 2026 20:37
@Clubhouse1661
Copy link
Contributor Author

That's right. Removed set_shrink_height(true) as it's the default.
The fix now only adds set_min_size(Size2(0, 0)) in show_popup(), which is a cleaner fix.
Tested with the MRP, and it works correctly.

@scgm0
Copy link
Contributor

scgm0 commented Jan 10, 2026

I believe the actual fix should be to remove popup->set_shrink_width(false) rather than manually calling popup->set_min_size(Size2(0, 0)). When shrink_width is true, PopupMenu automatically shrinks its width in _pre_popup().
So why was popup->set_shrink_width(false) added in the first place?(#114438)
@bruvzg

@bruvzg
Copy link
Member

bruvzg commented Jan 10, 2026

So why was popup->set_shrink_width(false) added in the first place?

Because, it should not shrink width to anything smaller than the size of the OptionButton only expand.

@bruvzg
Copy link
Member

bruvzg commented Jan 10, 2026

For 4.7 we probably should revert #104399, #112604 and #114438 and fully reconsider how popup auto sizing works.

@scgm0
Copy link
Contributor

scgm0 commented Jan 10, 2026

For 4.7 we probably should revert #104399, #112604 and #114438 and fully reconsider how popup auto sizing works.

Perhaps we could have _pre_popup() accept the screen_rect from popup(const Rect2i &p_screen_rect), and if screen_rect is valid, use screen_rect.size instead of 0 to adjust the size?

@Clubhouse1661
Copy link
Contributor Author

Clubhouse1661 commented Jan 10, 2026

Currently it looks like sizing responsibilities are split between Window, PopupMenu, and individual widgets.

If we're considering a full redesign for 4.7, we could introduce a PopupConstraints class which includes all sizing behavior in a single structure, and be called via a new method like popup_with_constraints()

@akien-mga akien-mga merged commit ec5a4f0 into godotengine:master Jan 10, 2026
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Clubhouse1661 Clubhouse1661 deleted the fix-option-button-popup-shrinking branch January 11, 2026 00:03
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.

OptionButtons PopupMenu not shrinking to correct size

6 participants