Skip to content

Simplify ScriptServer::get_global_class_list#108577

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
YYF233333:global_class_list
Sep 30, 2025
Merged

Simplify ScriptServer::get_global_class_list#108577
Repiteo merged 1 commit into
godotengine:masterfrom
YYF233333:global_class_list

Conversation

@YYF233333

Copy link
Copy Markdown
Contributor

The implementation of this function is rather awkward: it first copies the elements into a List, then copies them again into r_global_classes. We can simply return the intermediate List so that users can use it directly.

Since I was already working on this code, I also took the opportunity to tidy up some variable names and change unnecessary Lists to LocalVectors.

@YYF233333 YYF233333 requested review from a team as code owners July 13, 2025 14:10
Comment thread editor/gui/create_dialog.cpp Outdated
Comment on lines 80 to 84

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Considering this use-case, I'd probably keep using a parameter (instead of returning a new list), but make it use LocalVector for both get_class_list and get_global_class_list. You can use SortArray to sort just the subsection of the newly added elements to preserve behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Change as suggested. I'm not sure if I should change get_class_list to use SortArray, it currently sort all values in the list (although I think it is always given an empty list). Not sure which meaning is correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine, especially since the other one only sorts the added elements. Fingers crossed nothing relies on the previous behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a comment to clarify the current behavior of these functions.

@Ivorforce Ivorforce left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

Comment thread editor/doc/doc_tools.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure why it has to be moved to back, but if it has an effect on which other classes can register properties on it, we probably shouldn't move the final element into its place. Instead, the element should be removed and then re-added to back (same as move_to_back).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my bad. I must have misunderstood how move_to_back is implemented. Fixed.

Comment thread editor/gui/create_dialog.cpp Outdated
Comment on lines 80 to 84

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine, especially since the other one only sorts the added elements. Fingers crossed nothing relies on the previous behavior.

@Ivorforce Ivorforce modified the milestones: 4.x, 4.6 Jul 13, 2025
@YYF233333 YYF233333 force-pushed the global_class_list branch 3 times, most recently from 0ee3ce3 to 5beacd6 Compare July 14, 2025 06:26
@YYF233333 YYF233333 requested a review from a team September 16, 2025 07:00
@Repiteo Repiteo merged commit fdf32d1 into godotengine:master Sep 30, 2025
20 checks passed
@Repiteo

Repiteo commented Sep 30, 2025

Copy link
Copy Markdown
Contributor

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.

4 participants