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

Remove hardcoded shortcuts from /scene and instead use the input action system to allow them to be customised. #43663

Merged

Conversation

EricEzaM
Copy link
Contributor

  • Made shortcuts which were previously hardcoded customisable. (this is just the backend implementation - the UI to do so will come in a later PR)
  • Shortcuts that fall into this category are things like cut, copy paste, etc on TextEdit. In the code, these were defined in gui_input with things like
    if (k->is_pressed() && k->get_keycode() == KEY_C && k->get_command() == true){ /* copy */}
  • Cleaned up logic... it is much more readable now. The below code example handles 1) moving the cursor left 1 char, 2) moving the cursor left by 1 word 3) moving the cursor to the start of the line (on Apple only) and finally 4) doing the previous 3 with selection (shift) pressed.

godot/scene/gui/text_edit.cpp

Lines 2876 to 2945 in 07112b9

case KEY_LEFT: {
if (k->get_shift()) {
_pre_shift_selection();
#ifdef APPLE_STYLE_KEYS
} else {
#else
} else if (!k->get_alt()) {
#endif
deselect();
}
#ifdef APPLE_STYLE_KEYS
if (k->get_command()) {
// Start at first column (it's slightly faster that way) and look for the first non-whitespace character.
int new_cursor_pos = 0;
for (int i = 0; i < text[cursor.line].length(); ++i) {
if (!_is_whitespace(text[cursor.line][i])) {
new_cursor_pos = i;
break;
}
}
if (new_cursor_pos == cursor.column) {
// We're already at the first text character, so move to the very beginning of the line.
cursor_set_column(0);
} else {
// We're somewhere to the right of the first text character; move to the first one.
cursor_set_column(new_cursor_pos);
}
} else if (k->get_alt()) {
#else
if (k->get_alt()) {
keycode_handled = false;
break;
} else if (k->get_command()) {
#endif
int cc = cursor.column;
if (cc == 0 && cursor.line > 0) {
cursor_set_line(cursor.line - 1);
cursor_set_column(text[cursor.line].length());
} else {
bool prev_char = false;
while (cc > 0) {
bool ischar = _is_text_char(text[cursor.line][cc - 1]);
if (prev_char && !ischar) {
break;
}
prev_char = ischar;
cc--;
}
cursor_set_column(cc);
}
} else if (cursor.column == 0) {
if (cursor.line > 0) {
cursor_set_line(cursor.line - num_lines_from(CLAMP(cursor.line - 1, 0, text.size() - 1), -1));
cursor_set_column(text[cursor.line].length());
}
} else {
cursor_set_column(cursor_get_column() - 1);
}
if (k->get_shift()) {
_post_shift_selection();
}
} break;

Quite hard to follow in my opinion. This logic now looks like this, except the following logic can use any key combination, not just the left arrow key.

// In gui_input:
if (k->is_action("ui_text_caret_left")) {
	_move_cursor_left(shift_pressed, false);
}
if (k->is_action("ui_text_caret_word_left")) {
	_move_cursor_left(shift_pressed, true);
}
//
if (k->is_action("ui_text_caret_line_start")) {
	_move_cursor_to_line_start(shift_pressed);
}
// Other methods in the class:
void TextEdit::_move_cursor_left(bool p_select, bool p_move_by_word);
void TextEdit::_move_cursor_to_line_start(bool p_select);

The keys used to perform these actions are defined in InputMap:

inputs = List<Ref<InputEvent>>();
inputs.push_back(InputEventKey::create_reference(KEY_HOME));
map.insert("ui_text_caret_line_start", inputs);

inputs = List<Ref<InputEvent>>();
inputs.push_back(InputEventKey::create_reference(KEY_A, KEY_MASK_CTRL));
inputs.push_back(InputEventKey::create_reference(KEY_LEFT, KEY_MASK_CMD));
map.insert("ui_text_caret_line_start.OSX", inputs);

Added Utility Method
Added static methods ::create_reference on InputEventKey and JoypadButton which is a one-line way to do:

Ref<InputEventJoypadButton> ie;
ie.instance();
ie->set_button_index(p_btn_index);

The above code is used in many places with Buttons and Keys, so moving it to a utility one-liner saves A LOT of lines of code, at least a couple hundred lines.

Unified definitions of the same shortcuts
Moved builtin input actions away from being defined twice in ProjectSettings and EditorSettings to only being defined once in InputMap. Project/Editor Settings instead just get the builtin actions and loop through them to add them to their settings list. Reduces code duplication.

Closes #17927
Closes #29490
Closes #12164
Closes #42139
Closes #26854
Closes godotengine/godot-proposals#1532

This is the fourth PR of a larger series based on #42600.

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 23, 2020

Requesting review from @Paulb23 as we had quite the dialog about this PR in the associated proposal thread, and I believe he is fairly well acquainted with this part of the code.

@akien-mga
Copy link
Member

For the reference, here's the list of default actions in the InputMap as of this PR. I know #42600 might refine this further but this might still be worth a discussion on whether all of those should really be configurable (like caret up/right bound to Up/Right), or if they should be renamed/better sorted to make it more obvious what is being affected by those.

My main concern seeing this list is that it's huge for something that most users won't even want to configure. On the other hand the same can likely be said of some of the original ui_ actions prior to this. (Though many users tend to reuse ui_<direction> and ui_accept for their game logic on simple projects.)

So IMO it's most important to ensure that the InputMap editor in the Project Settings will be as user-friendly as possible, even more so than the Editor Settings' shortcut configuration tab which is more advanced / less critical to each project. Again, this might be handled in #42600 already but discussion there seemed to focus on the Editor Settings. I'll give it a try to see.

User-defined actions and advanced UI configuration should probably be separated so that users can focus on their game actions first and foremost, and cater to fine tuning of Control actions only when they needed it (very rare cases IMO).

Screenshot_20201123_115805
Screenshot_20201123_115834
Screenshot_20201123_115849
Screenshot_20201123_115905
Screenshot_20201123_115936

@akien-mga akien-mga requested a review from Paulb23 November 23, 2020 11:00
@akien-mga akien-mga added this to the 4.0 milestone Nov 23, 2020
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 23, 2020

@akien-mga yes that pr deals with that issue, see below Gif

search_and_toggle_show

@EricEzaM
Copy link
Contributor Author

@akien-mga There should also be a full list at input_map.cpp @ line 267

Here I put the builtin action display names, for cases where they need to be displayed in a popup menu (like cut, copy paste, etc).

static const _BuiltinActionDisplayName _builtin_action_display_names[] = {
	/* clang-format off */
	{ "ui_accept", 									"Accept" },
	{ "ui_select", 									"Select" },
	{ "ui_cancel", 									"Cancel" },
	{ "ui_focus_next", 								"Focus Next" },
	{ "ui_focus_prev", 								"Focus Prev" },
	{ "ui_left", 									"Left" },
	{ "ui_right", 									"Right" },
	{ "ui_up", 										"Up" },
	{ "ui_down", 									"Down" },
	{ "ui_page_up",									"Page Up" },
	{ "ui_page_down", 								"Page Down" },
	{ "ui_home", 									"Home" },
	{ "ui_end", 									"End" },
	{ "ui_cut", 									"Cut" },
	{ "ui_copy", 									"Copy" },
	{ "ui_paste", 									"Paste" },
	{ "ui_undo", 									"Undo" },
	{ "ui_redo", 									"Redo" },
	{ "ui_text_completion_query", 					"Completion Query" },
	{ "ui_text_newline", 							"New Line" },
	{ "ui_text_newline_blank",						"New Blank Line" },
	{ "ui_text_newline_above", 						"New Line Above" },
	{ "ui_text_indent", 							"Indent" },
	{ "ui_text_dedent", 							"Dedent" },
	{ "ui_text_backspace", 							"Backspace" },
	{ "ui_text_backspace_word", 					"Backspace Word" },
	{ "ui_text_backspace_word.OSX", 				"Backspace Word" },
	{ "ui_text_backspace_all_to_left", 				"Backspace all to Left" },
	{ "ui_text_backspace_all_to_left.OSX", 			"Backspace all to Left" },
	{ "ui_text_delete", 							"Delete" },
	{ "ui_text_delete_word", 						"Delete Word" },
	{ "ui_text_delete_word.OSX", 					"Delete Word" },
	{ "ui_text_delete_all_to_right", 				"Delete all to Right" },
	{ "ui_text_delete_all_to_right.OSX", 			"Delete all to Right" },
	{ "ui_text_caret_left", 						"Caret Left" },
	{ "ui_text_caret_word_left", 					"Caret Word Left" },
	{ "ui_text_caret_word_left.OSX", 				"Caret Word Left" },
	{ "ui_text_caret_right", 						"Caret Right" },
	{ "ui_text_caret_word_right", 					"Caret Word Right" },
	{ "ui_text_caret_word_right.OSX", 				"Caret Word Right" },
	{ "ui_text_caret_up", 							"Caret Up" },
	{ "ui_text_caret_down", 						"Caret Down" },
	{ "ui_text_caret_line_start", 					"Caret Line Start" },
	{ "ui_text_caret_line_start.OSX", 				"Caret Line Start" },
	{ "ui_text_caret_line_end", 					"Caret Line End" },
	{ "ui_text_caret_line_end.OSX",					"Caret Line End" },
	{ "ui_text_caret_page_up", 						"Caret Page Up" },
	{ "ui_text_caret_page_down", 					"Caret Page Down" },
	{ "ui_text_caret_document_start", 				"Caret Document Start" },
	{ "ui_text_caret_document_start.OSX",			"Caret Document Start" },
	{ "ui_text_caret_document_end", 				"Caret Document End" },
	{ "ui_text_caret_document_end.OSX", 			"Caret Document End" },
	{ "ui_text_scroll_up", 							"Scroll Up" },
	{ "ui_text_scroll_up.OSX", 						"Scroll Up" },
	{ "ui_text_scroll_down", 						"Scroll Down" },
	{ "ui_text_scroll_down.OSX", 					"Scroll Down" },
	{ "ui_text_select_all", 						"Select All" },
	{ "ui_text_toggle_insert_mode", 				"Toggle Insert Mode" },
	{ "ui_graph_duplicate", 						"Duplicate Nodes" },
	{ "ui_graph_delete", 							"Delete Nodes" },
	{ "ui_filedialog_up_one_level", 				"Go Up One Level" },
	{ "ui_filedialog_refresh", 						"Refresh" },
	{ "ui_filedialog_show_hidden", 					"Show Hidden" },
	{ "",											""}
	/* clang-format on */
};

@EricEzaM
Copy link
Contributor Author

Hmm those descriptions will also need a TTRC() I think

@EricEzaM EricEzaM force-pushed the PR/INP4-dehardcode_scene_shortcuts branch 2 times, most recently from a156bee to 1242b2e Compare November 23, 2020 11:29
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Code looks fine to me, I think this level of configuration is okay, mainly comes down to having a suitable UX, which is easier said then done. The other point is needing someway to document or as perhaps as part of the UX, that appending .OSX overrides it for apple keyboards. Though similar to Aiken suggestion in #42600, it might be worth doing the UX changes as a separate proposal / PR so we don't get bogged down here.

Needs a hefty rebase after #41100, which also adds a new shortcut ui_swap_input_direction = KEY_QUOTELEFT to LineEdit and TextEdit.

Lastly, found #22927 which this should close.

@EricEzaM EricEzaM force-pushed the PR/INP4-dehardcode_scene_shortcuts branch from 1242b2e to 3e3ff29 Compare November 29, 2020 06:51
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 29, 2020

Rebased.

This should include all changes from the #41100 PR.
I added the ui_swap_input_direction action mapping and created a method for it in LineEdit and TextEdit.

@EricEzaM EricEzaM force-pushed the PR/INP4-dehardcode_scene_shortcuts branch 2 times, most recently from 7b60e5a to a132e89 Compare December 7, 2020 11:32
@akien-mga akien-mga merged commit 7eb4e64 into godotengine:master Feb 18, 2021
@akien-mga
Copy link
Member

Thanks!

@EricEzaM
Copy link
Contributor Author

It's in! How exciting!

I'm really happy with how this simplifies the logic in gui input for text and line edit

@christinoleo
Copy link
Contributor

christinoleo commented Apr 3, 2021

@EricEzaM Thanks for the work here.
I have a question to you. It seems that due to commits 074f535 and 49714b0 (maybe there are more, but this is as far as I got), by default the autocompletion in the editor accepts Space, Enter and KP_Enter. The space one is causing (at least me) to always when editing a gdscript to autocomplete AABB when typing " = ". The last space autocompletes to " =AABB" instead of leaving a space. Of course I can just remove space from the ui_accept shortcuts, but it took me some days to figure this out since godot 3.2 does not have this issue (at least by default). In your opinion, do you think it's worth not having the spacebar as a default ui_accept or is there some other code change which is actually causing this weird autocomplete to be done? I intended to create a new issue ticket but I wish to get your opinion first..
aaaa
Thanks!

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Apr 3, 2021

@christinoleo Thanks for pointing this out! Perhaps a new action needs to be created, specifically for accepting auto-complete options, which only includes tab, enter and kp_enter.

@Calinou
Copy link
Member

Calinou commented Apr 4, 2021

@christinoleo Thanks for pointing this out! Perhaps a new action needs to be created, specifically for accepting auto-complete options, which only includes tab, enter and kp_enter.

How should we implement this for use in the editor? Having to edit project-specific settings to tweak autocompletion to your liking sounds bad, since you don't want to commit that to version control for your whole team. Personally, I'd prefer to have only Tab accept suggestions, but I understand most people want Enter to accept suggestions as well.

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Apr 4, 2021

@Calinou never mind actually - it is a very easy fix. For some reason I check for both ui_text_completion_accept and ui_accept in the code... It should really only be the former. I don't believe there is any reason to have ui_accept in there. text_completion_accept only has Tab by default so that would also fulfil your request :)

godot/scene/gui/text_edit.cpp

Lines 3262 to 3266 in ed2f51b

if (k->is_action("ui_accept", true) || k->is_action("ui_text_completion_accept", true)) {
_confirm_completion();
accept_event();
return;
}

@Thane5
Copy link

Thane5 commented Feb 22, 2023

I tried the latest release candidate (rc3) but my modified "cut" hotkey still is ignored

@YuriSizov
Copy link
Contributor

Which shortcut have you modified, and what exactly are you trying to do to test it?

@Thane5
Copy link

Thane5 commented Feb 22, 2023

I set "ui_cut" to ctrl+shift+x
image

For testing, i created a new project. Created a "control" scene, added a button.

If i select it and press ctrl+shift+x, nothing happens. If i press ctrl+x, the object gets cut.

@akien-mga
Copy link
Member

Works fine for me on Linux with 4.0 RC 3:

image

Selecting text in the script editor and pressing Ctrl+Shift+X cuts it. Pressing Ctrl+X writes "x".

@Thane5
Copy link

Thane5 commented Feb 22, 2023

Oh, i just realised that there is a seperate "cut" command for the scene tree. I did not find it at first since i use the german version of the editor and unlike "ui_cut", the "scene tree cut" has already been translated into german.

However i find it a bit weird that "ui_undo" and "ui_redo" can affect the scene tree as expected, while "ui_cut" is specific to the text editor.

Anyways, at least my issue is solved now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment