-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Support extension icons in ScriptCreateDialog #98914
base: master
Are you sure you want to change the base?
Support extension icons in ScriptCreateDialog #98914
Conversation
Attempt to grab the icon for a script type from an extension first, before falling back to the theme. This adds a method to option_button to allow the ScriptCreateDialog to set the max size of an item icon, so that it can set the size from the Editor Theme when reading the icon from extension.
4f8db25
to
bf3441c
Compare
2e22ce8
to
2d42776
Compare
2d42776
to
0a3fc27
Compare
Ref<Texture2D> language_icon = get_editor_theme_icon(ScriptServer::get_language(i)->get_type()); | ||
// Check if the extension has an icon first | ||
String script_type = ScriptServer::get_language(i)->get_type(); | ||
Ref<Texture2D> language_icon = ed.extension_class_get_icon(script_type); | ||
if (language_icon.is_valid()) { |
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.
It may not be easily doable with how the themes are set up, but don't you think a theme's script language icon should trump the extension's icon, if set explicitly?
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.
Yeah that's a good point.
Debugging through the code though, I'd have to check the return of get_editor_theme_icon
to see if it's the same icon as default fallback. That's probably fine? I'll see if I can get it to work.
cccc520
to
4f207b1
Compare
@Ivorforce Updated so that theme takes precedence unless it returns the fallback icon, at which point it will ask the extension. |
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 is two changes in one (resizable language icon, and asking GDExtensions for language icons); normally I'd put them in two PRs or at least two commits, but I think it's fine here.
Implementation looks good to me!
<param index="0" name="idx" type="int" /> | ||
<param index="1" name="width" type="int" /> | ||
<description> | ||
Sets the maximum allowed width of the icon for the item at the given [param idx]. The height is adjusted according to the icon's ratio. |
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.
It might be nice to link to [method PopupMenu.set_item_icon_max_width]
here to have cross coverage.
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.
🤔 You're probably right, but if that's the case we should probably add it to all the get_item_*
methods yeah? I feel like it would be weird if this was the only one that cross linked...
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 guess that's true. Let's consider it outside the scope of this PR. We could make a follow-up issue about this, though i guess it's not all that important.
Attempt to grab the icon for a script type from an extension first, before falling back to the theme.
This adds a method to option_button to allow the ScriptCreateDialog to set the max size of an item icon, so that it can set the size from the Editor Theme when reading the icon from extension.
Example from Godot Dart:
Fixes #98800