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

Add popup_create_dialog() for EditorInterface to create custom create dialog #100135

Merged

Conversation

Lazy-Rabbit-2001
Copy link
Contributor

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 commented Dec 7, 2024

Supersedes #99926

Adds a new method pop_create_dialog() for EditorInterface, which allows users to pop a custom create dialog.

Parameters

Callable callback: This callback requires only one argument which is of StringName type, where a user can define instantiating an object by calling ClassDB.instantiate(...) or Script.new(...).
StringName base_type: This defines the root type of the create dialog. By default it is Object. For example, if one defines this as Node2D, only a Node2D and all of its derived class can be displayed.
String dialog_title: This allows you tp customize the title of the create dialog. An empty string for this will make the title fall back to "Create New ".
Array[StringName] type_blocklist: This will customize which types should be hidden from the custom create dialog. A hidden type is unable to be selected or interacted.
Dictionary type_suffixes: This will customize the suffixes of types shown in the custom create dialog. Suffixes, for global script classes, are by default the file names of the scripts. If a suffix is an empty string, there will be no suffix following the type name.

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested review from a team as code owners December 7, 2024 04:15
@YeldhamDev YeldhamDev added this to the 4.x milestone Dec 7, 2024
@Lazy-Rabbit-2001 Lazy-Rabbit-2001 force-pushed the expose-create-dialog-new branch from 94dd750 to 91b4317 Compare December 7, 2024 05:13
@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested a review from a team as a code owner December 7, 2024 08:53
@AThousandShips AThousandShips changed the title Adds pop_create_dialog() for EditorInterface to create custom create dialog Add pop_create_dialog() for EditorInterface to create custom create dialog Dec 7, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Dec 7, 2024

  • Type blacklist doesn't take effect immediately:
i1f6SKNmzW.mp4

You need to close the dialog and open it again.

  • Canceling the dialog does not call the callback. It should be called with empty String, for consistency with other popup methods.
  • Type suffixes have the same problem as blacklist.
  • Also there is no way to remove a suffix. If you use empty String, the previous valid suffix will be repeated.
    Looking at the proposal, empty String is supposed to remove the suffix altogether. I'd suggest the following:
  • If you pass an empty string as suffix, remove the suffix.
  • If a class is no longer in suffix Dictionary (in subsequent call), restore default suffix.

I think exposing the dialog this way makes sense, but the additional functionality (blacklist, suffixes) is questionable. The proposal doesn't seem to have gained support. Can't say how much useful it is.

@AThousandShips
Copy link
Member

Should preferably be called "blocklist" instead, as per the general pattern and preferred terminology

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 force-pushed the expose-create-dialog-new branch 2 times, most recently from 4e5d629 to d776185 Compare December 8, 2024 11:01
@Lazy-Rabbit-2001
Copy link
Contributor Author

  • Type blacklist doesn't take effect immediately:
  • Canceling the dialog does not call the callback. It should be called with empty String, for consistency with other popup methods.
  • Type suffixes have the same problem as blacklist.
  • Also there is no way to remove a suffix. If you use empty String, the previous valid suffix will be repeated.
    Looking at the proposal, empty String is supposed to remove the suffix altogether.

All of these bugs are fixed

@KoBeWi

This comment was marked as resolved.

@@ -145,6 +148,7 @@ class EditorInterface : public Object {
void popup_property_selector(Object *p_object, const Callable &p_callback, const PackedInt32Array &p_type_filter = PackedInt32Array(), const String &p_current_value = String());
void popup_method_selector(Object *p_object, const Callable &p_callback, const String &p_current_value = String());
void popup_quick_open(const Callable &p_callback, const TypedArray<StringName> &p_base_types = TypedArray<StringName>());
void popup_create_dialog(const Callable &p_callback, const StringName &p_base_type = "", const TypedArray<StringName> &p_custom_type_blacklist = TypedArray<String>(), const Dictionary &p_custom_suffix = Dictionary());
Copy link
Member

Choose a reason for hiding this comment

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

The Dictionary could be typed.
(AFAIK you ran into some problems with it; it would still be nice to resolve it though)

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 Dictionary could be typed. (AFAIK you ran into some problems with it; it would still be nice to resolve it though)

Yeah, but I'd wait for others who maintains the typed dictionary to solve the problem and then I'll make a new pr to transform all dictionaries into typed dictionaries (as long as their keys or values should be typed)

Copy link
Member

Choose a reason for hiding this comment

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

Well the problem is that adding type technically breaks compatibility, so we should do it swiftly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nvm, wait for someone to fix #100137 ...

editor/editor_interface.h Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/create_dialog.h Outdated Show resolved Hide resolved
editor/create_dialog.h Outdated Show resolved Hide resolved
editor/create_dialog.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Dec 9, 2024

The replace mode that you newly exposed sounds like niche feature 🤔 It could default to false and the argument could be moved before replace_type_name. Though not sure if it's even useful.

doc/classes/EditorInterface.xml Outdated Show resolved Hide resolved
editor/editor_interface.h Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Dec 9, 2024

The replace mode that you newly exposed sounds like niche feature 🤔 It could default to false and the argument could be moved before replace_type_name. Though not sure if it's even useful.

I prefer to make it exposed, as some users will pop the dialog for literally replacing the type.
Or add a method to override the title, which is also ok.

editor/editor_interface.cpp Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Dec 10, 2024

The crash is still not fixed #100135 (comment)

@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Dec 10, 2024

The crash is still not fixed #100135 (comment)

Fixed in the latest pr that will be pushed soon

...
create_dialog->set_type_suffixes(suffix_map);

const bool is_type_valid = ClassDB::class_exists(p_base_type) || ScriptServer::is_global_class(p_base_type);
if (!is_type_valid) {
	ERR_PRINT(vformat("Invalid base type '%s'. The base type has fallen back to 'Object'.", p_base_type));
}
const String safe_base_type = !p_base_type.is_empty() && is_type_valid ? p_base_type : "Object";

create_dialog->set_base_type(safe_base_type);
...

editor/editor_interface.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Functionality-wise it's fine now.
Apparently type blocklist already existed, it was just hard-coded before.
The suffixes and dialog title are questionable, but eh.

editor/editor_interface.h Outdated Show resolved Hide resolved
editor/editor_interface.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips changed the title Add pop_create_dialog() for EditorInterface to create custom create dialog Add popup_create_dialog() for EditorInterface to create custom create dialog Dec 12, 2024
@akien-mga akien-mga removed the request for review from a team December 12, 2024 12:44
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 12, 2024
@akien-mga akien-mga merged commit 4cf1b0d into godotengine:master Dec 12, 2024
20 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.

5 participants