Properly show detach script button when script is added via inspector

[Kazuo256]

Fixes #107059.

The SceneTreeDock was not tracking script changes in selected nodes in any capacity as far as I could assess. To do
that, my solution essentially connects the "script_changed" signal from selected nodes to
SceneTreeDock::_update_script_button() whenever the selection changes. It actually queues the update to make sure it
happens only once no matter how many nodes are selected.

However, only connecting that signal would leave previously selected nodes with a signal connection that should no
longer exist. To properly disconnect previously selected nodes, we have to store the list of currently selected nodes so
we can disconnect them when the selection changes.

The PR also replaces all uses of SceneTreeDock::_update_script_button() with the queued alternative, besides other minor minor improvements requested in the review. See commit message for more details.

[KoBeWi]

You can't just connect script_changed to _update_script_button(), because it will update the button for every selected node. It should happen only once. You can add something like _queue_update_script_button().

[Kazuo256]

You can't just connect script_changed to _update_script_button(), because it will update the button for every selected node. It should happen only once. You can add something like _queue_update_script_button().

Makes sense. Do you have a suggestion for where to check for queued updates? My first guess would under the NOTIFICATION_PROCESS case in the _notification() method.

[KoBeWi]

The usual pattern is

void queue_update() {
	if (is_update_queued) {
		return
	}
	is_update_queued = true
	call_deferred("actual_update")
}

[Kazuo256]

I've implemented the requested changes. One thing I realized is that clicking the script button in the scene tree dock to remove the script will call _update_script_button twice (regardless of how many nodes are selected). One because of the callback I added to the script_changed signal, and another as part of the usual logic of the remove script button. Is that OK or should I also adjust the script button to queue the update?

[KoBeWi]

should I also adjust the script button to queue the update?

Now that we have a queue method, it can be used more. So yes.

[Kazuo256]

Replaced all SceneTreeDock::_update_script_button() calls with the new queued version. See commit message for more details.

I chose to divide the PR's work into three commit since they were separate atomic changes that someone might want to cherry-pick/revert/etc independently, but I can squash them together if that is preferrable.

[Repiteo]

Could you squash the commits? See our PR workflow documentation for details: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

[Kazuo256]

Could you squash the commits? See our PR workflow documentation for details: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

No problem, I'm aware of the workflow. Just to be sure, I'll reiterate my last comment, but will squash as asked:

I chose to divide the PR's work into three commit since they were separate atomic changes that someone might want to cherry-pick/revert/etc independently, but I can squash them together if that is preferable.

[Repiteo]

Thanks!

[xuwaters]

When closing the scene tab, the pointers in LocalVector<Node *> node_previous_selection becomes dangling pointers, which causes random crash.

[Kazuo256]

When closing the scene tab, the pointers in LocalVector<Node *> node_previous_selection becomes dangling pointers, which causes random crash.

I'll work on a fix. Is there an issue open yet?

[xuwaters]

Thank you, opened an issue: #108783

[Kazuo256]

PR to fix the crash: #108785