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

Unify theme item lookup in Controls and respect default font #55497

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Nov 30, 2021

Okay, let's try this. This is 3.x only. Fixes #36614.

The problem was that the default font defined on any theme is never respected when looking up theme items from controls. The only way to receive the default font is to either directly fetch it from the Theme or to ask that Theme for an item that doesn't exist without checking with has_... first. This means that you can never, realistically, hit it for controls automatically. And that means, you can never get it from the custom project theme.

Now, this change is a fix for this behavior, and it matches the behavior currently present in master (added in #41100). This fix means that if any theme in the lookup chain has a default font, it will be used (as fallback), and no further lookup will be performed. This breaks current behavior, but I don't think that the impact of that change outweighs the benefit. In fact, since the default font never worked as expected, I doubt anyone actually used it. So I think this change is justified for 3.5.


While at it I've also unified the theme item lookup to match what we have in master. This can probably fix a few issues as well, since the old behavior was arbitrary for each theme item type. I excluded shaders from this change, since they are already deprecated and I don't want to add a shader type to the DataType enum and possibly create problems for the theme editor and other stuff.

This change is rather exciting, as it lays the groundwork for a backport of theme type variations (#50169). If this is accepted, theme type variations can be added without any compatibility breakage and can be a very cool feature for 3.5.

Copy link
Member

@akien-mga akien-mga 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 to me.

Might need a rebase/quick re-test for good measure given that a lot of stuff has been merged/cherry-picked in the meantime.

@YuriSizov YuriSizov force-pushed the control-theme-lookup-tuneup-3.x branch from f50d426 to 5831956 Compare January 13, 2022 12:25
@YuriSizov
Copy link
Contributor Author

Still works!

@akien-mga akien-mga merged commit d5c9b93 into godotengine:3.x Jan 13, 2022
@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.

2 participants