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.

Sign up for GitHub

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

Jump to bottom

Update size or size cache when toggling expand_icon in Button #75958

Merged
merged 1 commit into from
May 17, 2023
Merged

Update size or size cache when toggling expand_icon in Button #75958

merged 1 commit into from
May 17, 2023

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Apr 12, 2023 •
edited
Loading

When the expand_icon is switched, the size cache can be updated to solve the issue that the cache cannot be updated when the OptionButton is enabled with fit_to_longest_item.

Fix #68930. Combining with #64351 may work even better.

Commit Image
Before 0
This PR 1
This + #64351 2

@Rindbee Rindbee requested a review from a team as a code owner April 12, 2023 01:40
@Rindbee Rindbee mentioned this pull request Apr 12, 2023
Closed
@Rindbee Rindbee force-pushed the update_size_cache_in_Button branch from 052d0b5 to 21406a4 Compare April 12, 2023 02:07
@Rindbee Rindbee mentioned this pull request Apr 12, 2023
Merged
@YeldhamDev YeldhamDev added this to the 4.1 milestone Apr 12, 2023
@YeldhamDev
Copy link
Member

Feels a little hackish to just bring _queue_refresh_cache() back a class.

@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 12, 2023

When the properties of the parent class change, the way to notify the subclass, here all I can think of is a callback function or a signal. Maybe there's a better way than either of these. But it feels unnecessary to use a signal for this property change alone.

KoBeWi
KoBeWi approved these changes May 2, 2023
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

It's hacky, but I can't really think of a better solution (we could make set_expand_icon() virtual, but it's meh). Maybe we could rename the method to something more generic, like _expand_icon_toggled(), but the current name is fine.

@akien-mga akien-mga requested a review from YuriSizov May 8, 2023 11:30
YuriSizov
YuriSizov suggested changes May 16, 2023
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it's hacky, but pretty minor. Can you please rename the method though, so its purpose clear? _queue_update_size_cache should do the trick.

@Rindbee
Update size or size cache when toggling expand_icon in Button
9bd1d3b 
When the `expand_icon` is switched, the size cache can be updated to
solve the issue that the cache cannot be updated when the `OptionButton`
is enabled with `fit_to_longest_item`.
@Rindbee Rindbee force-pushed the update_size_cache_in_Button branch from 21406a4 to 9bd1d3b Compare May 16, 2023 22:08
@Rindbee
Copy link
Contributor Author

Rindbee commented May 16, 2023

OK, renaming is done.

@akien-mga akien-mga merged commit 019fef7 into godotengine:master May 17, 2023
@akien-mga
Copy link
Member

Thanks!

@Rindbee Rindbee deleted the update_size_cache_in_Button branch May 17, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriSizov YuriSizov YuriSizov approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
bug topic:gui
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

OptionButton's minimum size does not update after changing expand_icon
5 participants
@Rindbee @YeldhamDev @akien-mga @KoBeWi @YuriSizov