-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Add a warning when accessing theme prematurely and fix surfaced issues #73475
Conversation
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()); |
There was a problem hiding this comment.
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.
a67a6a9
to
8c97f1b
Compare
Well, what do you know, there were a couple of issues with |
8c97f1b
to
4a87336
Compare
void CodeEdit::_update_theme_item_cache() { | ||
TextEdit::_update_theme_item_cache(); | ||
|
||
/* Gutters */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* Gutters */ | |
// Gutters. |
You could use the normal comment style. /**/
isn't used consistently anyway.
EDIT:
Eh, but you used it in the header 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
4a87336
to
8563b0f
Compare
8563b0f
to
9b500ab
Compare
Thanks! |
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>
andhas_theme_<item_type>
in bothControl
andWindow
. 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 usingNOTIFICATION_POSTINITIALIZE
andNOTIFICATION_THEME_CHANGED
.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
, andColorPicker
are notable for these issues. For the most part this is because I haven't touched them when reworking theme cache before. The*Edit
s had some caching already and felt quite complex, andColorPicker
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).