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 EditorProperty spacing #93089

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

passivestar
Copy link
Contributor

EditorProperty was trying to use a wrong constant for horizontal separation resulting in broken spacing (most noticeably for the keying icon). This PR adds missing horizontal space and also centers the key icon vertically

Before/After:

ezgif-5-8f4c11ed48

Tested with all of the possible icons that may appear in EditorProperty and a custom theme:

1

Also tested RTL:

2

Note: Key.svg shape was not centered vertically in the svg file because there's also a KeyNext.svg which includes an additional plus sign element below the key. It's likely best to make Key.svg centered regardless (which I did in this PR), otherwise it looks broken in the editor. Spent some time trying to figure out why it isn't centered before I realised it's the icon itself

@passivestar passivestar requested a review from a team as a code owner June 12, 2024 14:20
@AThousandShips AThousandShips added this to the 4.3 milestone Jun 12, 2024
@akien-mga
Copy link
Member

Note: Key.svg shape was not centered vertically in the svg file because there's also a KeyNext.svg which includes an additional plus sign element below the key. It's likely best to make Key.svg centered regardless (which I did in this PR), otherwise it looks broken in the editor. Spent some time trying to figure out why it isn't centered before I realised it's the icon itself

KeyNext.svg is used when keying incremental properties like Sprite2D's frames:

image

It would be good to check that it doesn't look weird after this PR. I guess it should be fine as they're used on different lines, and not as two changing states of the same button.

@passivestar
Copy link
Contributor Author

It would be good to check that it doesn't look weird after this PR

Looks pretty good to me. You be the judge

image

@@ -291,7 +302,7 @@ void EditorProperty::_notification(int p_what) {
} else {
draw_texture(checkbox, check_rect.position, color2);
}
int check_ofs = get_theme_constant(SNAME("hseparator"), SNAME("Tree")) + checkbox->get_width() + get_theme_constant(SNAME("h_separation"), SNAME("CheckBox"));
int check_ofs = checkbox->get_width() + get_theme_constant(SNAME("h_separation"), SNAME("Tree"));
Copy link
Member

Choose a reason for hiding this comment

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

Is the removal of the CheckBox separation intentional?

I suppose this checkbox is used for stuff like Theme Overrides in control nodes:

image

Copy link
Contributor Author

@passivestar passivestar Jun 12, 2024

Choose a reason for hiding this comment

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

Just seems excessive in the context of EditorProperty because it's the only button in EditorProperty that you can independently control the "padding" of (i.e why only checkbox and not the revert button, key button, etc?), and also the way it was implemented is it was adding two constants together which I think makes theming less predictable

If we think that the flexibility of specifying the checkbox spacing independently from EditorProperty "global" horizontal spacing is important I'd say it would probably make more sense to only take the checkbox h_separation into the account there

@@ -88,17 +88,17 @@ Size2 EditorProperty::get_minimum_size() const {

if (keying) {
Ref<Texture2D> key = get_editor_theme_icon(SNAME("Key"));
ms.width += key->get_width() + get_theme_constant(SNAME("hseparator"), SNAME("Tree"));
ms.width += key->get_width() + get_theme_constant(SNAME("h_separation"), SNAME("Tree"));
Copy link
Member

Choose a reason for hiding this comment

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

That would be for a future PR, but we should probably move these and other theme constants to a ThemeCache struct like we have in some other editor controls (e.g. EditorFileDialog).

@akien-mga akien-mga requested a review from KoBeWi June 12, 2024 15:15
@akien-mga akien-mga merged commit f94c5e8 into godotengine:master Jun 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@passivestar passivestar deleted the editor-property-spacing branch June 14, 2024 09:10
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.

4 participants