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 call validation to CommandPalette #82194

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 23, 2023

Code from confused user™:

@tool
extends EditorScript

func bundle_audio():
	print("Hello World")
	
func _run():
	var command_palette = get_editor_interface().get_command_palette()

	var name = "audio/bundle"
	command_palette.remove_command(name)
	
	command_palette.add_command("Bundle Audio", name, bundle_audio) 

	print("command added")

The EditorScript instance is gone before the command is run, so it can't be called. However editor palette doesn't do any call validation and it just fails silently.

This PR adds error messages:

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 23, 2023

I would probably use a toast for this. I don't see a reason to raise a blocking popup. But otherwise seems like a good idea!

@KoBeWi KoBeWi force-pushed the your_command_failed._Good_luck_finding_out_why branch from 343a9e9 to abd4652 Compare September 23, 2023 15:09
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 23, 2023

image

@KoBeWi KoBeWi force-pushed the your_command_failed._Good_luck_finding_out_why branch from abd4652 to 7d52d9f Compare September 23, 2023 15:13
@akien-mga
Copy link
Member

Fails building:

In file included from ./core/object/method_bind.h:34,
                 from ./core/object/class_db.h:34,
                 from ./core/object/ref_counted.h:34,
                 from ./core/io/resource_uid.h:34,
                 from ./core/io/resource.h:34,
                 from ./core/input/input_event.h:35,
                 from ./core/input/shortcut.h:34,
                 from ./editor/editor_command_palette.h:34,
                 from editor/editor_command_palette.cpp:31:
./core/variant/binder_common.h: In instantiation of 'void call_with_variant_args_helper(T*, void (T::*)(P ...), const Variant**, Callable::CallError&, IndexSequence<Is ...>) [with T = EditorCommandPalette; P = {String&}; long unsigned int ...Is = {0}]':
./core/variant/binder_common.h:417:40:   required from 'void call_with_variant_args(T*, void (T::*)(P ...), const Variant**, int, Callable::CallError&) [with T = EditorCommandPalette; P = {String&}]'
./core/object/callable_method_pointer.h:104:25:   required from 'void CallableCustomMethodPointer<T, P>::call(const Variant**, int, Variant&, Callable::CallError&) const [with T = EditorCommandPalette; P = {String&}]'
./core/object/callable_method_pointer.h:100:15:   required from here
Error: ./core/variant/binder_common.h:303:25: error: cannot bind non-const lvalue reference of type 'String&' to an rvalue of type 'String'
  303 |  (p_instance->*p_method)(VariantCasterAndValidate<P>::cast(p_args, Is, r_error)...);
      |  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 24, 2023

I know, but I can't decipher the error.

@akien-mga
Copy link
Member

execute_command takes a String &, but you're passing a String.
Apparently the direct call syntax handles that fine but callable_mp is more picky.

@KoBeWi KoBeWi force-pushed the your_command_failed._Good_luck_finding_out_why branch from 7d52d9f to 9f0b8c0 Compare September 24, 2023 22:10
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 24, 2023

I added const and it magically works now.

@akien-mga akien-mga merged commit f0a9808 into godotengine:master Sep 25, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the your_command_failed._Good_luck_finding_out_why branch September 25, 2023 15:30
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.

3 participants