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

Add support for custom items to editor right-click context menus #94582

Merged

Conversation

citizenll
Copy link
Contributor

@citizenll citizenll commented Jul 21, 2024

Add right-click menu plugin functionality to implement this proposal

Currently, I am only working on 3.x, but in order to incorporate this feature, this is a feature version of 4.x. The difference compared to 3.x is that additional submenu options have been added (currently only expanding the create new option for filesystem).

submenu options example

extends EditorContextMenuPlugin
const PLUG = preload("res://addons/context-plugin/plug.svg")

func _can_handle(paths: PackedStringArray) -> bool:
	return true

func _init() -> void:
	add_context_menu_item(PLUG, "Svg...", _handle_create_svg, SUBMENU_SLOT_FILESYSTEM_CREATE)

func _handle_create_svg(args):
	print(args)

Please refer to the 3.x submission for a more detailed explanation here

@citizenll citizenll requested review from a team as code owners July 21, 2024 09:13
@AThousandShips AThousandShips changed the title feat(plugin):support context menu plugin in 4.x Support adding custom items to editor right-click context menus Jul 21, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Jul 25, 2024

So I tested this and it works, but the API is super weird.
add_context_menu_item() does not add an item, it defines an item that will appear when plugin is activated. This means that each plugin can add only one menu item. The function's first argument is an icon, which is completely optional for menus. Also if you don't call this method, the editor will crash.
There is SUBMENU_SLOT_NONE which does nothing (?) and SUBMENU_SLOT_FILESYSTEM_CREATE, which is a part of different enum and can't be used.

I think instead of _can_handle(), there could be _popup_menu(arguments: Array) virtual function. Though maybe paths is fine, not sure where else we will want to add options.
Inside that function the user is supposed to call add_context_menu_item(), as many times as they want. I'd change the signature to add_context_menu_item(text: String, callback: Callable, icon: Texture2D = null, shortcut: Shortcut = null). The plugin would have a vector of structs representing added menu items. The editor would then use this vector to add menu items to the PopupMenu.

Instead of keeping an array of plugins per slot, I think it can be a single LocalVector. When opening menu, go through the vector and check the plugin slot. If the slot matches, call _popup_menu() and then add options to menu.

To make your life easier, this can be handled with internal methods:
add_options_from_plugins(PopupMenu *p_menu, ContextMenuSlot p_slot, const PackedStringArray &p_paths) (this can be a method in EditorData; you need to keep the registered plugins somewhere)
EditorContextMenuPlugin::add_options(const PackedStringArray &p_paths) - it would clear the option structs vector, call the virtual _popup_menu() and then add items based on added options.

Feel free to ask if something is not clear. You can also join the developer chat.

@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 19076fa to 8b925a4 Compare August 1, 2024 13:25
@citizenll
Copy link
Contributor Author

Based on the recommendations, the final API has been adjusted; _can_handle has been changed to _popup_menu. The method add_context_menu_item will store data in a local HashMap, meaning that menu items with the same name will be unique. Additionally, to enhance understanding, submenu slots are now on the same level as other slots. Below is the example code.

extends EditorContextMenuPlugin
const PLUG = preload("res://addons/csv-update/plug.svg")

func _popup_menu(paths: PackedStringArray) -> bool:
	## The return result will determine whether to display the added options.
	add_context_menu_item("File Custom options", handle, PLUG)
	return true

func handle(paths):
	print("file context item handle:", paths)

@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 8b925a4 to 8233b0b Compare August 1, 2024 13:58
@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 8233b0b to 56dce9a Compare August 2, 2024 01:09
@KoBeWi

This comment was marked as resolved.

@@ -101,6 +102,13 @@ class EditorPlugin : public Node {
AFTER_GUI_INPUT_CUSTOM,
};

enum ContextMenuSlot {
Copy link
Member

@KoBeWi KoBeWi Aug 2, 2024

Choose a reason for hiding this comment

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

This could be moved to EditorContextMenuPlugin instead.
EditorPlugin has similar enums, but they don't have a dedicated class.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 2, 2024

Looking at all the new methods in EditorData, I think they can be moved to some new class called EditorContextMenuManager 🤔 Like EditorInspector manages inspector plugins or 3D editor manages gizmo plugins etc.
Though they are completely internal, so this can be done in a future refactor.

@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 8d008a8 to 706bb90 Compare August 24, 2024 03:36
@citizenll
Copy link
Contributor Author

image
The scene tree and script editor plugin both receive input events simultaneously... How should I handle this...? When I use the same shortcut, they both respond at the same time, which means that the shortcuts in these two areas cannot be the same.
image

The code supporting shortcuts has been updated. Currently, it should restrict the scene tree and code editor from using the same shortcuts. If there are no other issues, I will supplement the documentation, and we can close this PR.

@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch 2 times, most recently from 244a586 to 6976f39 Compare September 1, 2024 13:59
<method name="remove_context_menu_plugin">
<return type="void" />
<param index="0" name="slot" type="int" enum="EditorPlugin.ContextMenuSlot" />
<param index="1" name="plugin" type="EditorContextMenuPlugin" />
Copy link
Member

@KoBeWi KoBeWi Sep 1, 2024

Choose a reason for hiding this comment

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

Needs description.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I think functionality-wise it's ok now and the implementation is good enough. It can be merged once you address some last comments.

@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 6976f39 to 8916bf0 Compare September 2, 2024 00:25
@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 8916bf0 to f0efc90 Compare September 2, 2024 09:52
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 3, 2024
@akien-mga akien-mga force-pushed the feat_context_menu_plugin4.x branch from f0efc90 to 98d3d1b Compare September 3, 2024 09:00
@akien-mga
Copy link
Member

Amended the commit message to match our preferred style for clarity.

@citizenll citizenll force-pushed the feat_context_menu_plugin4.x branch from 98d3d1b to 6b2348a Compare September 3, 2024 12:20
@AThousandShips AThousandShips changed the title Support adding custom items to editor right-click context menus Add support for custom items to editor right-click context menus Sep 3, 2024
@citizenll
Copy link
Contributor Author

Oh, what have I done? I thought I was just supposed to modify the commit message. Did my resubmission break anything?

@AThousandShips
Copy link
Member

You fixed the message but it had already been fixed, and your branch was behind this one so you pulled it back to an earlier version

@citizenll
Copy link
Contributor Author

Will my new force push affect this modification? Do I need to correct this error, or can I leave it as is? I'm not sure how to handle the pull request in this situation.

@AThousandShips
Copy link
Member

You can leave it, I updated the PR message to match, but it's good in the future to pay attention to what happens in your PR to avoid undoing or redoing things

@akien-mga
Copy link
Member

My bad, I should have been more explicit that I did the action myself, and wasn't asking for you to do it :)

@akien-mga akien-mga merged commit 79da448 into godotengine:master Sep 3, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

Add a way to add "Create new..." entries Support adding custom items to editor right-click context menus
4 participants