Replacement markup parsing changes.

Fixed a bug where self-closing replacement markup was consuming whitespace.
Changed how invisible characters work in replacement markup, now you can say how many (if any) invisible characters were added into the string during replacement.

Author: McJones

CHANGELOG.md

@@ -8,6 +8,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Added
 
+- `ReplacementMarkerResult` struct that encapsulates diagnostics of replacement markup processors and also an invisible characters count.
+  - this allows for replacement markup to declare how many invisible characters they have added
+  - this is necessary because both Unity and Godot (and likely every tool out there) has some variant of rich text strings where the attributes on the rich text is within the string itself
+  - any sibling markup following the replacement needs to not be pushed down by the invisible elements of the string
+
 ### Updated
 
 - When the `tags:` header on a node is blank, `Node.Tags` now returns an empty collection, instead of a collection containing a single empty string.
@@ -22,6 +27,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Language Server: hovering smart variables now shows their definition.
 - Language Server: Make compilation asynchronous
 - Language Server: 'Undeclared variable' warnings have had their severity reduced to "hint".
+- Fixed a bug where self-closing replacement markup was consuming whitespace
+- `IAttributeMarkerProcessor` now return a `ReplacementMarkerResult` struct instead of just a list of diagnostics
 
 ### Removed
 

YarnSpinner.Tests/MarkupTests.cs

@@ -694,7 +694,39 @@ void TestSquishedMarkupStringsWithRewritersAreValid(string line, string comparis
             attributes.Should().HaveCount(attributeCount);
         }
 
-        List<LineParser.MarkupDiagnostic> IAttributeMarkerProcessor.ProcessReplacementMarker(MarkupAttribute marker, StringBuilder childBuilder, List<MarkupAttribute> childAttributes, string localeCode)
+        [Theory]
+        [InlineData("this is a line with non-replacement[a/]  markup", "this is a line with non-replacement markup", new string[] { "a" },new int[] { 35 })]
+        [InlineData("this is a line [bold]with some replacement[/bold] markup and a non-replacement[a/]  markup", "this is a line <b>with some replacement</b> markup and a non-replacement markup", new string[] { "a" }, new int[] { 65 })]
+        [InlineData("this is a [bold]line with some [italics]nested[a trimwhitespace=false /] tags[/italics][b trimwhitespace=false /][/bold] in[c trimwhitespace=false /] it", "this is a <b>line with some <i>nested tags</i></b> in it", new string[] { "a", "b", "c" }, new int[] { 31, 36, 39 })]
+        [InlineData("this is a line with [blocky]markup[/blocky] that actually has[a trimwhitespace=false /] visible characters", "this is a line with [markup] that actually has visible characters", new string[] { "a" }, new int[] { 46 })]
+        [InlineData("this is a line with [wacky]markup[/wacky] that actually has[a trimwhitespace=false /] both", "this is a line with <b>[markup]</b> that actually has both", new string[] { "a" }, new int[] { 46 })]
+        [InlineData("this is a line with [wacky]internal[a trimwhitespace=false /] both[/wacky] markup","this is a line with <b>[internal both]</b> markup", new string[] { "a" }, new int[] { 29 })]
+        [InlineData("this is a [wacky]line with some [blocky]nested[a trimwhitespace=false /] tags[/blocky][b trimwhitespace=false /][/wacky] in[c trimwhitespace=false /] it", "this is a <b>[line with some [nested tags]]</b> in it", new string[] { "a", "b", "c" }, new int[] { 33, 39, 43 })]
+        void TestSquishedMarkupStringsWithInvisibleCharactersAreValid(string line, string comparison, string[] markerNames, int[] markerPositions)
+        {
+            markerNames.Should().HaveSameCount(markerPositions);
+
+            var lineParser = new LineParser();
+            lineParser.RegisterMarkerProcessor("bold", this);
+            lineParser.RegisterMarkerProcessor("italics", this);
+            lineParser.RegisterMarkerProcessor("blocky", this);
+            lineParser.RegisterMarkerProcessor("wacky", this);
+
+            var result = lineParser.ParseStringWithDiagnostics(line, "en");
+
+            result.diagnostics.Should().BeEmpty();
+            result.markup.Text.Should().Be(comparison);
+            result.markup.Attributes.Should().HaveCount(markerPositions.Length);
+
+            for (int i = 0; i < markerNames.Length; i++)
+            {
+                var name = markerNames[i];
+                result.markup.TryGetAttributeWithName(name, out var attribute).Should().BeTrue();
+                attribute.Position.Should().Be(markerPositions[i]);
+            }
+        }
+
+        public ReplacementMarkerResult ProcessReplacementMarker(MarkupAttribute marker, StringBuilder childBuilder, List<MarkupAttribute> childAttributes, string localeCode)
         {
             var diagnostics = new List<LineParser.MarkupDiagnostic>();
             switch (marker.Name)
@@ -706,7 +738,37 @@ void TestSquishedMarkupStringsWithRewritersAreValid(string line, string comparis
                         childBuilder.Insert(0, "<b>");
                         childBuilder.Append("</b>");
 
-                        return diagnostics;
+                        return new ReplacementMarkerResult(diagnostics, 7);
+                    }
+                case "italics":
+                    {
+                        childBuilder.Insert(0, "<i>");
+                        childBuilder.Append("</i>");
+                        return new ReplacementMarkerResult(diagnostics, 7);
+                    }
+                case "blocky":
+                    {
+                        childBuilder.Insert(0, '[');
+                        childBuilder.Append(']');
+
+                        for (int i = 0; i < childAttributes.Count; i++)
+                        {
+                            childAttributes[i] = childAttributes[i].Shift(1);
+                        }
+
+                        return new ReplacementMarkerResult(diagnostics, 0);
+                    }
+                case "wacky":
+                    {
+                        childBuilder.Insert(0, "<b>[");
+                        childBuilder.Append("]</b>");
+
+                        for (int i = 0; i < childAttributes.Count; i++)
+                        {
+                            childAttributes[i] = childAttributes[i].Shift(1);
+                        }
+
+                        return new ReplacementMarkerResult(diagnostics, 7);
                     }
                 case "localise":
                     {
@@ -719,16 +781,22 @@ void TestSquishedMarkupStringsWithRewritersAreValid(string line, string comparis
                         {
                             childBuilder.Append("chat");
                         }
-                        return diagnostics;
+                        return new ReplacementMarkerResult(diagnostics, 0);
+                    }
+                case "scr":
+                    {
+                        childBuilder.Append("scr");
+                        return new ReplacementMarkerResult(diagnostics, 0);
                     }
                 default:
                     {
                         childBuilder.Append("Unrecognised markup name: ");
                         childBuilder.Append(marker.Name);
-                        return diagnostics;
+                        return new ReplacementMarkerResult(diagnostics, 0);
                     }
             }
         }
+
         [Fact]
         void TestLocalisedStringReplacement()
         {
@@ -1294,6 +1362,20 @@ public void TestOlderSiblingNearReplacementMarkersCorrectlyRespectsWhitespaceCon
 
             markup.markup.Text.Should().Be(expected);
         }
+
+        [Theory]
+        [InlineData("a line with a self-closing[scr /] replacement tag","a line with a self-closingscr replacement tag")]
+        [InlineData("a line with a self-closing[scnr /] -non-replacement tag","a line with a self-closing-non-replacement tag")]
+        [InlineData("a line with a self-closing[scnr trimwhitespace=false /] non-replacement tag","a line with a self-closing non-replacement tag")]
+        public void TestSelfclosingReplacementMarkersDoNotConsumeWhitespace(string line, string expected)
+        {
+            var lineParser = new LineParser();
+            lineParser.RegisterMarkerProcessor("scr", this);
+
+            var markup = lineParser.ParseStringWithDiagnostics(line, "en-AU");
+            markup.diagnostics.Should().BeEmpty();
+            markup.markup.Text.Should().Be(expected);
+        }
     }
 
     public class BBCodeChevronReplacer : IAttributeMarkerProcessor
@@ -1304,18 +1386,18 @@ public BBCodeChevronReplacer(string markerName)
             tag = markerName;
         }
 
-        public List<LineParser.MarkupDiagnostic> ProcessReplacementMarker(MarkupAttribute marker, StringBuilder childBuilder, List<MarkupAttribute> childAttributes, string localeCode)
+        public ReplacementMarkerResult ProcessReplacementMarker(MarkupAttribute marker, StringBuilder childBuilder, List<MarkupAttribute> childAttributes, string localeCode)
         {
             List<LineParser.MarkupDiagnostic> diagnostics = new List<LineParser.MarkupDiagnostic>();
             if (marker.Name != tag)
             {
                 diagnostics.Add(new LineParser.MarkupDiagnostic($"Asked to replace {marker.Name} but this only handles {tag} markers."));
-                return diagnostics;
+                return new ReplacementMarkerResult(diagnostics, 0);
             }
 
             childBuilder.Insert(0, $"<{tag}>");
             childBuilder.Append($"</{tag}>");
-            return diagnostics;
+            return new ReplacementMarkerResult(diagnostics, 5 + tag.Length * 2);
         }
     }
 }

YarnSpinner/YarnSpinner.Markup/IAttributeMarkerProcessor.cs

@@ -62,7 +62,58 @@ public interface IAttributeMarkerProcessor
         /// </list>
         /// </example>
         /// <returns>The collection of diagnostics produced during processing,
-        /// if any.</returns>
-        public List<LineParser.MarkupDiagnostic> ProcessReplacementMarker(MarkupAttribute marker, System.Text.StringBuilder childBuilder, List<MarkupAttribute> childAttributes, string localeCode);
+        /// and the number of invisible characters created during processing.</returns>
+        public ReplacementMarkerResult ProcessReplacementMarker(MarkupAttribute marker, System.Text.StringBuilder childBuilder, List<MarkupAttribute> childAttributes, string localeCode);
+    }
+
+    /// <summary>
+    /// A bundle of results from processing replacement markers.
+    /// </summary>
+    /// <seealso cref="IAttributeMarkerProcessor"/>
+    public struct ReplacementMarkerResult
+    {
+        /// <summary>
+        /// The collection of diagnostics produced during processing, if any.
+        /// </summary>
+        public List<LineParser.MarkupDiagnostic> Diagnostics;
+        /// <summary>
+        /// The number of invisible characters added into the line during processing.
+        /// </summary>
+        /// <remarks>
+        /// This will vary depending on what the replacement markup needs to do.
+        /// <list type="bullet">
+        /// <item>
+        /// <para>
+        /// When only inserting rich-text tags, this should be the length of all inserted text.
+        /// For example <c>"this is text with [bold]some bold[/bold] elements"</c> translated into Unity style rich-text become <c>"this is text with &lt;b>some bold&lt;/b> elements"</c>.
+        /// In this case then the value of <c>InvisibleCharacters</c> would be seven.
+        /// </para>
+        /// </item>
+        /// <item>
+        /// <para>
+        /// When only modifying the content of the children text, such as making all text upper case, this should be <c>0</c>.
+        /// For example <c>"this is text with [upper]some uppercased[/upper] elements"</c> is transformed into <c>"this is text with SOME UPPERCASED elements"</c>.
+        /// The number of invisible character will be zero.
+        /// </para>
+        /// </item>
+        /// <item>
+        /// When adding new content into the line (regardless of being added at the start, end, or middle) this should be zero but the replacement processor should make sure to shift along it's children attributes where appropriate.
+        /// For example <c>"this is text with [emph]some emphasised[/emph] elements"</c> transformed into <c>"this is text with !!some emphasised!! elements"</c> the value of <c>InvisibleCharacters</c> would be zero.
+        /// In this case however the <c>childAttributes</c> in <see cref="IAttributeMarkerProcessor.ProcessReplacementMarker"/> would need to be shifted down two.
+        /// </item>
+        /// </list>
+        /// </remarks>
+        public int InvisibleCharacters;
+
+        /// <summary>
+        /// Convenience constructor for replacement markup results.
+        /// </summary>
+        /// <param name="diagnostics">The diagnostics generated during processing</param>
+        /// <param name="invisibleCharacters">the number of invisible characters generated during processing.</param>
+        public ReplacementMarkerResult(List<LineParser.MarkupDiagnostic> diagnostics, int invisibleCharacters)
+        {
+            this.Diagnostics = diagnostics;
+            this.InvisibleCharacters = invisibleCharacters;
+        }
     }
 }