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.

Sign up for GitHub

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

Jump to bottom

Fix infinite call loop on theme change. #91595

Merged
merged 1 commit into from May 13, 2024
Merged

Fix infinite call loop on theme change. #91595

merged 1 commit into from May 13, 2024
+24 −21

Conversation

ajreckof
Copy link
Contributor

@ajreckof ajreckof commented May 5, 2024

Fixes #91553
Based on this comment I had removed updating_theme but it seems it is still necessary even with bulk_theme_override.

@AThousandShips AThousandShips changed the title fix infinite call loop on theme change. Fix infinite call loop on theme change. May 5, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone May 5, 2024
@Chaosus
Copy link
Member

Chaosus commented May 5, 2024

Works fine but when switching theme not all elements switching correctly (I don't observe that on 4.3 dev 6):

изображение

Chaosus
Chaosus approved these changes May 5, 2024
View reviewed changes
Copy link
Member

@Chaosus Chaosus left a 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.

@ajreckof ajreckof force-pushed the fix-infinite-call-loop-on-theme-change branch from 9be8bf9 to 1f833be Compare May 5, 2024 19:03
@ajreckof
Copy link
Contributor Author

ajreckof commented May 5, 2024

Works fine but when switching theme not all elements switching correctly (I don't observe that on 4.3 dev 6):

изображение

Not related to the crash but fixed it as it was also introduced by my modification. While testing realized that editorPropertyCategory background is not updated on theme change, tested and it happened even before my modification so left it for later/ maybe there is already a issue / PR for it.

@KoBeWi
Copy link
Member

KoBeWi commented May 5, 2024

Yeah so the reason this is necessary is that EditorPropertyResource adds theme overrides inside its own NOTIFICATION_THEME_CHANGED and adding overrides will emit that notification again, resulting in infinite recursion.

I don't think it's valid to set theme overrides inside NOTIFICATION_THEME_CHANGED of the same object. The node could connect to theme_changed signal of parent inspector instead, to avoid calling theme changes on itself.

@ajreckof ajreckof force-pushed the fix-infinite-call-loop-on-theme-change branch from 1f833be to f47d6e3 Compare May 7, 2024 19:51
@ajreckof ajreckof requested a review from a team as a code owner May 7, 2024 19:51
KoBeWi
KoBeWi reviewed May 12, 2024
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the fix-infinite-call-loop-on-theme-change branch from f47d6e3 to 282d1a9 Compare May 12, 2024 19:36
@ajreckof
Copy link
Contributor Author

I updated it to use the parent theme_changed signal and since it was becoming too big to do separetly on each EditorProperty needing it I implemented it with a simple bool indicating wether it should have borders. it is only implemented to be set at init time. Should I comment on it or should I just make it work at any time with a dedicated setter?

@akien-mga akien-mga requested a review from KoBeWi May 13, 2024 10:02
KoBeWi
KoBeWi reviewed May 13, 2024
View reviewed changes
editor/editor_inspector.cpp Outdated
Comment on lines 410 to 416
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));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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));

Copy link
Member

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:

Suggested change
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));

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'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.

Copy link
Member

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:

Copy link
Contributor Author

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.

@ajreckof ajreckof force-pushed the fix-infinite-call-loop-on-theme-change branch 2 times, most recently from 4900e60 to a4f8e41 Compare May 13, 2024 11:43
@KoBeWi
Copy link
Member

KoBeWi commented May 13, 2024

Property backgrounds don't refresh:
image

@ajreckof
fix infinite call loop on theme change.
114ab9d 
Apply suggestions from code review
@ajreckof ajreckof force-pushed the fix-infinite-call-loop-on-theme-change branch from a4f8e41 to 114ab9d Compare May 13, 2024 12:39
@ajreckof
Copy link
Contributor Author

Fixed thanks

@akien-mga akien-mga merged commit 4fe9764 into godotengine:master May 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
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

@AThousandShips AThousandShips AThousandShips left review comments

@Chaosus Chaosus Chaosus approved these changes

Assignees
No one assigned
Labels
bug crash regression topic:core topic:editor
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Changing editor font or theme, cause stack overflow
5 participants
@ajreckof @Chaosus @KoBeWi @akien-mga @AThousandShips