Duplicating node by Ctrl+D duplicates code

[volzhs]

Godot version:
3.2.2 beta 2 8cf450c

OS/device including version:
Linux mint 19.3

Issue description:
Duplicating node by Ctrl+D duplicates code of current line, not duplicating node.

Steps to reproduce:

1. Duplicate node by Ctrl + D with 2D viewport
2. Duplicate node by Ctrl + D with script editor
   

Minimal reproduction project:

[MrRevington]

I think #37839 should fix that too.

It's based on the current master(ignore the branch name) maybe it should be cherry-picked for 3.2 @akien-mga ?

[volzhs]

I found that #37839 does not fully fix this issue.
Ctrl+D duplicates node and code at the same time.

[volzhs]

I found another issue that Ctrl+A is handled on both script editor and scene tree dock.

1. Click a node on Scene dock
2. Ctrl + A and cancel
3. Click script editor
4. Ctrl + A
5. New node popups up and selects all text in script editor at the same time.

I guess it needs to find why this regression happens in the first place.

[EricEzaM]

I am experiencing this in 3.2.1 stable as well btw. Something I noticed but never created an issue for, so thanks! 😂

[MrRevington]

I found that #37839 does not fully fix this issue.
Ctrl+D duplicates node and code at the same time.

I could not replicate that on my machine(Windows 10) but what is strange is that it looks exacly like in your example: the scene dock has focus (white highlighted border), a node is selected but only the code line is duplicated if I press Ctrl+D.

Edit:
I can only get a node to dublicate if the scene window is open, if the script window is open the scene dock doesn't to Ctrl+D but it does react to Ctrl+A like in you first example even is the script window is open.

Edit2:
Sounds like the bug mentioned in #5374

[EricEzaM]

I have the same results as @volzhs, #37839 does not resolve the issue for me. I think the issue lies deeper than that PR. I have had a look into it further and noticed a few things. The "duplicate" bug and "select all/new node" bug actually have 2 different causes (but in the end are related to the same underlying issue):

The sequence of events/method calls:
Select all/New Node (Control + A)

1. Window gets input at Window::_window_input(p_ev)
2. Input does not get handled by calling input(), so unhandled_input() is called
3. _unhandled_input() is called on all nodes which are registered to listen for it.
4. Here lies the issue... the scene tree dock shortcut is called on _unhandled_input(), but the event is never marked as handled. In BaseButton::_unhandled_input(), the action is executed but never marked as handled. This means that we continue...
5. Since the event thinks it hasn't been handled yet, it goes onto execute get_tree()->_call_input_pause(unhandled_key_input_group, "_unhandled_key_input", ev, this);, which means it now calls _unhandled_key_input() on all nodes who are registered to receive this input.
6. The MenuButton on the text editor gets the event and finds the shortcut which matches... which is "select all", so it executes it. Thus, text is selected AND the new node window comes up.

Solution: handle the button event first, so the event doesn't keep propagating. Control focus seems to have nothing to do with this problem.

Duplicate Node (Control + D)

1. Window gets input at Window::_window_input(p_ev)
2. Input is handled by text editor at _input() and the event is accepted.
3. Event propagation does not continue

So, in this case, yes, #37839 does fix this as the `_input() call will stop running since the text editor does not have focus.

But the same issue as before arises, because there are 2 paths to get to the code to clone a line.

1. ScriptTextEditor::_input() -> clone_lines_down()
2. MenuButton::_unhandled_key_input() -> PopupMenu::activate_item_by_event() -> CodeTextEditor::clone_lines_down

So yes, while the PR does fix 1., 2. still persists the issue.

[volzhs]

@EricEzaM said

I am experiencing this in 3.2.1 stable as well btw.

@MrRevington said

Sounds like the bug mentioned in #5374

so. maybe it's not regression, but not solved for a long time I think.

[EricEzaM]

So basically shortcuts are a bit cooked, because it doesn't matter where they are initiated from - they are checked in every available menu that has shortcuts. This means that multiple shortcuts with the same key combo are a no-go.

For example, try this:

1. Start a new scene and a node.
2. Attach a script (built in or whatever)
3. Add something to the script
4. Add a new node to the scene tree.
5. With the script editor visible, and with the scene tree focused, press Control+Z (undo)
6. Notice that the text was undone, not the addition of the node.
7. Now go to Scene -> Undo, and notice that the node was removed (undone)

This happens because the text editor receives the event first and accepts it.

Now, go to your editor settings and change "Script Editor" -> "Undo" to something thats not control + Z, and try the same thing again. Lo and behold, the undo now works for the scene tree.

This behaviour is visible almost anywhere where there are duplicate shortcut keys. The thing is that shortcuts can be initiated from several different input handling methods:

• _input()
• _gui_input()
• _unhandled_input()
• _unhandled_key_input()

So shortcut clashes exist where the shortcuts are handled at the same "level". So this just adds exceptions to the issue, and cause further complexity.

[EricEzaM]

Yep, just as I had thought...

In the code below, every node which implements p_method (e.g. _unhandled_key_input) has the method called on it, until the input gets handled. Note that this array is iterated in reverse. The script editor MenuButton is at index 43, while SceneTreeEditor is alllll the way down at 13, meaning that it never even gets a chance to receive the event if the script editor handled it first.

godot/scene/main/scene_tree.cpp

Lines 857 to 885 in 4645097

	//copy, so copy on write happens in case something is removed from process while being called
	//performance is not lost because only if something is added/removed the vector is copied.
	Vector<Node *> nodes_copy = g.nodes;
	int node_count = nodes_copy.size();
	Node **nodes = nodes_copy.ptrw();
	Variant arg = p_input;
	const Variant *v[1] = { &arg };
	call_lock++;
	for (int i = node_count - 1; i >= 0; i--) {
	if (p_viewport->is_input_handled()) {
	break;
	}
	Node *n = nodes[i];
	if (call_lock && call_skip.has(n)) {
	continue;
	}
	if (!n->can_process()) {
	continue;
	}
	n->call_multilevel(p_method, (const Variant **)v, 1);
	//ERR_FAIL_COND(node_count != g.nodes.size());
	}

Not really sure how one would go about resolving this... seems like an issue that would require core changes to how shortcuts are checked and input is handled.

We could potentially make it so that shortcuts only work where the user has focus... But this would mean some duplication of shortcuts or "fallback" system. For example, if you are focused in the Canvas Item Editor (or Spatial Editor), then pressing Control + D will still duplicate a the node, however this shortcut goes through the scene tree dock, not the item editor itself. So you would need to make duplicates of shortcuts, or have some sort of fallback system where the item editor will check the scene tree dock after it checks itself for shortcut matches. Just spitballing here, but you get the idea.

@akien-mga any thoughts? Should a new issue be started dealing with the shortcut issue I have found here?

[Calinou]

Duplicate of #36205.