-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Fix infinite call loop on theme change. #91595
Fix infinite call loop on theme change. #91595
Conversation
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.
Perhaps it would be helpful as a quick fix.
9be8bf9
to
1f833be
Compare
Not related to the crash but fixed it as it was also introduced by my modification. While testing realized that |
Yeah so the reason this is necessary is that EditorPropertyResource adds theme overrides inside its own I don't think it's valid to set theme overrides inside |
1f833be
to
f47d6e3
Compare
f47d6e3
to
282d1a9
Compare
I updated it to use the parent |
editor/editor_inspector.cpp
Outdated
get_parent()->connect("theme_changed", callable_mp(this, &EditorProperty::_update_property_bg)); | ||
_update_property_bg(); | ||
} | ||
} break; | ||
case NOTIFICATION_EXIT_TREE: { | ||
if (has_borders) { | ||
get_parent()->disconnect("theme_changed", callable_mp(this, &EditorProperty::_update_property_bg)); |
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.
get_parent()->connect("theme_changed", callable_mp(this, &EditorProperty::_update_property_bg)); | |
_update_property_bg(); | |
} | |
} break; | |
case NOTIFICATION_EXIT_TREE: { | |
if (has_borders) { | |
get_parent()->disconnect("theme_changed", callable_mp(this, &EditorProperty::_update_property_bg)); | |
get_parent()->connect(SNAME("theme_changed"), callable_mp(this, &EditorProperty::_update_property_bg)); | |
_update_property_bg(); | |
} | |
} break; | |
case NOTIFICATION_EXIT_TREE: { | |
if (has_borders) { | |
get_parent()->disconnect(SNAME("theme_changed"), callable_mp(this, &EditorProperty::_update_property_bg)); |
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.
Or, including scene/scene_string_names.h
:
get_parent()->connect("theme_changed", callable_mp(this, &EditorProperty::_update_property_bg)); | |
_update_property_bg(); | |
} | |
} break; | |
case NOTIFICATION_EXIT_TREE: { | |
if (has_borders) { | |
get_parent()->disconnect("theme_changed", callable_mp(this, &EditorProperty::_update_property_bg)); | |
get_parent()->connect(SceneStringName(theme_changed), callable_mp(this, &EditorProperty::_update_property_bg)); | |
_update_property_bg(); | |
} | |
} break; | |
case NOTIFICATION_EXIT_TREE: { | |
if (has_borders) { | |
get_parent()->disconnect(SceneStringName(theme_changed), callable_mp(this, &EditorProperty::_update_property_bg)); |
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'm assuming this is what you meant as the code you suggested didn't seem to work SceneStringNames::get_singleton()->theme_changed
. Please correct me if this was not what you were suggesting.
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.
No I mean exactly what I suggested, it requires a rebase:
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.
ok it was merged two hours ago that is why I didn't have it. Thanks for clarifying.
4900e60
to
a4f8e41
Compare
Apply suggestions from code review
a4f8e41
to
114ab9d
Compare
Fixed thanks |
Thanks! |
Fixes #91553
Based on this comment I had removed
updating_theme
but it seems it is still necessary even withbulk_theme_override
.