Fixed issues with input propagation triggering unwanted shortcuts

[EricEzaM]

Fixes #36205, fixes #38922, fixes #37807, fixes #37054, fixes #17591 and potentially others.

This fix adds a check in viewport.cpp which checks that the focused node and none of its parents can handle the input, before propagating the input to all editor nodes which implement _unhandled_key_input. iterates through the node and it's parents, propagating calls to it's children. This ensures that _unhandled_input is called not only on the node itself, but things like MenuButtons and Buttons which have assigned shortcuts.

Also, a few accept_events() were added where needed to stop multiple
shortcuts from running at once. See #38922 for more comments.

Replaces/builds upon #37839 and #37068.

Requesting review/feedback from @reduz @akien-mga and others to see what they think. I think this is more a 'band-aid' fix over some bigger shortcut/input issues but hey, it works.

Before (3.2.1, latest master):

After:

[EricEzaM]

Updated for rebase and to account for removal of call_multilevel, which was used before.

[RandomShaper]

I'll have a look to this likely next week. Sorry, but I've been very busy and I'd like to check it deeply.

[EricEzaM]

My recent changes have changed the logic a bit. This makes it also solve issue like #17591, where previously it did not.

Instead of just using node->call(), I now use node->propagate_call. This ensures that _unhandled_input is called not only on the node itself, but things like MenuButtons and Buttons which have assigned shortcuts. The call implementation missed all MenuButton shortcuts, for example. The iteration through parents also stops when node == this (i.e. this viewport) so that events do not propagate to parent windows, like in the Input Map Editor, for example. If this check was not there, events from the "Press a key" dialog in the input mapper would get all the way back to the main EditorNode control and as a result call the shortcut on things like the script editor. So if you assigned Ctrl + D to a shortcut, lines would also duplicate.

[RandomShaper]

I'm not very convinced about this change. It's indeed a hack. I believe we can keep most of it but wiring it in a different way to the engine.

The accept_event() thing looks fine. My problem is with the part about key event bubbling. I'd suggest the following:

• Leave _unhandled_key_input alone since that's a callback every node type can get and it's not related to GUI. Patching it for GUI needs doesn't look like a good idea.
• Leveraging Viewport::_gui_input_event() to handle key events in addition to mouse and touch. That function would contain the bubbling logic for GUI, much like it does with mouse/touch, and also much like the code in the PR does, but constrained to the scope of GUI.
• Changing all the shortcut handling in the code of the editor to check for them in _gui_input_event(). We could consider that doing that is the new, recommended way to listen for shortcuts, while the old one keeps being functional so plugins, etc. relying on it don't get broken. A note about this in the release notes and the docs would be nice.

The goal of this changes is to get the very welcome changes of your PR in a cleaner, and non-breaking way. This proposal may still have some loose ends. What do you think?

[EricEzaM]

@RandomShaper

1. Sorry, I'm a bit confused... you say "leave _unhandled_key_input alone" but I haven't actually touched its implementation in this PR. Could you clarify what you mean by this statement?
2. I can see that _gui_input_event is used for mouse and touch inputs, as well as navigation with keys which are bound to the ui_ set of actions. I suppose moving shortcut logic here makes sense.
3. So what you are saying is to move all shortcut handling logic to _gui_input? I think I like this idea as it would allow mouse buttons to also be assigned to shortcuts, which they currently cannot. So this may allow easier execution of Added ability for mouse buttons to be assigned as editor shortcuts #38375, which solves Can't assign mouse buttons or mouse wheel to editor shortcuts #38326.

[RandomShaper]

1. I was referring to keeping the logic to call _unhandled_key_input intact.
2. Cool.
3. Yes, I got the name wrong, but that's indeed the idea.

[EricEzaM]

Well that certainly is a bigger job than what this current PR is.

Since that is a big change, should we consider some other way of handling this completely? Or is bubbling the events up and then propagating through children the best way to do it overall?

[EricEzaM]

@RandomShaper LineEdit (and others) use _gui_input for text input. If propagate_call is used with _gui_input then it means that line edit's will handle the event if it is a valid character input (e.g. A, Shift D, delete, etc) as well as valid shortcuts for the line edit (e.g. Ctrl A). If we were to use _gui_input, how do we discern between shortcuts and text input? This issue makes me think shortcuts going through _gui_input is not the right solution.

[EricEzaM]

Godor devel IRC 10th September 2020

[14:06:00] <EricEzaM[m]> reduz, Akien Groud , anyone else with input: do you have anything to say regarding fixing shortcut issues, as I tried in my attempt with #39039. I understand it is an imperfect solution but I just cannot think of other good ways to solve the issue of the shortcuts, specifically duplicate shortcuts. I sent a similar message yesterday or the day before but it did not get much traction since I didn't ask anyone
[14:06:00] <EricEzaM[m]> directly. Would you have any ideas on the direction you would want to go with this issue?
[14:06:01] <IssueBot_> #39039: Fixed issues with input propagation triggering unwanted shortcuts | https://git.io/JfVim
[14:07:01] <EricEzaM[m]> I have been mulling it over since RandomSharper's response but I just can't think of good, robust and reliable ways of doing it on my own at the moment
[14:07:48] <Groud> In the editor we did not make use of accept_event that much but it's in fact a mistake. So accept_event there should be called indeed, especially if it fixes some problems
[14:08:29] <Groud> Regarding the modification of the code in viewport, this is a lot more tricky, so you should ask reduz for that
[14:38:04] <reduz> EricEzaM[m]: i think this needs re-thinking to be honest
[14:38:40] <EricEzaM[m]> I've been trying 🤔 haha
[14:57:00] <EricEzaM[m]> @reduz so the main issue I see is two-fold. First, you don't want duplicate shortcuts to trigger things twice, like `Ctrl+D` works both in the script/text editor and the scene tree dock. How do you discern which operation the user wants to complete? Focus? Not all editor plugins/controls are focusable in such a way that would allow this. Maybe if they were made do, doing it based on focus would be viable.
[14:57:00] <EricEzaM[m]> Secondly, sometimes you _do_ actually want shortcuts to propagate from one section of the editor to another. Like in the viewport, if you press `Ctrl+A`, you want to add a new node - but this shortcut is on the scene tree dock. Currently it works because the editor just loops through everything in the `unhandled_key_input` group until the event is accepted.
[14:58:30] <reduz> EricEzaM[m]: I think for duplicate/delete, we need to make those global shortcuts instead of per editor and hook to signals or something, and to do a real fix, we need to ensure that only one editor can have an active selection at the same time
[14:58:41] <EricEzaM[m]> Regarding the first issue, I can't think of a better method than doing it based on focus. What other metric is there? not sure. For the second one, maybe certain parts of the editor could be "linked" together, where if a shortcut did not get accepted in one part then it would be passed onto another
[14:59:00] <reduz> EricEzaM[m]: as an example, if you select nodes, animation keys should deselect. If you select animation keys, nodes should deselect
[14:59:35] <reduz> EricEzaM[m]: the second issue is not too bad and mostly related to the text or tree editors, if you press cltr-a and the control with key input handles it, it should avoid the global shortcut
[14:59:53] <reduz> but i think it does this mostly now already
[15:01:45] <reduz> EricEzaM[m]: maybe we can even keep things as they are now, where all controls have their own selection, but each time a new selection is initiated in either area, we send a signal or group call so other editors disable their selection
[15:02:19] <EricEzaM[m]> @reduz I think that would be the simplest solution right now
[15:03:25] <reduz> EricEzaM[m]: if you want to work on that feel free, i think its a very simple solution, at least should solve the problem of ctrl-d/delete/etc
[15:04:50] <reduz> EricEzaM[m]: maybe add a signal to EditorNode that is "selection_request" witrh an "editor" argument, so each time one of the editor requests the selection, it does EditorNode::get_singleton()->emit_signal("selection_request",<editorname>) which can be like "scene_tree" , "animation_keys", "etc"
[15:05:13] <reduz> EricEzaM[m]: so when you get the signal, if its not your own, you clear the selection
[15:15:52] <EricEzaM[m]> reduz: I'll give that a go, thanks for the feedback. Another thing I have noticed though is that shortcuts are handled in all of `_input`, `_gui_input`, `_unhandled_input` and `_unhandled_key_input`, with `_unhandled_key_input` being relied on a lot for shortcuts (which btw means mouse buttons cannot be used). There is no 'one place' where they go... should there be? Shortcuts are for GUI input, so should there
[15:15:52] <EricEzaM[m]> be some sort of secondary shortcut-specific gui input method? It seems unnecessary, but we can't use current `_gui_input` since things like `LineEdit` and `TextEdit` use it for processing normal text input. It could even possibly allow for removal of `_unh_key_input` since, as I said, in the editor it is really only used for handling shortcuts.
[15:18:26] <EricEzaM[m]> The existence of `_unhandled_key_input` seems quite peculiar to me. It is only called after `_unhandled_input` and only if the event is an `InputEventKey`. If you needed this, why can't you just do it in `_unhandled_input`?
[16:09:47] <reduz> EricEzaM[m]: it really depends on the usage to be honest, some work when controls are focused while some works globally
[16:10:16] <reduz> EricEzaM[m]: but that is generally normal, as focused controls have priority and can consume events, while then unhandled ones will receive it if unconsumed

[EricEzaM]

Superseded by #42109