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

Add methods to remove theme overrides #47252

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 22, 2021

Resolves #32110

@KoBeWi KoBeWi added this to the 4.0 milestone Mar 22, 2021
@KoBeWi KoBeWi requested review from a team as code owners March 22, 2021 12:04
@akien-mga
Copy link
Member

Aside from add_theme_color_override, all other theme property overrides can be cleared by passing a given value (0 or null) as written in the docs.

If we add remove_theme_*_override for all of them, then maybe we should also remove the behavior where they clear themselves on specific values.

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 22, 2021

I say we remove that behavior in master, but keep it in 3.x (as this PR can be cherrypicked otherwise). It doesn't make sense for a function called add_override to have a special case value that removes an override instead. I feel like it's just a hack for the Inspector, and nothing else (which should be tested if we do remove this trick).

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 22, 2021

If we add remove_theme_*_override for all of them, then maybe we should also remove the behavior where they clear themselves on specific values.

I tried this. When you set a null stylebox it will crash, so I just left it as is.

@akien-mga
Copy link
Member

When you set a null stylebox it will crash, so I just left it as is.

Sounds like a good reason not to leave it as is :) Or at least this needs a bug report as it contradicts the documentation.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 22, 2021

Maybe it should just throw error on null 🤔
btw by "as is" I mean the current behavior of removing override.

EDIT:
Also other add_*_overrides don't seem to have code that would handle erasing...

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 22, 2021

Ok altered the old behavior in another commit.

@akien-mga akien-mga merged commit 5d0cc7c into godotengine:master Mar 31, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Impossible to clear Control theme override values
3 participants