godot
godot copied to clipboard
godotengine
Allow configuration warnings to refer to a property
Closes https://github.com/godotengine/godot-proposals/issues/550
Configuration warnings changed from PackedStringArray to an Array that accepts a mix of Strings and Dictionaries { message: String, property?: String }.
If property is set, then show a clickable warning icon on property in the inspector.
Strings in this array are converted to an equivalent Dictionary with message filled.
This PR also lays the groundwork for further configuration warning enhancements like https://github.com/godotengine/godot-proposals/issues/2519
As an example, the PointLight2D node was updated to use this system.
https://user-images.githubusercontent.com/5117197/200654466-370da225-17ea-4d90-85c5-04b5bcd7bc49.mp4
The PR is split into two commits for easier reviewing. The second commit only fixes the signature of the get_configuration_warnings method for all classes using it.
TODO:
- [x] Change warning icon based on how many warnings the property has. Probably have to move the property name filtering logic out of the
as_stringmethod. - [ ] Would be nice to have a tooltip when hovering the icon, but since other icons don't have this yet, it can be done in a follow-up PR.
- [x] It seems to be necessary to introduce a compatibility method for GDExtension here.
- [ ] No idea how to test it, so someone should verify what it's doing is correct. Especially since there's also a virtual method involved here, which is not covered by this change (I followed what was done in AnimationMixer.compat.inc)!
Wow... what work. My only concern is that it's not just a breaking change after 4.0, but it'd make the virtual method ever so laborious to use for simpler cases. Having to create { message: "Something is wrong etc. etc."}, that is.
I can assume that my concerns are exaggerated, as not everyone casually makes use of the method, but it makes me wonder if it could be an Array of Variant, where each one of them can either be a String or a Dictionary. This way the previous, simplistic behavior is also kept. Having it as a Dictionary is definitely the way to go, however.
@Mickeon I totally agree. Since there is no concrete proposal behind this, I implemented it in the most straightforward way I could think of. But for the sake of compatibility and convenience, I'd agree that treating plain strings just like before would be nice.
Would like some more opinions on this before I try implementing anything more, e.g. if this is the kind of API breakage that's also acceptable for minor versions (since like you said, this API isn't commonly used as it stands).
I think it's a bit weird duplicating config warnings like this. It gives off a feeling that two things are wrong, instead of just one.
If I gather correctly, this can be implemented in a not compat-breaking way, by having separate "node configuration" and "property configuration" warnings? Here's my idea:
- Keep config warnings as is.
- Have property warnings point to a property with a warning message.
- Property warnings update the config warning in standardized ways, i.e. "[property_name] of this [node_name] has not been set up properly."
When I thought on how to implement this, I also thought of using a general String format, kind of like hint_strings, to expand the current behavior. But I do get the use of Dictionaries. I believe it allows greater control in the future. Imagine being able to click on the PhysicsBody2D's CollisionShape warning, press a button to swiftly fix the issue, and a related action Callable opens the Add Node dialog. Very convenient and user-friendly.
Updated so that get_configuration_warnings may be a mixed array of Strings and Dictionaries. This keeps compatibility with existing C++ and GDScript code, and keeps an easy way to write simple configuration warnings.
Also for easier review, I split the changes into two commits, since one is just a large batch of signature changes which do not need reviewing.
I checked the code and the implementation seems fine, but changing return type still breaks compatibility. Although Array and PackedStringArray are mostly compatible so maybe it's not a big deal.
Yyyeaah, this one cannot be done now without breaking compatibility, unless it is possible for it not to.
Hi @Mickeon , your PR #87535 removed Node::get_configuration_warnings_as_string which I intended to use here.
Clicking on the :warning: button that appears next to a property, a dialog similar to the one from the scene tree would show up. It would filter to only show relevant warnings, so the function would get an optional property parameter. (As discussed above, tooltips aren't currently implemented for inspector buttons, so the dialog would be the primary way to get this info for now...)
Without the function, this would involve duplicating the logic involving the bullet points, indenting new lines etc.
Do you think it would be fine to re-introduce it here, now that there'd be multiple places that need it for different purposes? Or was there a strong reason against having that function around in the first place?
Do you think it would be fine to re-introduce it here, now that there'd be multiple places that need it for different purposes? Or was there a strong reason against having that function around in the first place?
The reason it was removed was because it was only used two times: The tooltip, and the popup. However, the way these two are displayed is entirely different, to the point where relying on that function's result was basically useless, as the strings had to be reformatted anyhow. It also was not encompassed with TOOLS_ENABLED, which included it in release builds for no reason (if the compiler is stupid).
With that said, I am not sure if it should be brought back. At the same time, this PR may override what was made in https://github.com/godotengine/godot/pull/87535, so you may as well... go all in?
The original popup itself has always kind of sucked, this may be an opportunity to refactor it outright.
What should GDScript plugins do if they support engine versions prior this change?
func _get_configuration_warnings() -> PackedStringArray:
Updated so that get_configuration_warnings may be a mixed array of Strings and Dictionaries. This keeps compatibility with existing C++ and GDScript code
GDScript doesn't have a preprocessor to make this conditionally implemented differently. If I write -> Array, it will break in versions prior this change.
That means I have to remove typing.
That means I have to remove typing completely.
Yes, that's the solution if you want to support older versions.