Allow hard-coded keyboard shortcuts in GUI controls to be customised

[EricEzaM]

Describe the problem or limitation you are having in your project:

In the Godot engine, a number of GUI controls have keyboard shotcuts 'built-in' to the C++ code. For example, in Text Edit, Copy and Paste are hard-coded to Ctrl + C and Ctrl + V respectively. This can cause issues and confusion when trying to rebind input that uses these hard-coded shortcuts.

Often these shortcuts give the impression that they can be rebound... Take a look at the Script Text Editor shortcuts in Editor Settings.

However, even if you change this binding, Ctrl + C and Ctrl + V still both work for copy and paste, since they are built in to the TextEdit code, which ScriptTextEditor uses under the hood.

Related issues:
godotengine/godot#29490
godotengine/godot#42139
godotengine/godot#12164
godotengine/godot#17927
godotengine/godot#26854
godotengine/godot#31739 (this is loosely related - but the comment @akien-mga makes at the end is relevant - "A recent example: in 28561 I added a feature to convert clipboard indentation on paste, but I can't have it overwrite Ctrl+V because that's hardcoded in TextEdit. The new API should allow overriding TextEdit's input management in CodeEdit and EditorCodeEdit."
godotengine/godot#20065 - Ideally this proposals would help make the built-in editor shortcuts more obvious.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

A method to be able to rebind these shortcuts which is completely separate to EditorSettings to avoid breaching godotengine/godot#29730.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

PLEASE NOTE: The implementation has changed a lot since this was first written. I am now using the InputAction system. Please read the latest comments on this post for a clearer picture of how it will work.

I have a test branch working already. Essentially, each GUI Control node would keep its own mini version of EditorSettings shortcuts which only applies to itself. Here is a mockup:

control.cpp

// *** Header *** 
HashMap<StringName, Ref<InputEvent>> built_in_shortcuts;

// *** cpp ***
void Control::_set_built_in_shortcut(const StringName &p_name, const Ref<InputEvent> &p_shortcut); // Protected
Ref<InputEvent> Control::get_builtin_shortcut(const StringName &p_name); // Public, or Protected if option 1) is chosen below.
void Control::override_builtin_shortcut(const StringName &p_name, const Ref<Shortcut> &p_shortcut); // Public. Checks if p_name is a registered shortcut first. Not needed if option 1) is chosen below.

From here, I think there are 2 options.

1. Controls which derive from Control define their own setters and getters for their own shortcuts. These would call _set_built_in_shortcut and get_builtin_shortcut internally. This would remove the need for override_builtin_shortcut In this example text edit has quite a few shortcuts so there would be a number of methods:

TextEdit::set_shortcut_copy(Ref<InputEvent> p_ev) {
	_set_built_in_shortcut("copy", p_ev);
}

// And other methods...
TextEdit::get_shortcut_copy();
TextEdit::set_shortcut_paste(...);
TextEdit::get_shortcut_paste();

2. Use a string based implementation. Instead of methods on the derived child, use the methods in Control to set the shortcuts. This would rely on strings (fragile) but would not require methods on the GUI control for every shortcut.

Either 1) or 2) would work... Built-in shortcuts don't change very often and C++ user's would not be adding them on the fly (like they can with EditorSettings), so this points to 1) being the better option as it does not rely on fragile strings and would be a more consistent and easy to understand API.

Ok, moving on... For brevity, I will use option 1) for the following examples.

Usage in nodes which derive from control, e.g. TextEdit

// Inside the Constructor, set the default key binding.
Ref<InputEventKey> ie;
ie.instance();
ie->set_control(true);
ie->set_keycode(KEY_C);
set_shortcut_copy(ie);

// Inside _gui_input, or wherever the shortcut needs to be checked:
if (get_shortcut_copy()->shortcut_match(p_ev)) {
	// < logic for shortcut >
}

// For comparison, the current implementation is essentially:
Ref<InputEventKey> k = p_ev;
if (k->get_keycode() == KEY_C && k->get_control() ) { // Oh dear... the shortcut can't be changed!
	// < logic for shortcut >
}

Usage when you want to override the shortcut... E.g. In ScriptTextEditor

// Inside ScriptTextEditor Constructor
code_editor->get_text_editor()->set_shortcut_copy(ED_GET_SHORTCUT("script_text_editor/copy", TTR("Copy"));
// Or, if you define the shortcut at the same time:
code_editor->get_text_editor()->set_shortcut_copy(ED_SHORTCUT("script_text_editor/copy", TTR("Copy"), KEY_MASK_CMD | KEY_C);

Now you can change the TextEdit shortcuts via an Editor Shortcut if you like, or you can just set a custom shortcut without using EditorSettings.

Note that ED_SHORTCUT is not being checked directly in the TextEdit, so if you make a change to EditorSettings it will no be used until the editor is restarted. I believe you can make the shortcuts update after a change if you handled NOTIFICATION_EDITOR_SETTINGS_CHANGED in notification() and just called set_shortcut_...(ED_GET_SHORTCUT(...)) again. Assuming the notification is called on shortcut change - that might have to be added, but this is a small detail overall.

[KoBeWi]

The main reason for hard-coded shortcuts is that regular shortcuts don't support echo events. godotengine/godot#36493 makes this issue much easier to fix.

[Paulb23]

You beat me to it!

Was abstracting AutoComplete thinking about how to handle shortcuts, came up with pretty much the same idea with a couple of differences:

• Keep it local to TextEdit and LineEdit rather then putting it in Control, as there are very few other shortcuts in /scene that do not already use ui_x actions. Unless there's a need to abstract it upwards for the editor?
• Enums rather then methods/strings, as I didn't want to add and bind 100s of methods to get/set shortcuts, or use strings, so it becomes: set_shortcut(SHORTCUT_PASTE, InputEvent); get_shortcut(SHORTCUT_PASTE);. Stored in a Vector<Ref<InputEvent>>.
• As @KoBeWi pointed out, where possible remove and use the standard system with echo support.

[EricEzaM]

@Paulb23

1. There are many controls with built-in shortcuts. TextEdit, LineEdit, GraphEdit, FileDialog and RichTextLabel are all that I could find in scene/gui. I guess it's not so many but re-implementing the same system on all of them would be a pain for maintenance.
2. Funny you say that! I had the exact same idea when discussing this with groud... As groud says, it might complicate the API for not much gain. Although I do see the benefit in it cutting down on the number of methods. Will have to get more feedback on this from reduz maybe.

EricEzaM:
Hmm
I just had this thought, might be shit:
How about if, to prevent a buttload of methods for shortcuts, they used an enum? TextEdit::Shortcuts::SHORTCUT_COPY
set_shortcut(TextEdit::Shortcut p_sc, Ref<InputEvent> p_event)
idk
Groud:
that's unusual, I'm not sure it is worth
I mean, unless you have like, more than 5 shortcuts, it might not be worth

3. @KoBeWi, echo has nothing to do with this PR. Echo is still fully supported even when using this system. To test, I implemented it for TextEdit's paste operation and it works just fine with Echoing pastes. As for KoBeWi's PR, that is specific to MenuButton, which does indeed disallow echo input. However, this is completely separate to that - simply doing shortcut->shortcut_match(p_event) has nothing to do with echo, or pressed, etc, so that would still need to be handled in a per-implementation basis. For example, if you only wanted to trigger the shortcut on release, you would do as the example below. Same goes with echo.

if (!p_ev->is_pressed()) {
	if (get_shortcut_copy()->shortcut_match(p_ev)) {
		// < logic for shortcut >
	}
}

[Paulb23]

Yeah agreed that re-implementing is a pain, though it doesn't feel like much of a burden as it's really only an extra Map / Vector, that's not exposed outside the class. The main advantage I can see is having an internaly consistent design.

The other problem is it being so heavily weighted, GraphEdit, FileDialog and RichTextLabel have around 3/4 shortcuts each, whereas TextEdit/CodeEdit and LineEdit will have easily 10+.

Assuming it does go into Control. A solution for enums is if we internally use something like _set_shortcut("paste", p_ev); then we could have a mixture, so GraphEdit etc can have one per method:

void set_shortcut_paste(const InputEvent &p_input_event) {
    _set_shortcut("paste", p_input_event);
}

Whereas TextEdit etc could have a wrapper using enums i.e

void set_shortcut(Shortcut p_shortcut, const InputEvent &p_input_event) {
    switch (p_shortcut) {
        case SHORTCUT_PASTE: {
           _set_shortcut("paste", p_input_event);
        } break;
    }
}

For the echo support, as you say it's not directly related, but rather we've had to hack some things into _input to grab the event and check the shortcut. Or is currently hardcoded for this reason. The echo support will hopefully allow us to remove some of these and use the standard system, so less shortcuts to support.

[groud]

Yes, to sum up my discussion with @EricEzaM. My opinion is that, if we have not a lot of shortcuts, we should expose getters/setters for each specific shortcuts. This make the API simpler. I don't know how many shortcuts textedit has, but my limit would be 5 for it being worth going for a string or enum based API.

However, I'm not against the fact that, internally, it would use strings (I think it's preferable compared to enums), to handle the internal logic.

So yeah, what @Paulb23 suggests sounds good to me.

[EricEzaM]

Personally I prefer consistency - so I am not a huge fan of mixing the styles. At this stage I would probably lean towards using enums over strings and just store them in a HashMap<int, Ref<InputEvent>>

Also, if we put the logic in Control then users can use the system to set up customisable shortcuts on their own custom controls.

I have done an initial look at TextEdit and below are the keyboard shortcuts I can find. I actually think this system will do a huge favour for cleaning up the codebase too - at the moment we have #ifndef APPLE_STYLE_KEYS all over the shop in TextEdit::_gui_input and sometimes it is really hard to tell what it does when you are just perusing the code. This would move that APPLSE_STYLE_KEYS logic to the place where you set the default shortcuts (i.e. in the constructor) and the _gui_input method would be much cleaner, with far less ifs, switches and various branching based on what is needed.

/*
	* Completion Index Up
	* Completion Index Down
	* Completion Index Page Up
	* Completion Index Page Down
	* Completion Index First
	* Completion Index Last
	* Completion Confirm
	*
	* New Line Below
	* New Line Above
	* Escape
	* Indent
	* Dedent
	* Backspace
	* Left + Keypad Variant
	* Right + Keypad Variant
	* Up + Keypad Variant
	* Down + Keypad Variant
	* Line Start (Home) + Keypad Variant
	* Line End (End) + Keypad Variant
	* Page Up + Keypad Variant
	* Page Down + Keypad Variant
	* Delete
	* Select All
	* Paragraph Start
	* Paragraph End (Control + E on Apple?)
	* 
	* 
	* Cut
	* Copy
	* Paste
	*
	* Undo
	* Redo
	*
	* Query Completion (e.g. Ctrl + Space)
	* Open Context Menu
	*
	* Toggle Insert Mode
	* 
	*/

[EricEzaM]

How would you deal with actions that have multiple shortcuts? E.g. confirming a completion option has Enter, Keypad Enter and Tab.

Same thing with Control+H = Backspace on apple keys.

The easiest way would be to just have alternate shortcuts included in the enum...

SHORTCUT_COMPLETION_CONFIRM
SHORTCUT_COMPLETION_CONFIRM_ALT1
SHORTCUT_COMPLETION_CONFIRM_ALT2

That is pretty messy though. I don't think storing a Vector<Ref<InputEvent>> would work either, as you want to be able to overwrite them on an individual basis. Maybe there needs to be some sort of wrapper around InputEventKey which allows multiple keycodes to be set? And when you check for a shortcut it checks all of them, and if any of them match then it is valid?

Edit: Actually I think you could store a Vector<Ref<InputEvent>>.

set_builtin_shortcut(int p_shortcut, Vector<Ref<InputEvent>> p_shortcuts); Then when it comes to checking the shortcuts you just loop thought the vector and if one of the matches, then it's a match.

[Paulb23]

The downside of enums is avoiding conflicts when you extend the class. If this is not exposed to the user then we don't have to worry as we can handle it in engine like so:

TextEdit::Shortcut {
 SHORTCUT_PASTE,
 SHORTCUT_TEXTEDIT_MAX 
}

CodeEdit::Shortcut {
  SHORTCUT_COMMENT_LINE = TextEdit::Shortcut::SHORTCUT_TEXTEDIT_MAX
}

Otherwise internally it will have to be Strings, to avoid from a user perspective, weird behaviour when they register their own shortcut and it overwrites the paste one.

For multiple shortcuts with the same action, was thinking exactly that, multiple entries. There shouldn't be many where this is needed.

Not sure I follow with not being able to overwrite them. The enum value is the vector index, so you can go

Vector<Ref<InputEvent>> shortcut_list;  
shortcut_list[TextEdit::Shortcut::SHORTCUT_PASTE]

Yeah I'm looking forward to cleaning up that method, especially with all the remapping. That looks like a solid list, couple are missing that I can think of off the top of my head:

clear_text
scroll_lines_up
scroll_lines_down
delete_word
delete_previous_word
start_of_text
end_of_text

And then CodeEdit will have some of the ones currently sitting in the editor, such as comment_line, fold_all etc.

[EricEzaM]

For multiple shortcuts with the same action, was thinking exactly that, multiple entries. There shouldn't be many where this is needed.

Unfortunately there are quite a few. A few examples include ENTER and KP_ENTER being interchangeable, Ctrl + C and Ctrl + Insert, as well as Ctrl V and Shift Insert. You also have things like for apple keys, Control + F/B/P/N/D/H for right, left, up, down, delete and backspace, respectively. Then don't forget there is also the Numpad version of those when numlock is off: 6, 4, 8, 2, preiod for R/L/U/D/delete and there is also Home/End and PgUp/PgDn there too!

There are more to be found for sure. As a result I think it would be pretty messy to have multiple entries/alternates for so many. And some would only have 2 alts, some would have 3... etc. I will see how it goes and maybe if I can get something nice-looking that uses something like HashMap<int, Vector<Ref<InputEvent>>> so multiple shortcuts can be stored against one index.

[Paulb23]

Hmm, having to pass in a list of InputEvents doesn't feel as clean, random idea but could do something like:

void set_shortcut(Shortcut p_shortcut, const InputEvent &p_input_event, const Vector<InputEvent> &p_alt_input_events = null);

Otherwise yeah, not to sure how else to handle it. Even with separate methods that doesn't feel quite right. set_paste_shortcut() and set_paste_shortcut_alt().

As for treating KP_4 the same as KEY_LEFT means we either, check the input events ourselves, inject these alternatives into the list of InputEvents, or don't even bother and leave it up to the user to add them.

Can store them in a Vector<Vector<Ref<InputEvent>> ;)

[EricEzaM]

Current iteration of work-in-progress API is:

HashMap<int, Vector<Ref<InputEvent>>> built_in_shortcuts;

// Having the user to construct the vector themselves seemed like a massive pain on the user. This limit's it to 5 alternates, but it's more convenient for the end-user.
void _set_built_in_shortcut(const int &p_idx, const Ref<InputEvent> &p_shortcut, const Ref<InputEvent> &p_alt_shortcut1 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut2 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut3 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut4 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut5 = Ref<InputEvent>());

// Loops through shortcuts and returns true if one matches
bool match_builtin_shortcut(const int &p_idx, const Ref<InputEvent> &p_event) const;

void override_builtin_shortcut(const int &p_idx, const Ref<InputEvent> &p_shortcut, const Ref<InputEvent> &p_alt_shortcut1 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut2 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut3 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut4 = Ref<InputEvent>(), const Ref<InputEvent> &p_alt_shortcut5 = Ref<InputEvent>());

I also added a convenience method in InputEventKey for quickly generating Ref<InputEventKey>.

// This method is static
Ref<InputEventKey> InputEventKey::create_reference(uint32_t p_keycode, uint32_t p_modifier_masks) {
	Ref<InputEventKey> ie;
	ie.instance();
	ie->set_keycode(p_keycode);

	if (p_modifier_masks & KEY_MASK_SHIFT) {
		ie->set_shift(true);
	}
	if (p_modifier_masks & KEY_MASK_ALT) {
		ie->set_alt(true);
	}
	if (p_modifier_masks & KEY_MASK_CTRL) {
		ie->set_control(true);
	}
	if (p_modifier_masks & KEY_MASK_CMD) {
		ie->set_command(true);
	}
	if (p_modifier_masks & KEY_MASK_META) {
		ie->set_metakey(true);
	}

	return ie;
}

//USAGE

ie = InputEventKey::create_reference(KEY_ENTER);
Ref<InputEventKey> ie2 = InputEventKey::create_reference(KEY_KP_ENTER);
Ref<InputEventKey> ie3 = InputEventKey::create_reference(KEY_TAB);
_set_built_in_shortcut(SHORTCUT_COMPLETION_CONFIRM, ie, ie2, ie3);

// What I had to do before... yuck
Ref<InputEventKey> ie;
ie.instance();
ie.set_keycode(KEY_ENTER);

Ref<InputEventKey> ie2;
ie2.instance();
ie2.set_keycode(KEY_KP_ENTER);

Ref<InputEventKey> ie3;
ie3.instance();
ie3.set_keycode(KEY_TAB);

// Also easy to add modifiers:
ie = InputEventKey::create_reference(KEY_ENTER, KEY_MASK_SHIFT | KEY_MASK_CMD);

[Paulb23]

That looks good to me, multiple args should be okay.

Couple of questions:

• I'm assuming we are putting setters on the nodes, as Control will not know about extended class enums, what the use of override_builtin_shortcut?
• Similarly for match_builtin_shortcut with enums, the usage will have to be routed through the extended nodes, so is this protected?

[EricEzaM]

• At the moment I do not have any separate methods on the GUI nodes like, for example, set_shortcut_copy, etc. I am working through TextEdit at the moment and my list of shortcut actions has grown significantly since I last posted it, so I am just directly calling the control set_ method. Below is an example. Yes, the Control class does not know about the extended class enums but since all it stores is a map with int as the key, it doesn't have to. As you pointed out before, this can cause issues when extending through GDScript and even in C++ it is not so convenient - I am not sure what the best plan of action here is. What is convenient though, is having one big named list of all the shortcuts (i.e. the enum). It is super helpful to know exactly what shortcuts there are. Maybe we could keep the enum for convenience, but map each item to a string representation, and then pass that to _set_built_in_shortcut which would take a string rather than an int as it's key. Mapping an enum to string is done in keyboard.cpp, in the _keycodes array. Maybe we could copy something like that?

TextEdit::TextEdit() {
	// ...
	ie = InputEventKey::create_reference(KEY_ENTER, KEY_MASK_SHIFT | KEY_MASK_CMD);
	ie2 = InputEventKey::create_reference(KEY_KP_ENTER, KEY_MASK_SHIFT | KEY_MASK_CMD);
	_set_built_in_shortcut(SHORTCUT_NEW_LINE_ABOVE, ie, ie2);
	// ...
}

• The use of override_ is to specifically override and existing shortcut. It first checks if a shortcut with that key exists, and if it does then it set's it, otherwise it returns. Since this is the public method (as opposed to set_ it stops users from accidentally setting builtin shortcuts when they shouldn't be allowed to.

void Control::override_builtin_shortcut(const int &p_idx, const Ref<InputEvent> &p_shortcut, const Ref<InputEvent> &p_alt_shortcut1, ... etc) {
	if (!data.built_in_shortcuts.has(p_idx)) {
		return;
	}

	_set_built_in_shortcut(p_idx, p_shortcut, p_alt_shortcut1, ... etc);
}

• match_builtin_shortcut will be protected, yes. It's only use case is from within derived classes so that makes sense.

[EricEzaM]

Final list of shortcuts... wow. I'm done with TextEdit almost I think. I'll put up a draft PR soon so you can see what the code looks like. I find TextEdit much easier to follow now, especially after putting a lot of logic which was in _gui_input into their own methods.

	// All Shortcuts. Default keybinds shown in comment. "SELECT" variants use SHIFT mask.
	enum Shortcuts {
		// AutoComplete
		SHORTCUT_COMPLETION_QUERY, // CMD SPACE (ALT SPACE on Apple)
		SHORTCUT_COMPLETION_INDEX_UP, // UP
		SHORTCUT_COMPLETION_INDEX_DOWN, // DOWN
		SHORTCUT_COMPLETION_INDEX_PAGE_UP, // PGUP
		SHORTCUT_COMPLETION_INDEX_PAGE_DOWN, // PGDN
		SHORTCUT_COMPLETION_INDEX_FIRST, // HOME
		SHORTCUT_COMPLETION_INDEX_LAST, // END
		SHORTCUT_COMPLETION_CONFIRM, // ENTER, TAB
		SHORTCUT_COMPLETION_CANCEL, // ESC
		SHORTCUT_COMPLETION_CLEAR_HINT, // ESC

		// Newlines
		SHORTCUT_NEW_LINE_BELOW, // CMD ENTER
		SHORTCUT_NEW_LINE_BELOW_SPLIT_CURRENT, // ENTER
		SHORTCUT_NEW_LINE_ABOVE, // CMD SHIFT ENTER

		// Indentation
		SHORTCUT_INDENT, // TAB
		SHORTCUT_DEDENT, // CMD TAB

		// Backspace and Delete
		SHORTCUT_BACKSPACE, // BKSPC
		SHORTCUT_BACKSPACE_WORD, // CMD BKSPC (ALT BKSPC on Apple)
		SHORTCUT_BACKSPACE_ALL_TO_LEFT, // None (CMD BKSPC on Apple)
		SHORTCUT_DELETE, // DEL
		SHORTCUT_DELETE_WORD, // CMD DEL (ALT DEL on Apple)
		SHORTCUT_DELETE_ALL_TO_RIGHT, // None (CMD DEL on Apple)

		// Cursor Movement left/right
		SHORTCUT_CURSOR_LEFT, // LEFT
		SHORTCUT_CURSOR_WORD_LEFT, // CMD LEFT (ALT LEFT on Apple)
		SHORTCUT_CURSOR_SELECT_LEFT,
		SHORTCUT_CURSOR_SELECT_WORD_LEFT,

		SHORTCUT_CURSOR_RIGHT, // RIGHT 
		SHORTCUT_CURSOR_WORD_RIGHT, // CMD RIGHT (ALT RIGHT on Apple)
		SHORTCUT_CURSOR_SELECT_RIGHT,
		SHORTCUT_CURSOR_SELECT_WORD_RIGHT,

		// Cursor Movement up/down
		SHORTCUT_CURSOR_UP, // UP
		SHORTCUT_CURSOR_DOWN, // DOWN
		SHORTCUT_CURSOR_SELECT_UP,
		SHORTCUT_CURSOR_SELECT_DOWN,

		// Cursor Movement Line start/end
		SHORTCUT_CURSOR_LINE_START, // HOME (CTRL A on Apple, CMD LEFT on Apple)
		SHORTCUT_CURSOR_LINE_END, // END (CTRL E on Apple, CMD RIGHT on Apple)
		SHORTCUT_CURSOR_SELECT_LINE_START,
		SHORTCUT_CURSOR_SELECT_LINE_END,

		// Cursor Movement Page up/down
		SHORTCUT_CURSOR_PAGE_UP, // PgUp
		SHORTCUT_CURSOR_PAGE_DOWN, // PgDn
		SHORTCUT_CURSOR_SELECT_PAGE_UP,
		SHORTCUT_CURSOR_SELECT_PAGE_DOWN,

		// Cursor Movement document start/end
		SHORTCUT_CURSOR_DOCUMENT_START, // CMD HOME (CMD UP on Apple)
		SHORTCUT_CURSOR_DOCUMENT_END, // CMD END (CMD DOWN on Apple)
		SHORTCUT_CURSOR_SELECT_DOCUMENT_START,
		SHORTCUT_CURSOR_SELECT_DOCUMENT_END,

		// Scrolling
		SHORTCUT_SCROLL_LINES_UP, // CMD UP (+ ALT on Apple)
		SHORTCUT_SCROLL_LINES_DOWN, // CMD DOWN (+ ALT on Apple)

		// Select all, Cut, Copy, Paste
		SHORTCUT_SELECT_ALL, // CMD A
		SHORTCUT_CUT, // SHIFT DEL, CMD X
		SHORTCUT_COPY, // CMD C, CMD INS
		SHORTCUT_PASTE, // CMD V, SHIFT INS

		// Undo/Redo
		SHORTCUT_UNDO, // CMD Z
		SHORTCUT_REDO, // CMD SHIFT Z, CMD Y

		// Misc
		SHORTCUT_OPEN_CONTEXT_MENU, // MENU
		SHORTCUT_TOGGLE_INSERT_MODE, // INS
	};

[EricEzaM]

I have switched from using Ref<InputEvent> to Ref<Shortcut> for storage.

An issue arose where when using override_builtin_shortcut(int, Ref<InputEvent>) because if the the override was from the editor ED_GET_SHORTCUT()->get_shortcut() then the editor needed to be restarted for the change to take effect. This is because it was storing the Ref to the InputEvent, not the ref to the Shortcut. Shortcut's keep the same memory address even when their shortcut InputEvent is changed. So now you can change the shortcut from EditorSettings and a restart will not be required.

[EricEzaM]

How big of a problem is this...?

In TextEdit, by default 'paste' has 2 shortcuts. Command + V and Shift + Insert. ScriptTextEditor has a customisable editor shortcut for paste, script_text_editor/paste.

Now, of course this only allows you to set one shortcut in the editor. When you use
override_builtin_shortcut(TextEdit::SHORTCUT_PASTE, ED_GET_SHORTCUT("script_text_editor/paste") you are effectively removing the second shortcut, Shift + Insert since you are overriding the builtin shortcuts with just one option. Is this fine?

[Paulb23]

Yeah if Control is going to have public methods then using strings is definitely the better choice. Though would still keep the mapping local to the nodes. With that said, I'm not a big fan of having public methods in Control, would rather make them private and force usage to go though wrapper methods in each node. Wrapper methods also means we can remove override_builtin_shortcut.

How big of a problem is this...?

We will end up calling override_builtin_shortcut for these on launch when the editor settings are restored, taking away all alternative shortcuts. So without dropping alt shortcuts altogether, I guess we can keep some hardcoded or add a way in editor to configure these. Quickest way I can think of, though not the best solution, would be to add a setting like autocomplete_on_tab.

[EricEzaM]

Yeah those sound like good changes to make.

One thing I am concerned about though: currently TextEdit has 55 shortcuts... this is easy to have visibility of in an Enum, but if we switch to strings it will then be much harder to track what built-in shortcuts exist.

One way to get both systems is to use something that maps the enum to a string... like perhaps a static const string _shortcutStrings[] = {"completion_query", "completion_index_up", etc} but I think this would be a bit annoying to maintain.

Another option which is a bit more 'hackish' would be to use a macro TO_STRING(x) #x to convert from a name to a string.

There is also the case of documenting these built in shortcuts... Users should know what the builtin shortcuts are and what strings are used to store them.

[Paulb23]

Random thought that I glossed over originally, but is there any reason we can't use actions / input_map, other then there being a lot of them, rather then building a different system? The only issues I can foresee are:

• Needing a way to edit the editor input map.
• Support for different platform defaults, quick way around this is to add new actions for these and #ifdef in text_edit for the action.

That said, Agreed, enums are great for documentation purposes, they are also available for the user to browse, whereas strings will not be. Other then manually adding the strings to the documentation, the easiest way, is as you mentioned, have an array of strings.

[EricEzaM]

I think the issue with using InputMap is that there are a lot of one-off shortcuts, like the completion_* ones which are really only used in TextEdit. Additionally, the shortcut list is currently very verbose with things like:

SHORTCUT_CURSOR_LINE_START, // HOME (CTRL A on Apple, CMD LEFT on Apple)
SHORTCUT_CURSOR_LINE_END, // END (CTRL E on Apple, CMD RIGHT on Apple)
SHORTCUT_CURSOR_SELECT_LINE_START,
SHORTCUT_CURSOR_SELECT_LINE_END,

I don't think this would look good on an input map. Now, granted, the above is an example of a shortcut that would very rarely want to be changed. I have included them as such so that all shortcuts follow the same system. This makes the code very simple and easy to understand.

However, I feel like having 55 shortcuts which include _SELECT_ variants for evert cursor movement option is way overkill for something that will essentially never be different.

One thing I am thinking of is instead of having all these different _SELECT_ variants, just have one keybind which is set to something like selection_modifier. Or even just always have the selection modifier as shift. Does anyone or any keyboard layout/culture even use anything other than shift for selection?

if (_match_builtin_shortcut(SHORTCUT_CURSOR_LINE_START, p_gui_input) && p_gui_input->get_shift()) {
	_move_cursor_to_line_start(p_select = true);
}

EDIT: Never mind, that won't work... shortcut_match(p_event) checks modifiers too so if you pass in an event with shift and the shortcut doesn't have shift, it will return false.

[EricEzaM]

Are we just complicating it too much by having the requirement that this functionality should be able to be extended in GDScript?

If we assume this should only be able to be used in C++, and not exposed to the user, then it simplifies it a bit. We can use Enums for storage and don't have to worry about storing the shortcut identifiers as strings.

Users would still be able to override the shortcuts if they wanted, using _override_default_shortcut(TextEdit::Shortcut sc, ...) but would not be able to add their own. Maybe we don't even expose every shortcut... I don't really see why left/right/up/down/delete/backspace etc should be exposed. Actually I think the only ones needing to be exposed are:

Completion Query
Select All
Cut
Copy
Paste
Undo 
Redo

The rest are stock-standard navigation and text manipulation shortcuts which have been assigned shortcuts which maintain consistency with the selected operating system (i.e. for mac, https://support.apple.com/en-au/HT201236#text)

[EricEzaM]

Here is the diff between current master and my WIP branch

• Uses enums (int) to store shortcuts rather than strings.
• override_builtin_shortcut is now on the nodes (e.g. TextEdit, GraphEdit), and not directly on control
• Only some shortcuts are exposed to scripting so extremely fundamental actions (e.g. moving cursor) are not allowed to be changed via script. However, all keyboard actions still use this system for consistency and simplification of logic.
• TextEdit diff is large but really all I did is move implementation into their own methods rather than it floating around in TextEdit::_gui_input
• You can see the shortcut matching logic at around line 3100 in TextEdit
• You can see where shortcuts are set at around line 6700 in TextEdit
• The diff in text edit is quite hard to follow. :(

I have tested all shortcuts on Windows and it is all working well for me. Would need someone to test on mac though.

https://github.com/godotengine/godot/compare/master...EricEzaM:WIP%2Foverridable-builtin-shortcuts?diff=split&expand=1

[Paulb23]

only used in TextEdit
I don't think this would look good on an input map

This is more a UX issue then anything, could solved by allowing you to hide sections in the UI, and / or sub sections. Which is why I'm not too worried about that. We also have some actions already such as ui_x that we can reuse for some shortcuts, ie ui_up for completion_index_up and ui_cancel, ui_page_down ect so we don't have to create brand new actions for everything in TextEdit. There's also the shared ones such as paste that will be used in FileDialog. One thing I'm sure of though, is we shouldn't have a mixture of the two.

Currently it feels like we are re-inventing the same thing, especially with allowing alt shortcuts in the editor etc. I guess the main contention point is how separate text editing should be to the rest of the UI nodes.

I don't think we are exposing too much, as some users want to add for example, vim controls. So in this case they would need to be able to remap caret move keys to: h,j,k,l. But, yes I don't see a use case for extending it in GDScript, as the user has full control of this side anyway, it's more so about extending it in engine.

Took a very quick glance and it looks amazing, the code is so much easier to follow!

[EricEzaM]

Ok this is heading towards a much broader discussion of input in general, but it's good to have.

This could probably do with a bigger overhaul of the input map editor for both Project and Editor Settings too... Something I have also been considering.

Anyway, currently existing ui_* shortcuts would be able to replace every Completion shortcut except COMPLETION_QUERY (cmd + space).

Additionally, we could shave off many SELECT variants if we just use assume shift is always used for the selection modifier when performing actions which move the cursor around.

Ref<InputEventKey> k = p_event;
k = k->duplicate();
bool shift_pressed = k->get_shift();
// Remove shift or else actions will not match
k->set_shift(false);

if (k->is_action_pressed("ui_text_cursor_left")) {
	_move_cursor_left(p_select = false);
}
if (k->is_action_pressed("ui_text_cursor_left") && shift_pressed) {
	_move_cursor_left(p_select = true);
}

As in my example above, I would imagine this entailing adding a number of built-in input maps for the various built-in nodes and things around the editor. I think this would be the exhaustive list. LineEdit would use the ui_text_* actions, and things like RichTextLabel could use the 'global' ui actions of page up/down, copy, etc, since all it uses is basic navigation and copying.

// Shortcuts for text-based nodes only
ui_text_newline
ui_text_newline_blank // (Cmd + Enter, enters a new line without splitting the current one at the cursor)
ui_text_newline_above

ui_text_indent
ui_text_dedent

ui_text_backspace
ui_text_backspace_word
ui_text_backspace_all_to_left

ui_text_delete
ui_text_delete_word
ui_text_delete_all_to_right

ui_text_cursor_left
ui_text_cursor_word_left

ui_text_cursor_right
ui_text_cursor_word_right

ui_text_cursor_up
ui_text_cursor_down

ui_text_cursor_line_start
ui_text_cursor_line_end

ui_text_cursor_page_up
ui_text_cursor_page_down

ui_text_cursor_document_start
ui_text_cursor_document_end

ui_text_scroll_up
ui_text_scroll_down

ui_text_select_all
ui_text_toggle_insert_mode

// Graph Edit
ui_graph_duplicate
ui_graph_delete

// FileDialog
ui_filedialog_up_one_level
ui_filedialog_refresh
ui_filedialog_show_hidden

// And then some shortcuts which I think could be global for ui, for text, graphs, etc
ui_cut
ui_copy
ui_paste

ui_undo
ui_redo

I think these would go into a separate section of Editor Shortcuts/Project Input Map where only these built-ins are shown. That would make it very clear to the user that these shortcuts are built-in to the engine.

One last thing to handle would be overriding these on a case-by-case basis... for example, in the Editor, the ScriptTextEditor has a shortcut set for Copy and Paste which are both customisable. However, this is independent and completely separate to the copy and paste inside TextEdit. Should they remain separate? If someone wanted to NOT used Ctrl+C for copy in ScriptTextEditor, should they have to change both that shortcut AND the built-in one ui_copy? I think that would have to be the case - I can't see it another way.

[EricEzaM]

I updated my branch with a new commit which uses input actions instead. I used the action list from my previous comment as the basis.

View it here:
https://github.com/godotengine/godot/compare/master...EricEzaM:WIP%2Foverridable-builtin-shortcuts?diff=split&expand=1

• Shortcuts were added only for the editor for now, in InputMap::load_default().
• All set_shortcut, match_shortcut and override methods were removed from Control and TextEdit.
• There is no longer _SELECT_ variants of the shortcut - shift is used in TextEdit for selection (hardcoded)

I think this is heading in the right direction. However one thing this does make harder is overriding shortcuts on an individual basis. For example, user wants to change copy shortcut from Ctrl C to Ctrl Shift Q. Now they need to change it in both Shortcuts/ScriptTextEditor, Shortcuts/VisualScriptEditor, and ui_copy... I guess one solution to this specific issue is to just make ScriptTextEdit (and others) common shortcuts like cut, copy, paste, etc NOT exposed to the editor shortcuts, and to just use ui_copy instead.

I think to go along with this, changes need to be made in the Input Map editor and Editor Shortcuts editor. Firstly builtin shortcuts should have their own section. I don't think hiding them is that great of an option. I would prefer a separate toggle-able section... Hacked together mockup:

[Paulb23]

Yeah 4.0 is a great time to get this sorted. Ahh, I was referring to the project input_map UI not the editor one, and being able to hide the default entries there, there's a ongoing issue about it here #303. Fixing that though is a secondary issue. For the editor however, that looks good, was thinking about a separate tab, but I can see having two shortcut tabs getting confusing.

Assuming, shift for selection should be fine, we can always make it an option later if there is a good use case for it. Otherwise, that looks solid. Only change I can see is perhaps one for ui_menu_key. As a side note, not currently in place, but I'm starting to rename references to the text cursor to caret wherever possible.

For the "customisable" ScriptTextEditor shortcuts like copy etc, I agree, would remove them. Making the drop down menus use the first one in the action list is good enough.

To expand on my earlier point, the downside of the input_map is different platform defaults, as it's linked to the project not the platform. The editor is pretty easy to solve as we can control that. But for games, if its created on a windows device where ui_text_caret_line_start is defaulted to the HOME key, then that will also carry over to Apple devices, rather then CMD LEFT. The only way I can think of is to have a ui_text_apple_caret_line_start option as well in these cases. As you mentioned these shouldn't be changed that much so while a little odd, should be okay?

[EricEzaM]

How did it work currently for things like TextEdit? The ifdefs for OSX_ENABLED and APPLE_STYLE_KEYS were directly in the _gui_input method, so how did that work on exported projects?

I don't really know how exporting works in Godot... did the Export Template for mac enable these pre-processor variables or something so TextEdit would compile with the right shortcuts? If I was developing on a windows machine and exporting for mac would the shortcuts be correct?

[Paulb23]

The "Export Templates" is a compiled version of Godot without /editor for each platform. So as they were hardcoded, the MacOS version of TextEdit would be compiled with those ifdefs active, whereas other platforms would not. A exported project, is that binary with your project data in a .pck.

[EricEzaM]

The input map is just a specialised view for "input" project settings. Normal project settings can be overridden for certain export templates... could the same thing be done for the input map?

So you would have only one action, but then underneath that you would have multiple assigned keys, and you could limit them to specific platforms.

[EricEzaM]

Ok so I just did a quick test in 3.2.2:

Make 2 input actions, one called test and one called test.HTML5. Set them to different keybinds. Make a scene for testing, I just did a color rect and changed the color when action pressed:

func _process(delta):
	if Input.is_action_pressed("test"):
		color = Color.red
	else:
		color = Color.white

Play the scene normally - the key mapped to 'test' works normally (I set it to W). Now, Go to Project > Export... and select HTML5. This should be enough to show the preview icon for HTML5. If you preview in HTML5 the normal test keybind will no longer work and test.HTML5 keybind will (I set mine to H). When I press H in html5, the rect goes red. When I press W nothing happens, as expected. So, awesome! this functionality of overriding on a per-export basis is already built in to Godot!

My input map:

All we really need to do is add some UI to make overrides for specific input actions. And, of course, add the defaults to the ProjectSettings constructor where the input actions are set.

[Paulb23]

Fantastic, I'm not too familiar with how that side works, but if we can do that, problem solved! I think this definitely is the right direction to take this now.

[EricEzaM]

Ok, I think I have this in a pretty good state right now. A lot of changes had to be made to the Action Editor so I could reuse it for EditorSettings (previously it was coupled to ProjectSettings). This led to a complete overhaul of the UI for editing actions to address a number of other issues too. I think this PR should close 10+ issues.

I also had to make sure that the overrides were correctly serialising to and loading from the editor_settings.tres. Which took a little while to get right :P

Below is an example of ScriptTextEditor using the builtin shortcut for ui_undo in it's menu. Now you can fully customise built-in shortcuts.

Here is a longer demo of the same interface reused in ProjectSettings.

Latest changes are pushed to my branch (diff here: https://github.com/godotengine/godot/compare/master...EricEzaM:WIP%2Foverridable-builtin-shortcuts?diff=split&expand=1) which can be built and ran if you want to take a look.