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 case where h_separation might not work in Button #64218

Merged

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Aug 10, 2022

Fix #64140.
Bugsquad edit: Fixes #64371

Before After
1 2

Split from #59340.

There are still issues with the drawing and will be fixed in another PR.

Edit:
This patch mainly solves two things:

  1. The typo of h_separation;
  2. Negative values of h_separation will be treated as 0 when used, to prevent the button's minimum width from being reduced by h_separation.

@Rindbee Rindbee requested a review from a team as a code owner August 10, 2022 13:21
@YuriSizov YuriSizov added this to the 4.0 milestone Aug 10, 2022
scene/gui/button.cpp Outdated Show resolved Hide resolved
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.

Looks good.

@Rindbee Rindbee force-pushed the fix-button-minimum-size-calculation branch from a6a4220 to bf9162b Compare August 10, 2022 22:38
@Rindbee Rindbee requested a review from a team as a code owner August 10, 2022 22:38
@Rindbee Rindbee requested a review from KoBeWi August 10, 2022 22:40
@KoBeWi
Copy link
Member

KoBeWi commented Aug 10, 2022

You added me as co-author (GitHub does this automatically when you accept a suggestion).

@Rindbee Rindbee force-pushed the fix-button-minimum-size-calculation branch from bf9162b to bebc75e Compare August 10, 2022 23:16
KoBeWi
KoBeWi previously approved these changes Aug 10, 2022
@Rindbee Rindbee force-pushed the fix-button-minimum-size-calculation branch from bebc75e to 8f54e69 Compare August 11, 2022 03:00
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 11, 2022

Sorry, h_separation should be ignored when expand_icon is enabled.

@Rindbee Rindbee requested a review from KoBeWi August 11, 2022 03:03
@KoBeWi
Copy link
Member

KoBeWi commented Aug 11, 2022

Uh why should it be ignored? It's just a space between icon and text. If you expand icon and resize to 0, it's still there and separation should take effect. The previous behavior was alright.

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 11, 2022

For Button-derived classes with internal elements, CheckBox, CheckButton, OptionButton, may appear to be twice as long (the icon is hidden).

Of course, they behave inconsistently, and it's not clear to me which one to use as a standard.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 11, 2022

Ah seems like CheckBox etc. apply the separation twice for their own icon.
image
In this case ignoring separation makes sense.

But there is still no reason to ignore it in Button. You could either add a virtual method that tells whether this should be ignored or, more simple but not much used in the codebase, some protected bool property.

@KoBeWi KoBeWi dismissed their stale review August 11, 2022 11:41

Seems like more changes are needed

@Rindbee Rindbee force-pushed the fix-button-minimum-size-calculation branch 4 times, most recently from d31d053 to 323d611 Compare August 12, 2022 03:02
@Rindbee

This comment was marked as off-topic.

@Rindbee Rindbee force-pushed the fix-button-minimum-size-calculation branch from 323d611 to ec66f7c Compare August 12, 2022 06:20
This patch mainly solves two things:
1. The typo of `h_separation`;
2. Negative values of `h_separation` will be treated as `0` when used, to prevent the button's minimum `width` from being reduced by `h_separation`.
@Rindbee Rindbee force-pushed the fix-button-minimum-size-calculation branch from ec66f7c to 4a3a15c Compare August 12, 2022 11:59
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 12, 2022

I seem to be getting more and more deviated from the original intention. Now I only fix typo and restrict h_separation from being negative.

When h_separation is non-negative, it should look the same as in alpha13

@KoBeWi
Copy link
Member

KoBeWi commented Aug 13, 2022

OptionButton minimum size seems broken:
godot windows tools 64_3hNJ8pA3rH

CheckBox center alignment makes the text go into separation area:
godot windows tools 64_FkKmFR9U8f
The worst part is that it's more to the left than Left alignment. CheckButton has similar problem.

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 13, 2022

OptionButton minimum size seems broken.

I guess it has something to do with fit_to_longest_item, switching expand_icon doesn't update the cache.
Steps to reproduce it:

  1. Make h_separation larger, such as 200;
  2. Let fit_to_longest_item be disabled and expand_icon be enabled;
  3. Then enable fit_to_longest_item;
  4. Disable expand_icon.

Currently, changes in the factors that affect get_minimum_size do not update the OptionButton cache. I think it's better to disable fit_to_longest_item first and then test, finally, solve the problem of fit_to_longest_item cache update in other PRs.

CheckBox center alignment makes the text go into separation area. The worst part is that it's more to the left than Left alignment. CheckButton has similar problem.

This should be a drawing issue, which will be resolved in #64351, where there is a set of comparison images with alpha13.

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.

I assume #64218 (comment) is addressed in #64351?

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 14, 2022

I assume #64218 (comment) is addressed in #64351?

I guess no, both patches are trying to get back to alpha13. Except that a negative h_separation is treated as 0, and limits the extent to which the text/icon can be drawn. These changes between alpha13 should be intuitively found from comparing pngs in #64351.

This issue in #64218 (comment) also exists in alpha13.

I'm a little unclear on what exactly it should look like. So had to use alpha13 as a reference. If there is something wrong in alpha13, I will fix it later. I'm a little concerned about over-modification causing other regressions.

It might be better to figure out what it should look like first.

  1. internal element (i_icon)
    1.1 existence
  2. custom elements
    2.1. custom icon (icon)
    2.1.1. existence
    2.1.2. expand_icon
    2.1.3. icon_alignment
    2.2. custom text (text)
    2.2.1. existence
    2.2.2. clip_text
    2.2.3. text alignment
    2.3. separation between custom elements (s1)
  3. separation between internal element and custom elements (s0)

The above will directly and indirectly affect the minimum size.
In alpha13, if there are only icon and i_icon, s0 is ignored. It's a little different when if i_icon, icon, text all exists.

captrue
The documentation's description and the actual drawing in alpha13 are inconsistent.

1
The actual drawing in alpha13 and your suggestions are inconsistent.

I haven't looked into the button's design criteria, so I'm a little confused.

However, this patch is intended to fix typo to get it back to a slightly better state.

@akien-mga akien-mga merged commit b9ea0e1 into godotengine:master Aug 22, 2022
@akien-mga
Copy link
Member

Thanks!

@Rindbee Rindbee deleted the fix-button-minimum-size-calculation branch August 22, 2022 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants