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

Fetch theme editor items from ThemeDB #84763

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 11, 2023

Together with #84760 fixes #82402

@KoBeWi KoBeWi added this to the 4.2 milestone Nov 11, 2023
@KoBeWi KoBeWi requested a review from YuriSizov November 11, 2023 18:15
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 12, 2023
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.

Needs a rebase, but otherwise looks good.

Do you think we leave any performance on the table by switching from a method pointer to get_theme_item_list? I know this is needed for ThemeDB anyway, so it's fine. But I'm not well versed in C++ so curious what your take is here.

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.

Actually, as I was testing #84760, I realized there is something wrong will all 3 PRs. While we can use get_class_own_items in the docs (and in fact we should), for the theme editor, the override list, and the argument autocompletion we need to include inherited items as well.

This requires a new get_class_items method, which returns not just own items. Plus we need a method to get such items filtered based on the data type, as outlined above. I think one of these PRs need to do all this groundwork, and then others can be built on top of it.

Let me know if you need any help with any of that.

PS. If you need an example of this being a problem, check CheckBox for instance.

@KoBeWi KoBeWi force-pushed the all-knowing_theme_editor branch from 287a11f to e86750d Compare January 31, 2024 12:28
@KoBeWi KoBeWi requested a review from a team as a code owner January 31, 2024 12:28
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 31, 2024

Updated. I added yet another argument to get_class_items(), which can be used to filter by type. If I were to add a new method, it would be pretty much the same, so I wanted to avoid code duplication.

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.

Code changes look fine to me.

@akien-mga akien-mga merged commit 32b0834 into godotengine:master Feb 13, 2024
16 checks passed
@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.

Theme items that don't exist in the default theme are not listed in the editor
3 participants