Skip to content

Refactor BoxContainer, Expose New Properties#6598

Open
Aeshus wants to merge 21 commits into
space-wizards:masterfrom
Aeshus:box-container-2.0
Open

Refactor BoxContainer, Expose New Properties#6598
Aeshus wants to merge 21 commits into
space-wizards:masterfrom
Aeshus:box-container-2.0

Conversation

@Aeshus
Copy link
Copy Markdown
Contributor

@Aeshus Aeshus commented May 27, 2026

About the PR

Most of the diff is just me adding documentation + formatting the files. I've put them in specific commits separate from my code modifications for easier review.

The main code change was that I've now exposed overrides and style properties for all of the properties of a BoxContainer. This means they all can fall back to stylesheet if it's not directly modified, which if there's no stylesheet it would default back to the original default.

This does have some breaking changes though. First, all of the properties are now nullable but marked at [NotNull]. If they were reference types iirc I could use [AllowNull], but I couldn't in this instance. I couldn't find any instances of this being a problem in the codebase as of yet, but it's still a breaking change.

Plus, I had to explicitly obsolete error out the SeparationOverride getter as it used to be able to tell if there's been an explicit override by checking if it was non-null, but now it's always non-null. I could probably add another field to allow for the nullable check, but imo there's not a real benefit for what is already going to cause breaking changes anyways just for an obsolete field (ignoring the fact that separation override is implemented incorrectly in all engine builds except master, so I think it's a fair cut).

This also fixes Align missing an InvalidateArrange() on its set path.

Also, it has 100% test coverage, and everything is very well documented now.

Why / Balance

I want a consistent way to work with properties of controls. I found it really annoying that we had to type out SeparationOverride for working with what should just be a Separation XML property.

Adding all of these overrides and this functionality will let us hoist up properties to the top-level of the compositional controls as we discussed in the DevBus, as now if they don't modify a hoisted property, it can default to the control's own styleproperty defaults.

Media

N/A

Migration(s)

Content master requires no modifications. All instances of the SeparationOverride setter (in e.g. XAML, only produces a warning) can be REGEX replaced by just Separation.

If they relied on getting/reading any of the properties of BoxContainer (which does not happen in content), they would need to do .Value on it.

If they ever read BoxContainer.SeparationOverride to check if it is currently using an overridden value (which was broken in all versions except master as it didn't actually override) by seeing if it is non-null, then there is no support for that behavior anymore. If they just wanted to read the current value, they can switch to BoxContainer.Separation's getter.

Changes

New StylePropertyOrientation and StylePropertyAlign properties to set defaults for BoxContainer.

BoxContainer.SeparationOverride is now obsolete. The getter now produces an error as the semantics are now different (not used in ss14), and the setter produces a warning.

BoxContainer.Orientation, BoxContainer.Align, and BoxContainer.Separation are all now [NotNull] nullable types. This means that when getting its value (not used in ss14 content), you have to do .Value to read its value, but you don't need to check it's non-null first. This allows you to set its value to null, which will fallback from your override to the default.

@Aeshus Aeshus requested review from DrSmugleaf and PJB3005 as code owners May 27, 2026 18:02
@Aeshus
Copy link
Copy Markdown
Contributor Author

Aeshus commented May 27, 2026

If y'all like the way this looks, I'm planning to go through all of the controls to update them to a similar style so all the breaking changes can be in one engine release (and maybe I can squeeze in my typed style property thingies :).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant