Shortcuts rework - fixed issues with input propagation and triggering of unwanted shortcuts.

[EricEzaM]

The key file to look at is base_button.cpp. The rest is mainly just adding the usage of the new API, and changing of names. There are a couple of other commits with some input/shortcut related changes. You can view those commits on their own to see what was changed.

What does this solve?
This solves a number of shortcut related issues which have been around the engine for quite some time. Issue listed below.
Fixes #36205, fixes #38922, fixes #37807, fixes #37054, fixes #17591, fixes #35834, fixes #31779
(Supersedes) closes #37068 and closes #39039

Most notably this allows greater control over what context shortcuts are allowed to be triggered.

What changes are there and how does it solve the issue?
Control's have a new method - set_shortcut_context(ObjectID p_ctx). This allows you to set the 'context' of where the shortcut is allowed to work. See the below image. The MenuButton's have the ScriptTextEditor as their context. This means that you have the option to ensure that their shortcuts will only trigger if the ScriptTextEditor or any of it's children have GUI focus in the viewport.

This context is used wherever you want to check for shortcuts. Most commonly, this will be inside unhandled_key_input. Here is an example for the ScriptTextEditor...

script_text_editor.cpp (constructor)

edit_menu = memnew(MenuButton);
edit_menu->set_text(TTR("Edit"));
edit_menu->set_shortcut_context(this);

menu_button.cpp

void MenuButton::_unhandled_key_input(Ref<InputEvent> p_event) {
	if (!is_focus_owner_in_shorcut_context()) {
		return;
	}

        < ... more logic ... >
}

If the focus of the viewport is not a child of the context, then no input is handled. Nice!

---

I am pretty sure I have caught every case of Buttons and MenuButtons being used as shortcuts and have added set_shortcut_context where it is needed, but it is possible I missed a few.

Through my work I also found a number of redundant pieces of code, which included some _unhandled_key_input calls which were never reached since their functionality was actually handled in a parent.

In Action:
#17591

#37054

Also Works in script...
In this clip I spam the button shortcut I set up. As you can see, it only works when the shortcut context has the viewport focus.

[EricEzaM]

Ok, after some initial discussion with @groud this might be too much of an overcomplication for what is needed. I'll do some updates and also post the IRC log here later. changes done now

[BeayemX]

Will this also solve #22843?

[EricEzaM]

@BeayemX No, pressing escape to close the bottom panel was removed completely. That functionality would need to be reinstated for it work.

[EricEzaM]

16/09/2020

[12:03:04] <EricEzaM[m]> Groud: Akien reduz yesterdays input and shortcuts discussion now in PR form... #42109.
[12:03:05] <IssueBot_> #42109: Shortcuts rework - fixed issues with input propagation and triggering of unwanted shortcuts. | https://git.io/JUBP2
[12:17:06] <Groud> EricEzaM: I don't get it, why would you have to create a whole new method instead of using _unhandled_key_input ?
[12:19:10] <Groud> The only thing needed was to check if the focus was in the given context no ? So like context.is_a_parent_of(focussed_node)
[12:20:20] <Groud> I mean this logic could have been implemented in the few places where we handle shortcuts, which are not that many
[12:22:09] <Groud> The justification you use for creating a new function is for the function to receive less events, but using _unhandled_key_input is rarely done, and it already processes few arguments
[12:22:16] <Groud> So there is no performance gain here
[12:23:02] <Groud> Honestly, I would simply use _unhandled_key_input, which was kind of meant for keyboard shortcuts, and not create yet another function dedicated to shortcuts.
[12:23:56] <EricEzaM[m]> @groud currently viewport doesnt expose a public method for getting focused node
[12:26:03] <EricEzaM[m]> also i mentioned shortcuts might not just be keyboard. Having ability for extra mouse buttons to act in shortcuts would be a plus
[12:26:49] <Groud> hmm
[12:27:15] <Groud> I don't know... why not but it kind of annoys me to create another function only for that...
[12:27:45] <EricEzaM[m]> I understand, I kind of thought the same thing but I didn't know what else to do
[12:27:55] <EricEzaM[m]> What you say does make me think though...
[12:28:20] <Groud> The thing is that it kind of make _unhandled_key_input useless now
[12:28:51] <EricEzaM[m]> The other thing is that shortcuts are for gui only
[12:29:21] <Groud> hmm, I guess that would make sense
[12:29:50] <Groud> but I am not sure it is worth creating yet another function only to allow shortcuts for mouse buttons
[12:29:57] <EricEzaM[m]> What if `unhandled_key_input` becomes `unhandled_button_input` or something similar, and is not only restricted to `InputEventKey` but Key, MouseButton and JoyButton as I have in this PR.
[12:29:58] <Groud> It's kind of an unusual use case
[12:30:11] <Groud> ah
[12:31:06] <Groud> That might be a good idea, as we would still have few events compared to handling mouse moves/joy sticks
[12:31:22] <Groud> I like it, that would solve the problem quite well
[12:31:25] <EricEzaM[m]> then, on controls you can set `set_shortcut_context` as I have in this PR. Then you register to use `unhandled_button_input`, and when you do, you check  if `get_viewport()->get_gui_focus()` is a parent of the `shortcut_context`
[12:31:43] <EricEzaM[m]> and I would need to add a viewport method which exposes the gui focus
[12:31:51] <EricEzaM[m]> publicly
[12:32:17] <Groud> I don't think you need that in fact
[12:33:06] <Groud> simply call context->get_focus_owner() to get the focus
[12:34:00] <EricEzaM[m]> oh yeah it is exposed that way
[12:34:02] <EricEzaM[m]> nice
[12:39:05] <Groud> EricEzaM: maybe let's see what reduz think about this first though :)

[EricEzaM]

FYI this was removed because the search popup is actually handled through ScriptEditor::_menu_option, via the Search menu button in the script editor. This code would never be hit.

[EricEzaM]

This looks like a lot of changes but all I did was add the is_valid() check

[EricEzaM]

@akien-mga Please let me know if you want me to squash some or all of the commits. I feel they are unrelated enough to warrant separate commits but it is up to you.

[akien-mga]

Please let me know if you want me to squash some or all of the commits. I feel they are unrelated enough to warrant separate commits but it is up to you.

The current commit lineup looks good to me 👍

[EricEzaM]

@akien-mga Can I bump this one please ;)

[akien-mga]

Thanks a ton, it's great to get all these issues finally solved :)

[Feniks-Gaming]

Thank you it will solve a lot of headaches I had