Pass current value to EditorInterface node/property popups

[Naros]

Fixes #94322

As outlined in the issue, while these new methods are nice in Godot 4.3, they don't seem to hit the mark in terms of being fully user friendly without being able to specify a current value, should these be used in cases where the addon already has an existing value and would prefer to highlight that in the dialog when its shown.

[Naros]

You need to bind compatibility methods, see here

Even if these methods were introduced in an alpha/beta build of Godot 4.3?

The practical reason behind this PR in the first place was to address the shortcomings of the original implementation so that when it gets into a stable build, these APIs are reasonably feature complete and to avoid needing compatibility methods.

[AThousandShips]

Oh my bad didn't know they were current, then it's unnecessary

[AThousandShips]

Not familiar with the internals so a second eye on this would be good but the code looks good to me and the existing interface is just employed

[KoBeWi]

The current_value in popup_property_selector() seems to do nothing. Nothing is selected no matter what I tried to use.

[Naros]

The current_value in popup_property_selector() seems to do nothing. Nothing is selected no matter what I tried to use.

I wonder if that is a bug in the actual base dialog implementation because all this PR allows you to pass the same value the editor would pass when using this dialog in the engine or C++ modules.

I'll take a look after work today and get back to you, @KoBeWi.

[Naros]

Hi @KoBeWi so I checked the code in the property_selector.h and property_selector.cpp files and it does seem that while one can pass the current "selected" value into the dialog, the logic within PropertySelector::_update_search does not actually do anything with the member variable selected.

Looking more closely at the uses of PropertySelector, there are 8 public methods that pass the current value and store that in the selected member variable; however, while the member variable's value is set by those 8 methods, it's not used internally in any of the presentation logic sadly.

So I think for completeness, we should at the very least keep the ability to pass this value on the EditorInterface. This minimizes the need to introduce any compatibility method after-the-fact. I think the question is really how do we want to address this bug, as a part of this PR or a follow-up.

I'm of the opinion that give where we are with 4.3, we should likely take care of addressing the PropertySelector implementation bug separately at the outset of 4.4 and then consider backporting that fix. That would give us enough time to battle test not only the variant exposed by the EditorInterface, but the other 7 public methods on the class in case the editor would like to make use of those in the future. What do others think? @akien-mga ?

Questions remain

[KoBeWi]

Well, you added a new argument and it didn't require any compatibility code, so looks like adding arguments in the future is not a problem.
I think this can be pushed to 4.4 and let you can finish it properly.

[KoBeWi]

Almost. The view is not centered on the property, but awkwardly cut at the top:

[Naros]

Almost. The view is not centered on the property, but awkwardly cut at the top

I'll take a look and see if I can reproduce it; however, I used scroll_to_item with center on item 🤔

[KoBeWi]

Well my test code is this:

@tool
extends EditorScript

func _run() -> void:
	EditorInterface.popup_property_selector(get_scene().get_node("Icon"), Callable(), [], "z_index")

(requires "Icon" node in the current scene)

[Naros]

Thanks @KoBeWi, fixed. It appears I needed to delay the scroll_to_item until the end of the frame since building of the list was still in-flight and the offset would obviously change due to additional items added.

[AThousandShips]

You've added your own compatibility file, this isn't correct, it should be added to the existing 4.3-stable.expected (creation of these files is handled by maintainers, you don't need to do it yourself)

[Naros]

You've added your own compatibility file, this isn't correct, it should be added to the existing 4.3-stable.expected (creation of these files is handled by maintainers, you don't need to do it yourself)

Well, as you can see that file doesn't exist based on the HEAD of this PR, and so I based those changes on what I saw in other PRs. I'll rebase and amend the existing file. Is this documented somewhere ?

[AThousandShips]

Don't think it's documented but it's a general maintenance thing, good to rebase after a release to make sure things are up to date

[KoBeWi]

Extraneous space

Suggested change

Pops up an editor dialog for selecting a [Node] from the edited scene. The [param callback] must take a single argument of type [NodePath]. It is called on the selected [NodePath] or the empty path [code]^""[/code] if the dialog is canceled. If [param valid_types] is provided, the dialog will only show Nodes that match one of the listed Node types. If [param current_value] is provided, the Node will be automatically selected in the tree, if it exists.

Pops up an editor dialog for selecting a [Node] from the edited scene. The [param callback] must take a single argument of type [NodePath]. It is called on the selected [NodePath] or the empty path [code]^""[/code] if the dialog is canceled. If [param valid_types] is provided, the dialog will only show Nodes that match one of the listed Node types. If [param current_value] is provided, the Node will be automatically selected in the tree, if it exists.

Same below.

[akien-mga]

Thanks!