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

Extract StyleBoxFlat, StyleBoxTexture and StyleBoxLine in their own file #68396

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

Geometror
Copy link
Member

Part of the ongoing effort to have only one class per file with the goal of improving the structure of the codebase in terms of readability and compilation speed.
style_box.cpp contained already over 1000 LOC, which made navigation quite cumbersome.
In addition, some methods and members have been reordered to further improve clarity.

@Geometror Geometror added this to the 4.0 milestone Nov 8, 2022
@Geometror Geometror requested review from a team as code owners November 8, 2022 02:27
@Mickeon
Copy link
Contributor

Mickeon commented Nov 8, 2022

I agree with this greatly, but where has the "ongoing effort" been stated, if anywhere?

A lot of classes could benefit from it, absolutely, but it makes me wonder if it would be worth it to group them in sub-folders someday and what is the argument against it.

@Geometror
Copy link
Member Author

Geometror commented Nov 8, 2022

It came up on RC a few times and in the discussion around #33027 as well, but like some other things we discussed it was never written down formally in https://docs.godotengine.org/en/latest/community/contributing/code_style_guidelines.html. We still need to do that.
Grouping some header/source files together in a sub-folder could make sense, but one (minor) argument against this would be that having a deeper folder structure impedes navigation (especially in tree views).

@Geometror Geometror force-pushed the split-stylebox branch 2 times, most recently from b20ba12 to 1b3e796 Compare November 8, 2022 17:44
@akien-mga
Copy link
Member

I approved the change in principle but I'm not sure this is the right time to merge such changes, as there's a lot more that would need splitting if we decide that this is the new standard. Started a discussion in maintainer channel, we can formalize this in a proposal later on to document the direction we want to go for. For now I'd tend to keep this and similar changes for 4.1.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Jan 13, 2023
@YuriSizov
Copy link
Contributor

With Akien's comment in mind and the timing, I've kicked this to 4.x (realistically to 4.1) for now.

@YuriSizov YuriSizov self-requested a review July 10, 2023 14:18
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 10, 2023
@YuriSizov
Copy link
Contributor

@Geometror I think we are ready to merge it in the next couple of weeks. So if you could do a rebase, it would be great!

@Geometror Geometror force-pushed the split-stylebox branch 3 times, most recently from 4d5e03c to b63d6a6 Compare July 14, 2023 20:35
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Mostly good, some changes are still required.

editor/editor_themes.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.h Show resolved Hide resolved
editor/plugins/canvas_item_editor_plugin.h Outdated Show resolved Hide resolved
scene/resources/style_box.cpp Outdated Show resolved Hide resolved
scene/resources/style_box_flat.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov merged commit b7c3998 into godotengine:master Jul 17, 2023
@YuriSizov
Copy link
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