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

Check the native base of scripts when resolving icons #81336

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Sep 5, 2023

See #80184 (comment). This is not a regression from #80184, though. The issue was present before as well, but it manifested differently:

image

The issue is that we only check the direct base class of script classes. This means that if your class extends another script class, we won't be able to find the fallback icon for it correctly. Before #80184 we would simply fall back onto the Node or Object, or whatever else was supplied as the common fallback icon. With #80184 the common fallback defaults to an empty string, so we end up with no icon whatsoever.

The fix is to find the first native base class instead of the direct base class and use its icon.

image

Fixes #79979.


PS. This PR touches code around the same lines as #81130, so it may lead to conflicts when one of them is merged. While this one is small and simple, I'd prefer #81130 to be merged first, if we're okay with it.

@YuriSizov YuriSizov added this to the 4.2 milestone Sep 5, 2023
@akien-mga
Copy link
Member

PS. This PR touches code around the same lines as #81130, so it may lead to conflicts when one of them is merged. While this one is small and simple, I'd prefer #81130 to be merged first, if we're okay with it.

This PR seems cherry-pickable, while rebasing on top of #81130 would make it incompatible with 4.1. So it may make more sense to merge the other way around?

@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 5, 2023
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 5, 2023

That's a good reason, yeah. It's just going to be slightly more annoying to rebase a large 3-commit PR, but I can manage :)

Note for cherry-picking, though, that it is based on #79203 (which may have caused this behavior, at least in its current form).

@akien-mga
Copy link
Member

Note for cherry-picking, though, that it is based on #79203 (which may have caused this behavior, at least in its current form).

#79979 mentions 4.1.1-stable though. But I guess this fix depends on #79203 anyway?

@YuriSizov
Copy link
Contributor Author

Yeah, the issue likely existed before, I vaguely remember it, but I didn't check. #79203 definitely changed where the affecting code resides and this PR changes the same part.

@akien-mga
Copy link
Member

akien-mga commented Sep 5, 2023

Alright, then I guess the order of merge isn't as important. I'm not sure the bug is problematic enough in 4.1 to warrant going out of our way for a custom backport. I guess it depends whether we do plan to cherry-pick #79203 or not.

@YuriSizov
Copy link
Contributor Author

I guess it depends whether we do plan to cherry-pick #79203 or not.

I'd say yes, it's a point of a lot of confusion. Making a custom PR would be an easy option as well, it will take less time than our conversation here has taken so far 😛

@YuriSizov
Copy link
Contributor Author

I need to rebase #81130 anyway now, so this is safe to merge once approved.

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.

Looks fine to me, though I'm not an area expert.

@YuriSizov YuriSizov merged commit 8449592 into godotengine:master Sep 6, 2023
15 checks passed
@YuriSizov
Copy link
Contributor Author

Thanks for reviews!

@YuriSizov YuriSizov deleted the editor-true-base-icon-lookup branch September 6, 2023 14:10
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor Author

Cherry-picked for 4.1.2.

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.

Extended class shows wrong icon
4 participants