General Character Editor Improvements - #2252
Conversation
DEATHB4DEFEAT
added
Size: 3-Medium
github-actions
Bot
added
Changes: C#
SimpleStation14
changed the title
DEATHB4DEFEAT
changed the title
…ngines into cooler-cooler-markings
This comment was marked as spam.
This comment was marked as spam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
Content.Client/Humanoid/MarkingPicker.xaml.cs (3)
1-17:⚠️ Potential issue
System.Collections.Genericimport is missingJust like the Sol‑bred slip‑up above,
List<T>et al. are used everywhere but never imported, dooming the whole editor to fail at the first compile.using System.Linq; using System.Numerics; +using System.Collections.Generic;
188-193: 🛠️ Refactor suggestionDouble‑localisation leads to garbled sort order
OrderBy(p => Loc.GetString(GetMarkingName(p)))callsLoc.GetStringtwice.GetMarkingNamealready returns the localised string, so the second call just re‑feeds an English sentence into the FTL lookup and probably returns the key. Sort will then be nonsense.- ).OrderBy(p => Loc.GetString(GetMarkingName(p))); + ).OrderBy(p => GetMarkingName(p));
315-345:⚠️ Potential issue
continue;renders the local function unreachable – won’t compileThe
continue;placed before the local functionChangedmakes that function unreachable, which C# forbids. Replace the pattern with an inline lambda.- colorSelector.OnColorChanged += Changed; - continue; - - void Changed(Color _) - { - _currentMarkingColors[colorIndex] = colorSelector.Color; - ColorChanged(prototype, colorIndex); - } + colorSelector.OnColorChanged += _ => + { + _currentMarkingColors[colorIndex] = colorSelector.Color; + ColorChanged(prototype, colorIndex); + };
🧹 Nitpick comments (6)
Content.Client/UserInterface/Controls/AlternatingBGContainer.xaml (1)
7-13: Consider enabling vertical expansion for the inner ScrollContainerRight now
ContentScrollContaineris scroll‑able but not set toVerticalExpand="True".
If the parent receives extra vertical space, the scroll area will stay its minimum height and the children will appear clipped until the user starts scrolling – not a great first impression for new recruits.- <ScrollContainer + <ScrollContainer Name="ContentScrollContainer" HorizontalExpand="True" + VerticalExpand="True" HScrollEnabled="False" VScrollEnabled="True" ReturnMeasure="True">Content.Client/Lobby/UI/HumanoidProfileEditor.xaml (1)
256-266: Minor accessibility nit – hard‑coded dark panel colourThe preview panel background
#2f2f2fis hard‑coded. Consider re‑using a colour from the shared palette (StyleBase.ColorDarkGreyetc.) so theme changes propagate automatically and contrast checks stay centralised.Content.Client/Lobby/UI/HumanoidProfileEditor.xaml.cs (2)
762-765: Give the Antag list container HorizontalExpand for consistent width
altis anAlternatingBGContainerinserted intoAntagList, but withoutHorizontalExpand = truethe column can collapse and mis‑align with the jobs list beside it.- var alt = new AlternatingBGContainer { Orientation = LayoutOrientation.Vertical, }; + var alt = new AlternatingBGContainer + { + Orientation = LayoutOrientation.Vertical, + HorizontalExpand = true, + };
1835-1839: Consolidate redundantInvalidateMeasure()callsCalling
InvalidateMeasure()four times in quick succession is overkill – the UI will already queue a full layout pass after the first call. A single call on the parent container (or on any oneSpriteView) is enough.- SpriteViewS.InvalidateMeasure(); - SpriteViewN.InvalidateMeasure(); - SpriteViewE.InvalidateMeasure(); - SpriteViewW.InvalidateMeasure(); + SpriteViewS.Parent?.InvalidateMeasure(); // one pass is sufficientContent.Client/UserInterface/Controls/AlternatingBGContainer.xaml.cs (1)
99-108:UpdateColorsassumes each panel has at least one child
child.Children.First()will throw if an emptyPanelContainersneaks in. A defensive check avoids a potential crash from rogue code.- child.Visible = child.Children.First().Visible; + if (child.ChildCount > 0) + child.Visible = child.Children.First().Visible;Content.Client/Humanoid/MarkingPicker.xaml.cs (1)
250-252: Ensure point display updates when filter changes
UpdatePoints()is called after adding/removing markings, but not afterPopulateif the filter merely changes. A stale points label misleads users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Content.Client/Humanoid/MarkingPicker.xaml(1 hunks)Content.Client/Humanoid/MarkingPicker.xaml.cs(11 hunks)Content.Client/Lobby/UI/HumanoidProfileEditor.xaml(8 hunks)Content.Client/Lobby/UI/HumanoidProfileEditor.xaml.cs(7 hunks)Content.Client/Lobby/UI/RequirementsSelector.xaml(1 hunks)Content.Client/UserInterface/Controls/AlternatingBGContainer.xaml(1 hunks)Content.Client/UserInterface/Controls/AlternatingBGContainer.xaml.cs(1 hunks)Content.Shared/Humanoid/Markings/MarkingPrototype.cs(1 hunks)Resources/Locale/en-US/preferences/ui/humanoid-profile-editor.ftl(1 hunks)Resources/Locale/en-US/preferences/ui/markings-picker.ftl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: YAML Linter
- GitHub Check: YAML map schema validator
- GitHub Check: Test Packaging
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Label
🔇 Additional comments (8)
Content.Shared/Humanoid/Markings/MarkingPrototype.cs (1)
41-43: Excellent addition of the PreviewDirection property, cadet!This new property allows for proper orientation control in marking previews, ensuring our personnel know exactly how they'll look before deployment. A proper strategic enhancement to our crew customization systems.
Content.Client/Lobby/UI/RequirementsSelector.xaml (3)
5-5: Margins properly aligned, ensign!This right-only margin of 5 units creates a more precise and orderly visual formation. The Biesel Republic's interfaces maintain proper spacing - unlike those sloppy Sol Alliance displays!
8-9: Strategic control insertion detected and approved!Adding this horizontally expanding control pushes other elements rightward - a perfect tactical maneuver for proper element alignment. This is how we maintain order in our fleet interfaces!
14-14: BoxContainer adjusted to regulation width, lieutenant!Setting a fixed width of 400 for the options container ensures consistent display across all viewports. The fleet demands precision in UI layouts!
Resources/Locale/en-US/preferences/ui/markings-picker.ftl (1)
29-29: Localization entry maintained at proper position, sailor!The "Underwear" category remains properly labeled in the interface. Our crew deserves clear markings for all customization options!
Resources/Locale/en-US/preferences/ui/humanoid-profile-editor.ftl (1)
28-31: Button labels properly clarified, commander!These more descriptive labels ("Import Character", "Export Character", "Save Changes", "Undo Changes") provide superior operational clarity compared to the previous vague designations. All crew members, regardless of rank, will now understand the exact function of each control!
The Biesel Republic demands clear communication in all interfaces - this update meets our high standards for UI design.
Content.Client/UserInterface/Controls/AlternatingBGContainer.xaml.cs (1)
64-79:FrameUpdatehack risks double‑adding controlsDuring the first frame every child not equal to
Containeris passed toAddControl, but if someone already calledAddControlmanually before the first frame you’ll end up wrapping the same control twice. Consider guarding withchild is PanelContaineror tracking which children were already processed.Content.Client/Humanoid/MarkingPicker.xaml.cs (1)
210-214: Null‑safe preview generationAccessing
marking.Sprites[0]without checkingSprites.CountrisksIndexOutOfRangeExceptionif a bad prototype sneaks through. A quick guard keeps the editor alive.- Texture = marking.Sprites[0].DirFrame0().TextureFor( + Texture = (marking.Sprites.Count > 0 + ? marking.Sprites[0].DirFrame0().TextureFor( Enum.TryParse<Direction>(marking.PreviewDirection, out var dir) ? dir : Direction.South) + : null),
| public Thickness ItemMargins = new(4); | ||
|
|
||
| /// The list of colors to alternate between | ||
| public List<Color> Colors = new() | ||
| { | ||
| Color.FromHex("#25252A"), | ||
| // Color.FromHex("#202025"), | ||
| Color.FromHex("#1B1B1E"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose collection changes safely or add documentation
public Thickness ItemMargins and public List<Color> Colors are mutable fields. A caller can mutate them at any time without updating existing children, desynchronising margins / colours. Either:
- Convert them to properties and invoke
UpdateColors()/ relayout in the setter, or - Add XML‑doc telling maintainers they must call
UpdateColors()after mutation.
| @@ -0,0 +1,110 @@ | |||
| using System.Linq; | |||
| using System.Numerics; | |||
There was a problem hiding this comment.
| using System.Numerics; |
| using Robust.Client.UserInterface.XAML; | ||
| using Robust.Shared.Timing; | ||
|
|
||
|
|
VMSolidus
left a comment
VMSolidus
left a comment
There was a problem hiding this comment.
There's also some new unused usings in HumanoidProfileEditor.xaml.cs that can be yeeyed in this PR before merging. Content.Client.Message and Direction = Robust.Shared.Maths.Direction can both be removed now that they're unused.
Description
This sucked, this sucks less now.
TODO
Media
Changelog
🆑