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 ColorPicker margin theme property #78468

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 20, 2023

Fixes #62937
Supersedes #76567

Changing the base type would break compatibility, so instead I just added another VBoxContainer inside MarginContainer inside ColorPicker. This is the rare occurrence where node's internal structure changes.

@KoBeWi KoBeWi added this to the 4.2 milestone Jun 20, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner June 20, 2023 10:50
@TheSecondReal0
Copy link
Contributor

TheSecondReal0 commented Jul 27, 2023

Tested and seems to work, what is the margin theme override supposed to do after this PR?
Also thanks for opening an improved version of my PR, really appreciate it and would love for this bug to get fixed!

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 27, 2023

margin adds a margin (padding?) around the ColorPicker. The previous implementation was supposed to do the same, but it wasn't implemented correctly.

@TheSecondReal0
Copy link
Contributor

Not sure how you tested or if it's meant to work this way but I just tried putting multiple colorpickers into an HBoxContainer and changing the margin override did nothing. Without this PR it moves the colorpicker down and to right without claiming more space which is also incorrect.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 27, 2023

Works correctly for me

godot.windows.editor.dev.x86_64_1Knf3ENqMj.mp4

@TheSecondReal0
Copy link
Contributor

Huh, I'll recompile and retest soon. If I get different results again I'll post a scene/project file, could you post yours?

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 1, 2023

Pretty sure it works in a fresh project.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Seems pretty straightforward. The original implementation looks super weird.

@akien-mga akien-mga merged commit 14256a2 into godotengine:master Aug 8, 2023
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the enmarginalization branch August 8, 2023 15:07
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.

ColorPicker nodes move when a scene is opened
4 participants