Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the "Quick Open" dialog available via EditorInterface #97633

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

ydeltastar
Copy link
Contributor

Allows to use the "Quick Open" dialog in tool scripts via the EditorInterface without exposing internal classes. Works the same as other editor popups added in #81655.

@tool
extends EditorScript

func _run():
	EditorInterface.popup_quick_open(_on_selected, "Texture2D", true, false, "Select Textures")

static func _on_selected(paths: PackedStringArray):
	print("Paths: %s" % paths)

@KoBeWi
Copy link
Member

KoBeWi commented Oct 3, 2024

Needs rebase after #56772

@@ -36,6 +36,7 @@
#include "editor/editor_main_screen.h"
#include "editor/editor_node.h"
#include "editor/editor_paths.h"
#include "editor/editor_quick_open.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include "editor/editor_quick_open.h"
#include "editor/gui/editor_quick_open_dialog.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the dialog is used and some features changed. It will require a bit more changes.

@ydeltastar
Copy link
Contributor Author

It is a close-on-selection dialogue now. Support for multi-selection was removed.
Will it be re-added or should I go with what it is now?

@ydeltastar
Copy link
Contributor Author

ydeltastar commented Oct 3, 2024

I changed the arguments to match the new implementation. Also, there was no way to tell if the dialog was canceled since the new implementation uses a callback instead of signals and it doesn't call when canceled. I made changes so it calls with an empty string when canceled.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 3, 2024

editor/gui/editor_quick_open_dialog.cpp:94 - Condition "p_base_types.is_empty()" is true.

error when using default second argument. You can make it default to ["Resource"].

I made changes so it calls with an empty string when canceled.

Is it a problem that canceled dialog doesn't call the callback?

Will it be re-added or should I go with what it is now?

Opening multiple files with the dialog is rather rare, so it was removed to allow opening the file faster.

@ydeltastar
Copy link
Contributor Author

error when using default second argument. You can make it default to ["Resource"].

Not sure if that's possible with the binding API, I couldn't find any example. I made it a required argument with an error check.

Is it a problem that canceled dialog doesn't call the callback?

Not for the editor's current behavior. But for addons the user may want a way to know. The other popups call it when canceled as well.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 3, 2024

You could use popup_hide signal then.

Not sure if that's possible with the binding API, I couldn't find any example.

You can make a TypedArray variable, append default and put it in DEFVAL.

@@ -336,6 +337,20 @@ void EditorInterface::popup_property_selector(Object *p_object, const Callable &
property_selector->connect(SNAME("canceled"), canceled_callback, CONNECT_DEFERRED);
}

void EditorInterface::popup_quick_open(const Callable &p_callback, const TypedArray<StringName> &p_base_types) {
ERR_FAIL_COND_MSG(p_base_types.is_empty(), "The list of base types for quick open dialog can't be empty.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you can just append "Resource" when the array is empty.

@ydeltastar
Copy link
Contributor Author

ydeltastar commented Oct 3, 2024

You could use popup_hide signal then.

Windows don't have it. Used canceled since EditorQuickOpenDialog is an AcceptDialog.

@ydeltastar ydeltastar force-pushed the quickly-quick-open branch 2 times, most recently from 70b2abe to 6f4e171 Compare October 3, 2024 22:59
@ydeltastar
Copy link
Contributor Author

You can make a TypedArray variable, append default and put it in DEFVAL.

Although it works fine CI is failing because the test suite doesn't support appended values for TypedArrays.

godot\tests/core/object/test_class_db.h(453): ERROR: CHECK_FALSE( !arg_defval_valid_data ) is NOT correct!
  values: CHECK_FALSE( true )
  logged: Invalid default value for parameter 'base_types' of method 'EditorInterface.popup_quick_open'. Must be zero.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2024

Well after you made popup_quick_open() auto-append if empty, you can make the default empty again.

editor/editor_interface.cpp Outdated Show resolved Hide resolved
Comment on lines 345 to 346
for (int i = 0; i < p_base_types.size(); i++) {
base_types.append(p_base_types[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i = 0; i < p_base_types.size(); i++) {
base_types.append(p_base_types[i]);
for (const StringName &type : p_base_types) {
base_types.append(type);

Copy link
Contributor Author

@ydeltastar ydeltastar Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another thing with TypedArrays that fail CI even though it works fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you need to use Variant as type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much benefit. TypedArrays are iterated with regular loops throughout the code base, including other PRs I did.

Copy link
Member

@KoBeWi KoBeWi Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely because range iteration on Array wasn't supported until recently and there is legacy code.
But whatever, someone might make a cleanup pass in the future anyway.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2024

Looks good now. One last thing that would be nice is checking for supported types. QuickOpen dialog only supports Resource types, if you use e.g. Node or some non-existent type, the dialog will be empty with no proper feedback.

You can check valid types like this:

for (const StringName &type : p_base_types) {
	ERR_FAIL_COND_MSG(!ClassDB::is_parent_class(type, "Resource"), "Only Resource-deriving types are supported in QuickOpen.");
}

It should also be documented in the method's description.

@ydeltastar ydeltastar force-pushed the quickly-quick-open branch 2 times, most recently from 3b46b9d to 775b58b Compare October 4, 2024 14:22
editor/editor_interface.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Oct 4, 2024
@akien-mga akien-mga merged commit 428c4a6 into godotengine:master Oct 4, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make EditorQuickOpen dialog available from plugins
4 participants