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

Rename internal EditorPlugin icon/name to match exposed methods #98132

Merged

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Oct 13, 2024

Same as PR #98039 but for EditorPlugin.

String EditorPlugin::get_name() const {
	String name;
	GDVIRTUAL_CALL(_get_plugin_name, name);
	return name;
}

const Ref<Texture2D> EditorPlugin::get_icon() const {
	Ref<Texture2D> icon;
	GDVIRTUAL_CALL(_get_plugin_icon, icon);
	return icon;
}

I noticed that the internal names of these functions were different from the names exposed to scripting. They were missing the plugin in the middle, in contrast to get_plugin_version.

This PR fixes the issue by renaming the internal functions, such that they look like this:

String EditorPlugin::get_plugin_name() const {
	String name;
	GDVIRTUAL_CALL(_get_plugin_name, name);
	return name;
}

const Ref<Texture2D> EditorPlugin::get_plugin_icon() const {
	Ref<Texture2D> icon;
	GDVIRTUAL_CALL(_get_plugin_icon, icon);
	return icon;
}

This PR preserves API compatibility with GDScript/C#/GDExtension/etc, which already use the _get_plugin_* names, however third-party engine modules may need to be updated to support the new name.

I believe using a more verbose name is the better approach to take compared to simplifying the name. Using a more-unique name improves the ability to search for the function in the codebase using grep or global search, and significantly improves the speed at which IDEs can find all uses of it (since they usually find all tokens matching the name, and then do analysis to see which are actual matches). See also PR #36382 and PR #44263 for precedent.

Note, if third-party modules need to be compatible with both 4.3 and 4.4, that is easy to do with this:

#if GODOT_VERSION < 0x040400
#define set_plugin_icon set_icon
#define set_plugin_name set_name
#endif

@KoBeWi
Copy link
Member

KoBeWi commented Oct 13, 2024

I assume get_plugin_name() is supposed to avoid conflict with Node's get_name(), and get_plugin_icon() is for consistency with the former. Using "plugin" in a plugin's method name is a bit redundant I think.

@aaronfranke aaronfranke force-pushed the editor-plugin-get-plugin-icon-name branch 2 times, most recently from 3901319 to b911bf4 Compare October 30, 2024 06:43
@aaronfranke
Copy link
Member Author

@KoBeWi Yes, it is redundant, but it's a trade-off. While redundant, this is more consistent, more verbose, easier to grep, and easier for IDEs to deal with.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 30, 2024

I wouldn't list "more verbose" as improvement. Also IDEs are generally able to handle ambiguous names.
Still, #98039 was merged, so this one is as acceptable.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

I personally agree with this one for the same reasons as PR #98039

@aaronfranke aaronfranke force-pushed the editor-plugin-get-plugin-icon-name branch from 7cdc67b to 0ab3dc2 Compare December 16, 2024 01:32
@Repiteo Repiteo merged commit 02e196e into godotengine:master Dec 16, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 16, 2024

Thanks!

@aaronfranke aaronfranke deleted the editor-plugin-get-plugin-icon-name branch December 16, 2024 23:11
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.

5 participants