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 a warning when accessing theme prematurely and fix surfaced issues #73475

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Feb 17, 2023

As noted by @KoBeWi when working on #73447, we often have these issues due to theme items being accessed too early, inside of the class' constructor. On top of that, there is often code missing to update such items on theme changes. At the earliest, node can access its own theme fetching methods during NOTIFICATION_POSTINITIALIZE.

So with this PR I'm attempting to catch all these errors. First of all, I'm adding a warning to all get_theme_<item_type> and has_theme_<item_type> in both Control and Window. Maybe some other methods of those classes should have a warning added as well, but that's enough for a start. Warnings are only dispatched once to prevent spam, and suggest using NOTIFICATION_POSTINITIALIZE and NOTIFICATION_THEME_CHANGED.

Code_2023-02-16_20-15-48

Second of all, and as a result of the first step, quite a few places were exposed and needed an update. 🙃 In the GUI code, TextEdit, CodeEdit, and ColorPicker are notable for these issues. For the most part this is because I haven't touched them when reworking theme cache before. The *Edits had some caching already and felt quite complex, and ColorPicker was being refactored at the time. So instead of just patching things, I moved all theme accessors to the new theme cache rails, and that solved everything.

In the editor code all sorts of things happen, and I generally tried to patch things instead of reworking them, though in a couple of places implementing theme cache was the easiest option. All in all, opening the editor now yields no warnings for me. But there are probably some other nodes that don't get initialized immediately, though that should be handled with follow-up PRs.

While not critical, this clean-up should prevent theming related issues in the future, and helps keeping the codebase consistent and organized. Putting it on the 4.1 milestone, because I could've made stupid typos while working on this, but if you feel lucky, can also be merged in 4.0 👀


Multiple commits for obvious reasons. All commits are technically independent, but the first one is useful to test the other 3 (in any order).

case NOTIFICATION_TRANSLATION_CHANGED:
case NOTIFICATION_LAYOUT_DIRECTION_CHANGED:
case EditorSettings::NOTIFICATION_EDITOR_SETTINGS_CHANGED: {
set_theme(EditorNode::get_singleton()->get_gui_base()->get_theme());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was very weird and goes back to #22770. I don't see any reason for this, unless theme propagation is somehow broken. But then this is not a solution.

@YuriSizov YuriSizov force-pushed the theme-is-busy-plz-come-back-later branch from a67a6a9 to 8c97f1b Compare February 17, 2023 12:28
@YuriSizov
Copy link
Contributor Author

because I could've made stupid typos while working on this

Well, what do you know, there were a couple of issues with TextEdit and CodeEdit. Should be good now.

@YuriSizov YuriSizov force-pushed the theme-is-busy-plz-come-back-later branch from 8c97f1b to 4a87336 Compare March 16, 2023 11:42
void CodeEdit::_update_theme_item_cache() {
TextEdit::_update_theme_item_cache();

/* Gutters */
Copy link
Member

@KoBeWi KoBeWi Apr 3, 2023

Choose a reason for hiding this comment

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

Suggested change
/* Gutters */
// Gutters.

You could use the normal comment style. /**/ isn't used consistently anyway.

EDIT:
Eh, but you used it in the header 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just copied those from somewhere else in the file, and then perhaps added my own.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

@Paulb23 might want to take a look at TextEdit changes, because you are moving around carefully organized members.

@YuriSizov YuriSizov force-pushed the theme-is-busy-plz-come-back-later branch from 4a87336 to 8563b0f Compare April 3, 2023 14:51
@YuriSizov YuriSizov force-pushed the theme-is-busy-plz-come-back-later branch from 8563b0f to 9b500ab Compare April 3, 2023 16:29
@YuriSizov
Copy link
Contributor Author

Rebased on top of #74729, tested and noticed no warnings in PM @KoBeWi

@akien-mga akien-mga merged commit 4f4b5a2 into godotengine:master Apr 5, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the theme-is-busy-plz-come-back-later branch April 5, 2023 10:17
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