Expose EditorSettings shortcut related functions

[IgorKordiukiewicz]

Closes godotengine/godot-proposals#4112.
Closes godotengine/godot-proposals#2024.

[IgorKordiukiewicz]

So I tried to resolve merge conflicts and now this code:

#if !defined(USE_LIGHTMAP)
		if (ambient_accum.a > 0.0) {
			ambient_light = ambient_accum.rgb / ambient_accum.a;
		}
#endif

is marked as changed in the files changed section. So tbh I don't know if that is normal or did I do something wrong?

[mhilbrunner]

Hey! Sorry you have been getting no feedback for this long. Our backlog is fairly large, but we're trying to improve that.
@YuriSizov will try to find some time to review this. As its not a blocking/compat breaking or blocking change, we'll move the target to 4.x to not block Godot 4.0 release. It can still go in Godot 4.0 if its reviewed and approved before feature freeze.

Thanks again for contributing and sorry this has been gathering dust!

[YuriSizov]

I think for the first draft this API makes sense to me. There is something to be said about making it a bit less verbose and a bit more abstract (see godotengine/godot-proposals#2024), maybe, but overall it's no worse than what you have to do in game code.

Here's a simple example I've made while testing:

@tool
extends EditorPlugin

func _enter_tree():
	var editor_settings := get_editor_interface().get_editor_settings()
	
	if not editor_settings.get_shortcut("my_plugin/alert"):
		var alert_shortcut := Shortcut.new()
		var key_stroke := InputEventKey.new()
		key_stroke.keycode = KEY_9
		alert_shortcut.events.append(key_stroke)
		editor_settings.add_shortcut("my_plugin/alert", alert_shortcut)

func _shortcut_input(event):
	var editor_settings := get_editor_interface().get_editor_settings()
	if event.pressed and editor_settings.is_shortcut("my_plugin/alert", event):
		print("ALERT!")

Works just fine, and I have no errors on reloading the plugin or the project completely.

I'd appreciate other plugin developers to offer their opinion on the API though. cc @AnidemDex @coppolaemilio

---

Two notes though:

• I think we could benefit from has_shortcut(name) in the public API. You can obviously try to get the shortcut and check it, but a helper like that would probably be nice to have for simplicity.
• The editor settings don't seem to pick up on the newly added shortcuts. So they can't be rebound, which is kind of the point. This needs fixing.

[coppolaemilio]

I think it looks great! Can't think of any "would be nice to have"s here. Looking forward to it!

[AnidemDex]

Looks good and simple enough for me, exactly what I wanted when I opened the proposal request.

Very good 👏

[programaths]

Why not offer a higher level function like:

editor_settings.add_shortcut("my_plugin/alert", "ctrl+a")

Or:

editor_settings.add_shortcut("my_plugin/alert", "ctrl+a",self,"select_all")

It would also make it much easier to see at a glance as we would not have to build the stroke in our head.
And because it's a one shot, decoding the string wouldn't make a perceptible impact on speed.

[YuriSizov]

@programaths What you propose is unrelated to this proposal and more related to the general input handling in Godot.

You can't bind methods to input events like that, you have to handle input events, be it raw strokes, shortcuts, or actions. Even if we could, it wouldn't be "select_all" as a string, btw, it would be a callable.

As for providing the combination as a string, it does look helpful, but again, this is unrelated to this PR. It can be a helper method for input events or a static helper method for input in general. That said, relying on strings is bad. You need to learn specific syntax with no completion or typing help. You will be limited in what you can and cannot set this way. And it's easy to make mistakes, typos, break it in some other subtle ways.

Either way, check https://github.com/godotengine/godot-proposals in case there is a proposal that covers what you want and give it a thumbs-up.

[programaths]

There is quite a big quality issue in the proposed code, relying on string is bad (#58585 (comment)) and Godot already solved that issue.

@tool
extends EditorPlugin

const alert_shortcut_name=&"my_plugin/alert"

func _enter_tree():
	var editor_settings := get_editor_interface().get_editor_settings()
	
	if not editor_settings.get_shortcut(alert_shortcut_name):
		var alert_shortcut := Shortcut.new()
		var key_stroke := InputEventKey.new()
		key_stroke.keycode = KEY_9
		alert_shortcut.events.append(key_stroke)
		editor_settings.add_shortcut(alert_shortcut_name, alert_shortcut)

func _shortcut_input(event):
	var editor_settings := get_editor_interface().get_editor_settings()
	if event.pressed and editor_settings.is_shortcut(alert_shortcut_name, event):
		print("ALERT!")

Indeed relying on strings is bad. You need to know the specific name of the shortcut with no completion or typing help. And it's easy to make mistakes, typos, break it in some other subtle ways.

[YuriSizov]

There is quite a big quality issue in the proposed code

It's not a proposed code, it's just an example of how the API can be used. It would sure be more stable to store the name of the shortcut in a variable and reference it, but it wasn't important for the demonstration. Unfortunately, here we can't get rid of strings completely, because they serve as a human-recognizable identifier. In case of the key combinations they would be responsible for a lot more than that.

[winston-yallow]

I only see methods for adding shortcuts to the editor settings. How would one clean up the shortcut when disabling the plugin? It would probably need some remove_shortcut() method, otherwise the shortcut will stay there for eternity

[YuriSizov]

@winston-yallow Good call. We don't have such need internally, but for plugins this makes sense to have.

[IgorKordiukiewicz]

I have added has_shortcut() and remove_shortcut() functions.

The editor settings don't seem to pick up on the newly added shortcuts. So they can't be rebound, which is kind of the point. This needs fixing.

This one I haven't solved yet because all shortcuts displayed in editor settings are shortcuts manually registered using ED_SHORTCUT() e.g. ED_SHORTCUT("animation_bezier_editor/focus", TTR("Focus"), Key::F); in AnimationBezierTrackEdit ctor.
And all shortcuts are added to the same shortcuts HashMap so I don't know if it is possible to differentiate between them during runtime

[EricEzaM]

The editor settings don't seem to pick up on the newly added shortcuts

tldr: my idea for additions to this PR https://github.com/IgorKordiukiewicz/godot/compare/expose-editor-settings-shortcut-related-functions...EricEzaM:58585?expand=1

The added shortcut does get returned in EditorSettings::get_singleton()->get_shortcut_list(&slist), but when constructing the shortcut list for the settings there is a check:

Ref<Shortcut> sc = EditorSettings::get_singleton()->get_shortcut(E);
if (!sc->has_meta("original")) {
	continue;
}

"original" is set for shortcuts for the "revert to default" functionality. All shortcuts registered through the editor have this. This is a bit of a 'gotcha' as it is specific to the way editor shortcuts work. Anyway, once you add that, it does start showing up in the settings, however the name is blank. This is because we use the resource name for the name of the shortcut. So on p_shortcut, we need to add ->set_name("shortcut name").

With all this being quite specific to adding shortcuts from scripting, it is probably worth adding a method specifically for adding shortcuts from script. This also helps with the remove_shortcut stuff, as we could then set_meta("from_scripting", true) and only allow removing shortcuts where get_meta("from_scripting") == true. This is just what I came up with off the top of my head - there may be better ways.

There is also a bit of a mess about when it comes to loading saved shortcut changes from editor settings. Basically, editor settings are loaded first, then your script is run, so we want to not have if not editor_settings.get_shortcut(alert_shortcut_name): in the script. We need to try add the shortcut every time - and if something was loaded from EditorSettings, then just take that shortcut array and apply it.

Hopefully looking at the code makes it all a bit clearer - here is a diff between this PR any my additions. https://github.com/IgorKordiukiewicz/godot/compare/expose-editor-settings-shortcut-related-functions...EricEzaM:58585?expand=1

Try to remove built in shortcut:

Shortcut changes save:

[YuriSizov]

it does start showing up in the settings, however the name is blank. This is because we use the resource name for the name of the shortcut. So on p_shortcut, we need to add ->set_name("shortcut name").

This can already be handled by user code, but we can also make the public add_shortcut method take a third argument and set it internally. We will be modifying the object supplied by the user already, though, so I would still probably prefer us to leave it to users and just explain it in the docs.

This also helps with the remove_shortcut stuff, as we could then set_meta("from_scripting", true) and only allow removing shortcuts where get_meta("from_scripting") == true. This is just what I came up with off the top of my head - there may be better ways.

I don't think it's a huge problem. You can also dynamically mess up other editor settings with the API we have. While it can be useful to safeguard this, it's not a requirement at this point IMO, and we can just rely on good will of plugin developers. Besides, none of it is gone forever and it only takes disabling the plugin and restarting the editor to restore the missing settings (albeit at their default values).

"original" is set for shortcuts for the "revert to default" functionality. All shortcuts registered through the editor have this. This is a bit of a 'gotcha' as it is specific to the way editor shortcuts work.

Ah, right. Then we should do the same for the shortcuts added through the public API. They shouldn't really differ from "editor shortcuts". Editor is also full of built-in plugins, so I'm not sure why we'd want the user-scripted plugins to be any different. Besides, they also need to have that default/user-configured distinction. So if that's what is missing then it needs to be added to the public API.

There is also a bit of a mess about when it comes to loading saved shortcut changes from editor settings. Basically, editor settings are loaded first, then your script is run, so we want to not have if not editor_settings.get_shortcut(alert_shortcut_name): in the script. We need to try add the shortcut every time - and if something was loaded from EditorSettings, then just take that shortcut array and apply it.

I wanted to ask "why", but then I guess if the plugin's code adds shortcuts with the "original" meta and user's stored information is used to override it, then yeah, we should probably always add it, and just merge the two. But again, it shouldn't really be any different from any built-in plugin (other than maybe a more complicated order of events).

Hopefully looking at the code makes it all a bit clearer - here is a diff between this PR any my additions. https://github.com/IgorKordiukiewicz/godot/compare/expose-editor-settings-shortcut-related-functions...EricEzaM:58585?expand=1

Other than what I've already noted, this is a good improvement that we should incorporate, yeah. For consistency, though, I think all existing methods should refer to the first argument as path too 😛

[IgorKordiukiewicz]

Updated, also about this change from: https://github.com/IgorKordiukiewicz/godot/compare/expose-editor-settings-shortcut-related-functions...EricEzaM:58585?expand=1

	Array use_events = p_shortcut->get_events();
	if (shortcuts.has(p_path)) {
		auto existing = shortcuts.get(p_path);
		if (!existing->has_meta("original")) {
			// Loaded from editor settings, but plugin not loaded yet.
			// Keep the events from editor settings but still override the shortcut in the shortcuts map
			use_events = existing->get_events();
		} else if (!Shortcut::is_event_array_equal(existing->get_events(), existing->get_meta("original"))) {
			// Shortcut exists and is customised - don't override with default.
			return;
		}
	}

@YuriSizov should I add it or just leave it as it is without it, since the already existing add_shortcut method doesn't have any checks against already existing shortcuts, etc. ?

[EricEzaM]

Try using the system without that code - it doesn't work properly when you try customise the shortcut and keep that customisation between editor restarts.

The real add shortcut method used in the editor code is ED_SHORTCUT, which does have checks about whether the shortcut already exists.

The public add_shortcut method isn't even used anywhere, except for when settings are being loaded. It should probably be made private.

[winston-yallow]

I don't have much knowledge how shortcuts work internally, but just from the API it seems weird that a method named get_shortcut() isn't just a getter. Instead it also adds that shortcut to a list if it is a builtin one. So as I understand it, a call to get_shortcut() can change the result of get_shortcut_list(). The documentation does mention this, but without many details. It was unclear to me what list it was talking about until I read the source code.

[KoBeWi]

Suggested change

	// bind helpers
	// Bind helpers.

[KoBeWi]

Suggested change

	Returns the shortcut specified by [param path]. If no shortcut with the provided path is found in the list, tries to find a built-in shortcut. If found, adds it to the list and returns it, otherwise returns [code]null[/code]
	Returns the shortcut specified by [param path]. If no shortcut with the provided path is found in the shortcut list, tries to find a built-in action. If found, adds it to the list and returns it, otherwise returns [code]null[/code]

"list" and "built-in shortcut" sound a bit unclear.

[servel333]

It's been a while since I have written C++, but I notice that the & was removed along with the const, and removing the & doesn't make sense to me, and I wanted to make sure that is intentional.