godot icon indicating copy to clipboard operation
godot copied to clipboard

Rename internal EditorPlugin icon/name to match exposed methods

Open aaronfranke opened this issue 1 year ago • 3 comments

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

aaronfranke avatar Oct 13 '24 03:10 aaronfranke

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.

KoBeWi avatar Oct 13 '24 06:10 KoBeWi

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

aaronfranke avatar Oct 30 '24 11:10 aaronfranke

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.

KoBeWi avatar Oct 30 '24 11:10 KoBeWi

Thanks!

Repiteo avatar Dec 16 '24 18:12 Repiteo