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

Implement hidden class in ScriptServer to replace EditorX class being hidden. #91020

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ajreckof
Copy link
Member

@ajreckof ajreckof commented Apr 22, 2024

Implements godotengine/godot-proposals#1047
This is the ScriptServer counterpart of #90765

This enables marking scripts as hidden for any ScriptServer. I have done the GdScript implementation (similar to @tool) but have yet to do the same for C# (never built or even used C# so might need some help with that).

For the GdScript syntax this is what I went for as a recommended syntax

@hide class_name CustomClass
extends RichTextLabel

but any place before class_name would work except for this

extends RichTextLabel
@hide class_name CustomClass

@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch 2 times, most recently from e81494b to 3dfb4c7 Compare April 23, 2024 12:25
@AThousandShips AThousandShips added this to the 4.x milestone Apr 23, 2024
@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch from 3dfb4c7 to 350113e Compare April 23, 2024 17:24
@ajreckof ajreckof marked this pull request as ready for review April 23, 2024 17:25
@ajreckof ajreckof requested review from a team as code owners April 23, 2024 17:25
@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch 3 times, most recently from 2b9fb92 to 3ab8c2a Compare April 23, 2024 19:40
@ajreckof
Copy link
Member Author

So I've added the C# implementation and separated the C# and Gdscript implementation into their own commits separate from the base one introducing the change in ScriptServer for easier reviewing (could be squashed later).
I'm not sure where to document the new C# attribute if someone could indicate the right place to put it ? (maybe it should be in the godot-docs repo?)

@raulsntos
Copy link
Member

I'm not sure where to document the new C# attribute if someone could indicate the right place to put it ? (maybe it should be in the godot-docs repo?)

The XMLDoc in the C# files should be the equivalent to the documentation you added in @GDScript.xml. For additional documentation you could add it to godot-docs: GDScript reference: Registering named classes for GDScript and C# global classes for C#.

@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch 2 times, most recently from 83ae3b8 to 8bb2abd Compare April 23, 2024 21:34
@ajreckof ajreckof requested a review from raulsntos April 23, 2024 23:06
@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch from 8bb2abd to f5092bc Compare April 24, 2024 03:13
<annotation name="@hide">
<return type="void" />
<description>
Prevent the current class from appearing in the Create Node dialog.
Copy link
Member

Choose a reason for hiding this comment

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

This also applies to Create Resource etc.

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.

Looks fine on editor side.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

The C# changes look good to me. It works as expected when using it with C# scripts.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 1, 2024
@akien-mga akien-mga modified the milestones: 4.3, 4.x May 1, 2024
@@ -429,6 +431,10 @@ void ScriptServer::remove_global_class_by_path(const String &p_path) {
bool ScriptServer::is_global_class(const StringName &p_class) {
return global_classes.has(p_class);
}
bool ScriptServer::is_class_hidden(const StringName &p_class) {
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
bool ScriptServer::is_class_hidden(const StringName &p_class) {
bool ScriptServer::is_class_hidden(const StringName &p_class) {

@adamscott
Copy link
Member

Hi there @ajreckof, my friend! I made a PR not knowing about yours.

I saw your code and it seems very complicated compared to my version. I don't know why we should bind the "hidden" property only to global classes, as it can simply be a boolean set the same way @tool sets is_tool to be true in scripts.

@raulsntos
Copy link
Member

Why would you hide a script that is not a global class? Those are already not shown in the Add Node/Create Resource dialogs.

@mihe
Copy link
Contributor

mihe commented Mar 14, 2025

@ajreckof Any chance that the merge conflicts in this PR (which mostly seem to come from #101489) could be resolved? It would be great to try to get this approved and merged for 4.5.

@ajreckof
Copy link
Member Author

ajreckof commented Mar 15, 2025

@ajreckof Any chance that the merge conflicts in this PR (which mostly seem to come from #101489) could be resolved? It would be great to try to get this approved and merged for 4.5.

I fully agree we first need to get the parent PR merged which I just rebased and answered the last concern. After that we can get this one merged. It would have been great to add this one to 4.4 but unfortunately I was too taken with work and I couldn't push for it.
Edit : forget what i said! I forgot the two PR were independent and just tightly related. So I'm rebasing it right now. I still think both should still be merged on the same pass as this one doesn't make sense without the other.

@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch from f5092bc to 59d0fcb Compare March 15, 2025 19:00
@ajreckof ajreckof requested review from a team as code owners March 15, 2025 19:00
@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch from 59d0fcb to d52b9e9 Compare March 15, 2025 20:13
@ajreckof ajreckof force-pushed the Implement-hidden-class-in-ScriptServer-to-replace-EditorX-class-being-hidden- branch from d52b9e9 to 6f1f1fc Compare March 15, 2025 20:19
ERR_FAIL_COND_MSG(p_class == p_base || (global_classes.has(p_base) && get_global_class_native_base(p_base) == p_class), "Cyclic inheritance in script class.");
GlobalScriptClass *existing = global_classes.getptr(p_class);
if (existing) {
// Update an existing class (only set dirty if something changed).
if (existing->base != p_base || existing->path != p_path || existing->language != p_language) {
if (existing->base != p_base || existing->path != p_path || existing->language != p_language || existing->language != p_language) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This new condition seems to be a duplicate.

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.

7 participants