Refactor BoxContainer, Expose New Properties#6598
Open
Aeshus wants to merge 21 commits into
Open
Conversation
I didn't realize that it wouldn't run setter = null initially, so it wouldn't do what I wanted. So much for caching the getter...
I didn't read that CSS book for nothing, justify != align!! Plus, added some more stuff to the exception and just had to update a lot of docs...
Contributor
Author
|
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 :).) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
SeparationOverridegetter 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
Alignmissing anInvalidateArrange()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
SeparationOverridefor working with what should just be aSeparationXML 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
SeparationOverridesetter (in e.g. XAML, only produces a warning) can be REGEX replaced by justSeparation.If they relied on getting/reading any of the properties of BoxContainer (which does not happen in content), they would need to do
.Valueon it.If they ever read
BoxContainer.SeparationOverrideto 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 toBoxContainer.Separation's getter.Changes
New
StylePropertyOrientationandStylePropertyAlignproperties to set defaults forBoxContainer.BoxContainer.SeparationOverrideis 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, andBoxContainer.Separationare all now[NotNull]nullable types. This means that when getting its value (not used in ss14 content), you have to do.Valueto read its value, but you don't need to check it's non-null first. This allows you to set its value tonull, which will fallback from your override to the default.