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 ThemeDB, expose previously static Theme methods #64119

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Aug 8, 2022

This PR adds a ThemeDB engine singleton that can be used, so far, to access the default and the project theme, as well as fetch (and set!) fallback values for the entire theming system.

godot windows tools 64_2022-08-08_20-12-10

This aims to provide ground work for any future implementation of godotengine/godot-proposals#4486. Regardless of what we agree upon in details, a theme database singleton makes a lot of sense to provide access to the theming system without overburdening the Theme resource itself with even more global data. I don't think I can finalize the idea of godotengine/godot-proposals#4486 before the Beta, let alone get it approved. So this should be a good first step to lay foundation for future changes. I expect ThemeDB to host and give access to well-defined theme properties of controls.

But thus far it only exposes static information related to themes and takes a bit of code off of theme.cpp's hands.

@YuriSizov YuriSizov added this to the 4.0 milestone Aug 8, 2022
@YuriSizov YuriSizov requested review from a team as code owners August 8, 2022 19:05
@YuriSizov YuriSizov force-pushed the theme-init-database branch from 98a5978 to f835f4e Compare August 8, 2022 19:06
@@ -17,6 +17,7 @@ SConscript("animation/SCsub")
SConscript("audio/SCsub")
SConscript("resources/SCsub")
SConscript("debugger/SCsub")
SConscript("theme/SCsub")
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this under gui more than its own folder, especially as it's only relevant for gui (with the exception of the GUI-adjacent Window and Label3D).

Copy link
Contributor Author

@YuriSizov YuriSizov Aug 8, 2022

Choose a reason for hiding this comment

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

Yeah, and Theme is a resource, so it could also be under resource... 🙃 I have a plan for another class that could go into scene/theme, and we already have audio that similarly has shared classes in their own folder.

@YuriSizov YuriSizov force-pushed the theme-init-database branch 6 times, most recently from 802f20b to 7f8e9fe Compare August 9, 2022 01:07
@YuriSizov
Copy link
Contributor Author

This PR will keep failing CI because the new singleton needs to be added to the regression testing project. Currently it doesn't know about ThemeDB, creates an instance of it and then frees it, thus breaking the singleton.

All other singleton exceptions are on this list (or are excepted based on the word "server" in their name, ThemeServer would've worked like a charm 🙃):
https://github.com/godotengine/regression-test-project/blob/4.0/CreatingAllThings/CreatingAllThings.gd#L4

Obviously, there is no point updating the test project if this PR is not approved. So the plan is to get this reviewed and approved, then update the test project, then rerun the regression test here, and then it can be safely merged.

@YuriSizov YuriSizov force-pushed the theme-init-database branch 2 times, most recently from 8ed6d20 to 68a97f8 Compare August 15, 2022 14:42
@YuriSizov YuriSizov force-pushed the theme-init-database branch from 68a97f8 to 6320a0f Compare August 26, 2022 16:23
@akien-mga akien-mga merged commit e60086f into godotengine:master Aug 29, 2022
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the theme-init-database branch August 29, 2022 12:03
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.

3 participants