godot
godot copied to clipboard
[macOS] Add native "Edit" menu.
- Adds default "Edit" menu (and some extra functions for native menu to show it after "Scene" in the editor).
- Adds handlers for native cut/copy/paste actions to the content view (redirected as key presses to Godot event handler).
Fixes https://github.com/godotengine/godot/issues/97437 (~I was not able to reproduce crash, but copy-paste now works in native file dialogs~, Edit: reproduced with updated info and added a fix).
Adding a default Edit menu with copy paste etc sounds good, especially for the editor. But how will this interact with setting up a NativeMenu Edit menu of your own? Will it be possible to add/modify callbacks or items in this Edit menu or opt out of it entirely if you want to make your own?
I have an Edit menu set up in my project with copy/paste and undo/redo functionality and I don't want that doubled up or overwritten.
Adding a default Edit menu with copy paste etc sounds good, especially for the editor. But how will this interact with setting up a NativeMenu Edit menu of your own? Will it be possible to add/modify callbacks or items in this Edit menu or opt out of it entirely if you want to make your own?
Like with the other default system menus, it's possible to add new items (by using get_system_menu(EDIT_MENU_ID) instead of create_menu() or attaching PopupMenu to a system menu), but not to modify remove default items. Default "Cut"/"Copy"/"Paste" simply send "ui_cut"/"ui_copy"/"ui_paste" actions to the active window, so it can be customized as well.
Ok, in my case I wouldn't like to remove the cut/copy/paste items. But I would like to change their callbacks to I can hook them into my own menu item object actions, and I would also like to enable/disable them as I do selection and clipboard content validation to check if the use can currently copy/cut/paste that disabled/enables the corresponding items.
If this is not possible it would be better if there was a project setting or way to just remove the default Edit menu, or at least hide it to be able to use my own.
Opened https://github.com/godotengine/godot/pull/97678 for the crash fix only.
Will probably update this one to move editor "Undo"/"Redo" to "Edit" (where it usually is), but this will require at least some default item customization capabilities.
For custom validation (when you can copy/paste/undo/redo) or custom undo action names in the menu items I guess you'd need to have some system similar to the delegate system in macOS where you supply reference to an object the engine can call to get this information when it needs it. But as far as I know Godot doesn't have a pattern like this to provide custom information suppliers, and this case might be a little specific to add one. But it's a very useful pattern in many GUI-related cases, so it could be worth considering how something like that could be introduced in Godot for other purposes as well. And if something like this was introduced it could also be used to handle the actual copy/paste/undo/redo actions if someone wants to override those instead of using the default implementations.
I guess you'd need to have some system similar to the delegate system
Since Godot is not using anything similar, I have added can_cut, cut, etc. callbacks to the NativeMenu.
Example
func _copy():
print("!copy!")
func _can_copy():
return $CheckBox.button_pressed
func _ready() -> void:
NativeMenu.can_copy = _can_copy
NativeMenu.copy = _copy
It might be worth implementing some sort of delegate system, but it out of the scope of this PR and probably deserve a separate proposal.
Since Godot is not using anything similar, I have added
can_cut,cut, etc. callbacks to theNativeMenu.
Sounds like a good compromise, and I guess you'll add similar ones for undo/can_undo/undo_action_description etc? I know you like to keep things short, but it might still be worth adding "callback" to these (as in can_copy_callback instead of can_copy) to clarify that these are callbacks, and not confuse them with the current state of the menu item. Especially since this will be a bit of an unusual pattern with callbacks requesting return values.
I guess you'll add similar ones for undo/can_undo/undo_action_description etc?
No, with the way editor undo/redo is currently implemented, this is pretty much impossible to implement without breaking the editor.
I guess you'll add similar ones for undo/can_undo/undo_action_description etc?
Done. Currently, it's not fully integrated with the editor, "Edit" menu items work in editor (as generic content independent Undo/Redo), but there are duplicate context specific menu items in the local menus (same is true for Copy/Cut/Paste). Which is not perfect, but I'm not sure what's the best solution for this.
I know you like to keep things short, but it might still be worth adding "callback" to these
I do not think any of Callable properties have _callback suffix, it's only done for getter/setter functions (which is true for this PR as well).
I guess you'll add similar ones for undo/can_undo/undo_action_description etc?
Done.
Nice. I see the undo_description and redo_description are now string properties, should these not be callbacks returning strings to be called on menu validation (before menu is opened) just like can_undo and can_redo to ensure they're updated at the correct time?
should these not be callbacks returning strings to be called on menu validation (before menu is opened) just like can_undo and can_redo to ensure they're updated at the correct time
Actually, this is probably unnecessary as well, and can be simplified to bool properties as well.
Actually, this is probably unnecessary as well, and can be simplified to
boolproperties as well.
As long as they can all be set during a popup_open_callback for the native Edit menu either way would be fine I guess, but I think it should be consistent. Either setting all of these overrides as properties or hooking up callbacks where you supply the values when they're needed. I think using callbacks would make it clearer that you're overriding the default implementation, and it would be clear the values you return from these callbacks would be used. With just bool and string properties I would be uncertain when these would/should be set and it would also invite expectations of getting the current state using these parameters and not just setting it to override default behavior.
Changed can_* properties to bool. Both can be set from popup_open_callback. In this case, callback seems like overkill.
Needs rebase.
I can't test (no Mac) but if it matches user expectations, I think it's good to merge. CC @Calinou @adamscott