Editor: Remove unused Class Name field from Create Script dialog #78573
Conversation
|
What happens when a user don't supply a class name? |
|
The template line is removed, at least that's how it seems in the source |
There was a problem hiding this comment.
As this would need thorough testing (as this is something that is used frequently), I suggest to merge this PR right after the 4.1 release. We will be able to get some feedback from the users if this PR is alright.
So, you get my pass. Thanks @dalexeev!
Good point. The field is not filled automatically. If the user leaves it empty, then a script without
I'm not rushing anyone with this, sorry if I'm distracting. I just did not find today how I can help with 4.1. |
Don't worry. @akien-mga said that he was happy to have PRs ready to merge right after the release of 4.1. |
Maybe we could otherwise add a green statement that say that the class will have a specified class name if one is submitted. If there's no class, then just don't have that statement. Usually, placeholders are used to define what is needed in the field, not that a field is optional. |
|
Yeah it's good to think about the UX a bit and the implications here. If the UI encourages users to name all their classes, they might end up with hundreds of global named classes where it's not needed (and thus potential conflicts, or slowdowns in parts of the editor that rely on querying global classes). |
The UI could say something like |
editor/script_create_dialog.cpp
Outdated
| void ScriptCreateDialog::_class_name_changed(const String &p_name) { | ||
| is_class_name_valid = _validate_class(class_name->get_text()); | ||
| is_class_name_valid = class_name->get_text().is_empty() || class_name->get_text().is_valid_identifier(); |
There was a problem hiding this comment.
Maybe we should check if the class has a name already used by a native or a global class. Then, we could output a custom error message about that.
|
Is this really the right move though? I think it would be better to remove this field. It's easy for the user to write |
|
There is an issue and a proposal about it, and there is also a similar proposal about the node name field in the node creation dialog, so I thought that users want this feature. But given the possible misuse of this field, I agree with you that it's better to remove it. |
dalexeev
force-pushed
the
editor-create-script-class-name
branch
from
d7959a8
to
ea1a0e0
Compare
dalexeev
changed the title
|
I have updated the PR. But it is possible that in the process we can return to the old version or come to something else. |
|
Be careful removing the feature, AFAIK it's required for C# scripts (and that's the reason this was added in the script create dialog). Any C# class needs to have a unique name (but it's not the same as the global script classes mechanism). |
|
I haven't tested this, but for C# nothing should change, the class name is generated from the file name as before. |
There was a problem hiding this comment.
I support this change, but I won't give it an approval because there are some other possibly unrelated changes in this PR like changes to the script templates, and I don't know if those are desired to have in the same PR or if they should be split off (or maybe, the removal of the unused field should be the one split off).
For C#, it is generally expected that the class name should match the file name, so I don't think we need a separate field for specifying the class name.
dalexeev
force-pushed
the
editor-create-script-class-name
branch
from
ea1a0e0
to
26ce861
Compare
|
Thanks! |
It looks like the field hasn't done anything for a very long time, either with GDScript or C# (link).
class_nameproperty through the Create Script menu godot-proposals#208.