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.

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

Fix the expand icon calculation logic to the function Button::get_minimum_size #59340

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Mar 20, 2022

Fix the expand icon calculation logic which belongs to the function Button::get_minimum_size

Type Before After
Button 1 3
OptionButton 2 4

If enable the expand_icon, the icon may be hidden when the width of the button is not wide enough. This is where I mainly want to fix in this patch.

Note:
The last thing worth noting is that the minimum achievable scale size is highly correlated with the font size, even if the text is empty. So, if you want a small texture size, you need to make the font size small enough. I'm not sure the specific design requirements, just adjusted some logic.

Should we make the texture keep aspect, or follow other rules?

Other matters

I'm glad it partially resolves #59202, but if the height of the button is too large, it will still appear.

56
I checked the source code of OptionButton, it overrides this method, maybe this problem needs to be fixed there. It is very likely that this property theme_override_constants/arrow_margin was not considered.

@Rindbee Rindbee requested a review from a team as a code owner March 20, 2022 05:48
@Chaosus Chaosus added this to the 4.0 milestone Mar 20, 2022
@Chaosus
Copy link
Member

Chaosus commented Mar 20, 2022

Maybe you should explain what this PR does in details and how it works (for example by providing some video to show)?

@EricEzaM
Copy link
Contributor

EricEzaM commented Mar 20, 2022

Is this fixing an existing issue? #59202 Perhaps? You can indicate that a PR will close a certain issue by writing, for example.

Fixes #issue number

There are a number of these keywords, see here: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Explaining a bit about what you did in the PR and perhaps an image or GIF showing the fixed behaviour also goes a long way, as Chaosus said.

@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 20, 2022

@Chaosus @EricEzaM Yes, the GIF can make the description more intuitive. Do you have any good tools to recommend on the Linux platform.

@Chaosus
Copy link
Member

Chaosus commented Mar 20, 2022

Do you have any good tools to recommend on the Linux platform.

Probably one of the recorders listed in here: https://itsfoss.com/best-gif-recorder-linux. On Windows (which I'm using), the screentogif serves me the best.

@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 20, 2022

Do you have any good tools to recommend on the Linux platform.

Probably one of the recorders listed in here: https://itsfoss.com/best-gif-recorder-linux. On Windows (which I'm using), the screentogif serves me the best.

Thanks, I will try it.

@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 20, 2022

Thanks again to @Chaosus for the recommended tool, peek. Now it behaves like in the GIF image above.

@Rindbee Rindbee changed the title Fix the expand icon calculation logic in Button. Fix the expand icon calculation logic to the function Button::get_minimum_size Mar 21, 2022
@Rindbee Rindbee force-pushed the fix-button-expand-icon-calculation-logic branch from 9a2049a to 0648399 Compare March 21, 2022 01:14
@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 21, 2022

@Chaosus @EricEzaM OK, now I think it would illustrate the main purpose of this patch. Please review. I feel like I found the cause of the problem in #59202 . Should I fix it in this PR, or open a new one?

@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 21, 2022

Before After
Before After

When the size is the smallest, modifying the property theme_override_constants/arrow_margin, now the arrow icon will not appear outside the button.

@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 21, 2022

7

Now, it fixes #59202 by adding a callback method.

The method will return the width caused by the properties beside those in Button.

Edited:

I found that there is a property _internal_margin already defined in Button. This property seems to have a similar effect to get the size of inner icons, but I'm not clear on its specific purpose.

@Rindbee Rindbee force-pushed the fix-button-expand-icon-calculation-logic branch from 858dc89 to 8d790fd Compare March 22, 2022 08:33
@Rindbee Rindbee force-pushed the fix-button-expand-icon-calculation-logic branch from 8d790fd to 1ba78ba Compare April 13, 2022 10:01
Take the theme property theme_override_constants/arrow_margin into account when calculating the minimum size.

Use the actual size, suitable for asymmetric.

Add a function to get the width of the extra properties, which may affect the width of the size.

Fixes godotengine#59202
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 13, 2022

Superseded by #64351.

@Rindbee Rindbee closed this Aug 13, 2022
@Rindbee Rindbee deleted the fix-button-expand-icon-calculation-logic branch August 13, 2022 08:46
@YuriSizov YuriSizov removed this from the 4.0 milestone Jan 18, 2024
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.

OptionButton expand_icon breaks the layout
4 participants