From a393efc87a63142b7f3974f54615da49d992b1df Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Mon, 12 May 2025 14:09:18 +1000 Subject: [PATCH] Modify markup tag interfaces and fix some bugs (#5442) * Modify markup tag interfaces * Why are nullable structs like this. * AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA * Avoid breaking changes * Replace IMarkupTag with IMarkupTagHandler in engine * Its a breaking change now I guess * cleanup --- RELEASE-NOTES.md | 4 +- .../UserInterface/Controls/OutputPanel.cs | 10 ++++ .../UserInterface/Controls/RichTextLabel.cs | 56 +++++++++++-------- .../UserInterface/RichText/BoldItalicTag.cs | 2 +- .../UserInterface/RichText/BoldTag.cs | 2 +- .../UserInterface/RichText/BulletTag.cs | 2 +- .../UserInterface/RichText/ColorTag.cs | 2 +- .../UserInterface/RichText/CommandLinkTag.cs | 4 +- .../UserInterface/RichText/FontTag.cs | 2 +- .../UserInterface/RichText/HeadingTag.cs | 2 +- .../UserInterface/RichText/IMarkupTag.cs | 32 +++++++++-- .../UserInterface/RichText/ItalicTag.cs | 2 +- .../RichText/MarkupTagManager.cs | 38 +++++++++++-- Robust.Client/UserInterface/RichTextEntry.cs | 48 ++++++++++++---- 14 files changed, 149 insertions(+), 57 deletions(-) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index f75527f472..241565302a 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -35,7 +35,7 @@ END TEMPLATE--> ### Breaking changes -*None yet* +* `IMarkupTag` and related methods in `MarkupTagManager` have been obsoleted and should be replaced with the new `IMarkupTagHandler` interface. Various engine tags (e.g., `BoldTag`, `ColorTag`, etc) no longer implement the old interface. ### New features @@ -43,7 +43,7 @@ END TEMPLATE--> ### Bugfixes -*None yet* +* OutputPanel and RichTextLabel now remove controls associated with rich text tags when the text is updated. ### Other diff --git a/Robust.Client/UserInterface/Controls/OutputPanel.cs b/Robust.Client/UserInterface/Controls/OutputPanel.cs index a9cf67f6b6..881c02b353 100644 --- a/Robust.Client/UserInterface/Controls/OutputPanel.cs +++ b/Robust.Client/UserInterface/Controls/OutputPanel.cs @@ -95,6 +95,12 @@ namespace Robust.Client.UserInterface.Controls public void Clear() { _firstLine = true; + + foreach (var entry in _entries) + { + entry.RemoveControls(); + } + _entries.Clear(); _totalContentHeight = 0; _scrollBar.MaxValue = Math.Max(_scrollBar.Page, _totalContentHeight); @@ -104,6 +110,7 @@ namespace Robust.Client.UserInterface.Controls public void RemoveEntry(Index index) { var entry = _entries[index]; + entry.RemoveControls(); _entries.RemoveAt(index.GetOffset(_entries.Count)); var font = _getFont(); @@ -189,6 +196,9 @@ namespace Robust.Client.UserInterface.Controls if (entryOffset > contentBox.Height) { entry.HideControls(); + + // We know that every subsequent entry will also fail the test, but we also need to + // hide all the controls, so we cannot simply break out of the loop continue; } diff --git a/Robust.Client/UserInterface/Controls/RichTextLabel.cs b/Robust.Client/UserInterface/Controls/RichTextLabel.cs index 7e7c03c9d4..7559484642 100644 --- a/Robust.Client/UserInterface/Controls/RichTextLabel.cs +++ b/Robust.Client/UserInterface/Controls/RichTextLabel.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Linq; using System.Numerics; using JetBrains.Annotations; using Robust.Client.Graphics; @@ -15,8 +17,7 @@ namespace Robust.Client.UserInterface.Controls { [Dependency] private readonly MarkupTagManager _tagManager = default!; - private FormattedMessage? _message; - private RichTextEntry _entry; + private RichTextEntry? _entry; private float _lineHeightScale = 1; private bool _lineHeightOverride; @@ -40,19 +41,26 @@ namespace Robust.Client.UserInterface.Controls public string? Text { - get => _message?.ToMarkup(); + get => _entry?.Message.ToMarkup(); set { if (value == null) - { - _message?.Clear(); - return; - } - - SetMessage(FormattedMessage.FromMarkupPermissive(value)); + Clear(); + else + SetMessage(FormattedMessage.FromMarkupPermissive(value)); } } + public void Clear() + { + _entry?.RemoveControls(); + _entry = null; + InvalidateMeasure(); + } + + public IEnumerable Controls => _entry?.Controls?.Values ?? Enumerable.Empty(); + public IReadOnlyList Nodes => _entry?.Message.Nodes ?? Array.Empty(); + public RichTextLabel() { IoCManager.InjectDependencies(this); @@ -61,8 +69,8 @@ namespace Robust.Client.UserInterface.Controls public void SetMessage(FormattedMessage message, Type[]? tagsAllowed = null, Color? defaultColor = null) { - _message = message; - _entry = new RichTextEntry(_message, this, _tagManager, tagsAllowed, defaultColor); + _entry?.RemoveControls(); + _entry = new RichTextEntry(message, this, _tagManager, tagsAllowed, defaultColor); InvalidateMeasure(); } @@ -73,31 +81,31 @@ namespace Robust.Client.UserInterface.Controls SetMessage(msg, tagsAllowed, defaultColor); } - public string? GetMessage() => _message?.ToMarkup(); + public string? GetMessage() => _entry?.Message.ToMarkup(); + + /// + /// Returns a copy of the currently used formatted message. + /// + public FormattedMessage? GetFormattedMessage() => _entry == null ? null : new FormattedMessage(_entry.Value.Message); protected override Vector2 MeasureOverride(Vector2 availableSize) { - if (_message == null) - { + if (_entry == null) return Vector2.Zero; - } var font = _getFont(); - _entry.Update(_tagManager, font, availableSize.X * UIScale, UIScale, LineHeightScale); - return new Vector2(_entry.Width / UIScale, _entry.Height / UIScale); + // _entry is nullable struct. + // cannot just call _entry.Value.Update() as that doesn't actually update _entry. + _entry = _entry.Value.Update(_tagManager, font, availableSize.X * UIScale, UIScale, LineHeightScale); + + return new Vector2(_entry.Value.Width / UIScale, _entry.Value.Height / UIScale); } protected internal override void Draw(DrawingHandleScreen handle) { base.Draw(handle); - - if (_message == null) - { - return; - } - - _entry.Draw(_tagManager, handle, _getFont(), SizeBox, 0, new MarkupDrawingContext(), UIScale, LineHeightScale); + _entry?.Draw(_tagManager, handle, _getFont(), SizeBox, 0, new MarkupDrawingContext(), UIScale, LineHeightScale); } [Pure] diff --git a/Robust.Client/UserInterface/RichText/BoldItalicTag.cs b/Robust.Client/UserInterface/RichText/BoldItalicTag.cs index eda24e5c19..aada7c8f99 100644 --- a/Robust.Client/UserInterface/RichText/BoldItalicTag.cs +++ b/Robust.Client/UserInterface/RichText/BoldItalicTag.cs @@ -5,7 +5,7 @@ using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public sealed class BoldItalicTag : IMarkupTag +public sealed class BoldItalicTag : IMarkupTagHandler { public const string BoldItalicFont = "DefaultBoldItalic"; diff --git a/Robust.Client/UserInterface/RichText/BoldTag.cs b/Robust.Client/UserInterface/RichText/BoldTag.cs index c1e832e563..261e9e88da 100644 --- a/Robust.Client/UserInterface/RichText/BoldTag.cs +++ b/Robust.Client/UserInterface/RichText/BoldTag.cs @@ -6,7 +6,7 @@ using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public sealed class BoldTag : IMarkupTag +public sealed class BoldTag : IMarkupTagHandler { public const string BoldFont = "DefaultBold"; diff --git a/Robust.Client/UserInterface/RichText/BulletTag.cs b/Robust.Client/UserInterface/RichText/BulletTag.cs index bafb898e29..3a90df2119 100644 --- a/Robust.Client/UserInterface/RichText/BulletTag.cs +++ b/Robust.Client/UserInterface/RichText/BulletTag.cs @@ -2,7 +2,7 @@ using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public sealed class BulletTag : IMarkupTag +public sealed class BulletTag : IMarkupTagHandler { public string Name => "bullet"; diff --git a/Robust.Client/UserInterface/RichText/ColorTag.cs b/Robust.Client/UserInterface/RichText/ColorTag.cs index dadc32895d..86facb02ca 100644 --- a/Robust.Client/UserInterface/RichText/ColorTag.cs +++ b/Robust.Client/UserInterface/RichText/ColorTag.cs @@ -6,7 +6,7 @@ namespace Robust.Client.UserInterface.RichText; /// /// Colors the text inside its opening and closing nodes /// -public sealed class ColorTag : IMarkupTag +public sealed class ColorTag : IMarkupTagHandler { public static readonly Color DefaultColor = new(200, 200, 200); diff --git a/Robust.Client/UserInterface/RichText/CommandLinkTag.cs b/Robust.Client/UserInterface/RichText/CommandLinkTag.cs index 19cd73a05c..0a7833209a 100644 --- a/Robust.Client/UserInterface/RichText/CommandLinkTag.cs +++ b/Robust.Client/UserInterface/RichText/CommandLinkTag.cs @@ -8,14 +8,14 @@ using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public sealed class CommandLinkTag : IMarkupTag +public sealed class CommandLinkTag : IMarkupTagHandler { [Dependency] private readonly IClientConsoleHost _clientConsoleHost = default!; public string Name => "cmdlink"; /// - public bool TryGetControl(MarkupNode node, [NotNullWhen(true)] out Control? control) + public bool TryCreateControl(MarkupNode node, [NotNullWhen(true)] out Control? control) { if (!node.Value.TryGetString(out var text) || !node.Attributes.TryGetValue("command", out var commandParameter) diff --git a/Robust.Client/UserInterface/RichText/FontTag.cs b/Robust.Client/UserInterface/RichText/FontTag.cs index f9d88fb2f7..157664c7fb 100644 --- a/Robust.Client/UserInterface/RichText/FontTag.cs +++ b/Robust.Client/UserInterface/RichText/FontTag.cs @@ -11,7 +11,7 @@ namespace Robust.Client.UserInterface.RichText; /// Applies the font provided as the tags parameter to the markup drawing context. /// Definitely not save for user supplied markup /// -public sealed class FontTag : IMarkupTag +public sealed class FontTag : IMarkupTagHandler { public const string DefaultFont = "Default"; public const int DefaultSize = 12; diff --git a/Robust.Client/UserInterface/RichText/HeadingTag.cs b/Robust.Client/UserInterface/RichText/HeadingTag.cs index 4d8c4e784e..eb742d13b4 100644 --- a/Robust.Client/UserInterface/RichText/HeadingTag.cs +++ b/Robust.Client/UserInterface/RichText/HeadingTag.cs @@ -6,7 +6,7 @@ using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public sealed class HeadingTag : IMarkupTag +public sealed class HeadingTag : IMarkupTagHandler { [Dependency] private readonly IResourceCache _resourceCache = default!; [Dependency] private readonly IPrototypeManager _prototypeManager = default!; diff --git a/Robust.Client/UserInterface/RichText/IMarkupTag.cs b/Robust.Client/UserInterface/RichText/IMarkupTag.cs index f5a0bd27bc..4df823ece6 100644 --- a/Robust.Client/UserInterface/RichText/IMarkupTag.cs +++ b/Robust.Client/UserInterface/RichText/IMarkupTag.cs @@ -1,9 +1,16 @@ +using System; using System.Diagnostics.CodeAnalysis; using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public interface IMarkupTag +/// +/// Classes that implement this interface will be instantiated by and used to handle +/// the parsing and behaviour of markup tags. Note that each class is only ever instantiated once by the tag manager, +/// and wil be used to handle all tags of that kind, and thus should not contain state information relevant to a +/// specific tag. +/// +public interface IMarkupTagHandler { /// /// The string used as the tags name when writing rich text @@ -54,17 +61,32 @@ public interface IMarkupTag } /// - /// Called inside the constructor of to - /// supply a control that gets rendered inline before this tags children
- /// Text continues to the right of the control until the next line and then continues bellow it + /// Called inside the constructor of to supply a control that gets rendered inline + /// before this tags children. The returned control must be new instance to avoid issues with shallow cloning + /// nodes. Text continues to the right of the control until the next line and + /// then continues bellow it. /// /// The markup node containing the parameter and attributes /// A UI control for placing in line with this tags children /// true if this tag supplies a control + public bool TryCreateControl(MarkupNode node, [NotNullWhen(true)] out Control? control) + { + control = null; + return false; + } +} + +[Obsolete("Use IMarkupTagHandler")] +public interface IMarkupTag : IMarkupTagHandler +{ + bool IMarkupTagHandler.TryCreateControl(MarkupNode node, [NotNullWhen(true)] out Control? control) + { + return TryGetControl(node, out control); + } + public bool TryGetControl(MarkupNode node, [NotNullWhen(true)] out Control? control) { control = null; return false; } - } diff --git a/Robust.Client/UserInterface/RichText/ItalicTag.cs b/Robust.Client/UserInterface/RichText/ItalicTag.cs index b70cc11fb0..15285fb50f 100644 --- a/Robust.Client/UserInterface/RichText/ItalicTag.cs +++ b/Robust.Client/UserInterface/RichText/ItalicTag.cs @@ -6,7 +6,7 @@ using Robust.Shared.Utility; namespace Robust.Client.UserInterface.RichText; -public sealed class ItalicTag : IMarkupTag +public sealed class ItalicTag : IMarkupTagHandler { public const string ItalicFont = "DefaultItalic"; diff --git a/Robust.Client/UserInterface/RichText/MarkupTagManager.cs b/Robust.Client/UserInterface/RichText/MarkupTagManager.cs index ea91322501..107fd4046f 100644 --- a/Robust.Client/UserInterface/RichText/MarkupTagManager.cs +++ b/Robust.Client/UserInterface/RichText/MarkupTagManager.cs @@ -16,7 +16,7 @@ public sealed class MarkupTagManager /// /// Tags defined in engine need to be instantiated here because of sandboxing /// - private readonly Dictionary _markupTagTypes = new IMarkupTag[] { + private readonly Dictionary _markupTagTypes = new IMarkupTagHandler[] { new BoldItalicTag(), new BoldTag(), new BulletTag(), @@ -44,13 +44,13 @@ public sealed class MarkupTagManager public void Initialize() { - foreach (var type in _reflectionManager.GetAllChildren()) + foreach (var type in _reflectionManager.GetAllChildren()) { //Prevent tags defined inside engine from being instantiated if (_engineTypes.Contains(type)) continue; - var instance = (IMarkupTag)_sandboxHelper.CreateInstance(type); + var instance = (IMarkupTagHandler)_sandboxHelper.CreateInstance(type); _markupTagTypes[instance.Name.ToLower()] = instance; } @@ -60,22 +60,48 @@ public sealed class MarkupTagManager } } + [Obsolete("Use GetMarkupTagHandler")] public IMarkupTag? GetMarkupTag(string name) + { + return _markupTagTypes.GetValueOrDefault(name) as IMarkupTag; + } + + public IMarkupTagHandler? GetMarkupTagHandler(string name) { return _markupTagTypes.GetValueOrDefault(name); } - public bool TryGetMarkupTag(string name, Type[]? tagsAllowed, [NotNullWhen(true)] out IMarkupTag? tag) + /// + /// Attempt to get the tag handler with the corresponding name. + /// + /// The name of the tag, as specified by + /// List of allowed tag types. If null, all types are allowed. + /// The instance responsible for handling tags of this type. + /// + public bool TryGetMarkupTagHandler(string name, Type[]? tagsAllowed, [NotNullWhen(true)] out IMarkupTagHandler? handler) { if (_markupTagTypes.TryGetValue(name, out var markupTag) // Using a whitelist prevents new tags from sneaking in. && (tagsAllowed == null || Array.IndexOf(tagsAllowed, markupTag.GetType()) != -1)) { - tag = markupTag; + handler = markupTag; return true; } - tag = null; + handler = null; return false; } + + [Obsolete("Use TryGetMarkupTagHandler")] + public bool TryGetMarkupTag(string name, Type[]? tagsAllowed, [NotNullWhen(true)] out IMarkupTag? tag) + { + if (!TryGetMarkupTagHandler(name, tagsAllowed, out var handler) || handler is not IMarkupTag cast) + { + tag = null; + return false; + } + + tag = cast; + return true; + } } diff --git a/Robust.Client/UserInterface/RichTextEntry.cs b/Robust.Client/UserInterface/RichTextEntry.cs index 817802cca0..75f398e93a 100644 --- a/Robust.Client/UserInterface/RichTextEntry.cs +++ b/Robust.Client/UserInterface/RichTextEntry.cs @@ -13,6 +13,9 @@ namespace Robust.Client.UserInterface { /// /// Used by and to handle rich text layout. + /// Note that if this text is ever removed or modified without removing the owning control, + /// then should be called to ensure that any controls that were added by this + /// entry are also removed. /// internal struct RichTextEntry { @@ -36,7 +39,7 @@ namespace Robust.Client.UserInterface /// public ValueList LineBreaks; - private readonly Dictionary? _tagControls; + public readonly Dictionary? Controls; public RichTextEntry(FormattedMessage message, Control parent, MarkupTagManager tagManager, Type[]? tagsAllowed = null, Color? defaultColor = null) { @@ -56,15 +59,35 @@ namespace Robust.Client.UserInterface if (node.Name == null) continue; - if (!tagManager.TryGetMarkupTag(node.Name, _tagsAllowed, out var tag) || !tag.TryGetControl(node, out var control)) + if (!tagManager.TryGetMarkupTagHandler(node.Name, _tagsAllowed, out var handler) || !handler.TryCreateControl(node, out var control)) continue; + // Markup tag handler instances are shared across controls. We need to ensure that the hanlder doesn't + // store state information and return the same control for each rich text entry. + DebugTools.Assert(handler.TryCreateControl(node, out var other) && other != control); + parent.Children.Add(control); tagControls ??= new Dictionary(); tagControls.Add(nodeIndex, control); } - _tagControls = tagControls; + Controls = tagControls; + } + + // TODO RICH TEXT + // Somehow ensure that this **has** to be called when removing rich text from some control. + /// + /// Remove all owned controls from their parents. + /// + public readonly void RemoveControls() + { + if (Controls == null) + return; + + foreach (var ctrl in Controls.Values) + { + ctrl.Orphan(); + } } /// @@ -74,7 +97,7 @@ namespace Robust.Client.UserInterface /// The maximum horizontal size of the container of this entry. /// /// - public void Update(MarkupTagManager tagManager, Font defaultFont, float maxSizeX, float uiScale, float lineHeightScale = 1) + public RichTextEntry Update(MarkupTagManager tagManager, Font defaultFont, float maxSizeX, float uiScale, float lineHeightScale = 1) { // This method is gonna suck due to complexity. // Bear with me here. @@ -112,10 +135,10 @@ namespace Robust.Client.UserInterface continue; if (ProcessMetric(ref this, metrics, out breakLine)) - return; + return this; } - if (_tagControls == null || !_tagControls.TryGetValue(nodeIndex, out var control)) + if (Controls == null || !Controls.TryGetValue(nodeIndex, out var control)) continue; control.Measure(new Vector2(Width, Height)); @@ -128,12 +151,14 @@ namespace Robust.Client.UserInterface desiredSize.Y); if (ProcessMetric(ref this, controlMetrics, out breakLine)) - return; + return this; } Width = wordWrap.FinalizeText(out breakLine); CheckLineBreak(ref this, breakLine); + return this; + bool ProcessRune(ref RichTextEntry src, Rune rune, out int? outBreakLine) { wordWrap.NextRune(rune, out breakLine, out var breakNewLine, out var skip); @@ -166,9 +191,10 @@ namespace Robust.Client.UserInterface internal readonly void HideControls() { - if (_tagControls == null) + if (Controls == null) return; - foreach (var control in _tagControls.Values) + + foreach (var control in Controls.Values) { control.Visible = false; } @@ -220,7 +246,7 @@ namespace Robust.Client.UserInterface globalBreakCounter += 1; } - if (_tagControls == null || !_tagControls.TryGetValue(nodeIndex, out var control)) + if (Controls == null || !Controls.TryGetValue(nodeIndex, out var control)) continue; // Controls may have been previously hidden via HideControls due to being "out-of frame". @@ -243,7 +269,7 @@ namespace Robust.Client.UserInterface return node.Value.StringValue ?? ""; //Skip the node if there is no markup tag for it. - if (!tagManager.TryGetMarkupTag(node.Name, _tagsAllowed, out var tag)) + if (!tagManager.TryGetMarkupTagHandler(node.Name, _tagsAllowed, out var tag)) return ""; if (!node.Closing)