Include computed node title in NodeInfo

Author: desplesda

YarnSpinner.Compiler/Utility.cs

@@ -11,6 +11,7 @@ namespace Yarn.Compiler
     using System.Globalization;
     using System.Linq;
     using System.Text;
+    using Yarn.Utility;
 
     /// <summary>
     /// Utility methods for working with line tags.
@@ -582,6 +583,81 @@ public static string GetCompiledCodeAsString(Program program, Library? l = null,
                 return null;
             }
         }
+
+        /// <summary>
+        /// Gets the title for a node as defined in the source code, along with
+        /// its unique title (which may be different to the source title.)
+        /// </summary>
+        /// <param name="sourceFileName">The name of the file in which the node
+        /// is defined, or <see langword="null"/> if not available.</param>
+        /// <param name="nodeContext">The parsed node's context.</param>
+        /// <param name="sourceTitle">On return, contains the title of the node, as it
+        /// appears in the source code.</param>
+        /// <param name="uniqueTitle">On return, contains the unique title of
+        /// the node, as stored in the output program.</param>
+        /// <returns><see langword="true"/> if the <paramref name="sourceTitle"/> and
+        /// <paramref name="uniqueTitle"/> could be determined; <see
+        /// langword="false"/> otherwise.</returns>
+        public static bool TryGetNodeTitle(
+            string? sourceFileName,
+            YarnSpinnerParser.NodeContext nodeContext,
+            [System.Diagnostics.CodeAnalysis.NotNullWhen(true)]
+            out string? sourceTitle,
+            [System.Diagnostics.CodeAnalysis.NotNullWhen(true)]
+            out string? uniqueTitle
+            )
+        {
+            // Try and find the current title, if present.
+            uniqueTitle = nodeContext.title_header()?.FirstOrDefault()?.title?.Text;
+
+            if (string.IsNullOrEmpty(uniqueTitle))
+            {
+                // No title was found.
+                uniqueTitle = sourceTitle = null;
+                return false;
+            }
+
+            // The node group name of a node is its title, as it appears in the
+            // source.
+            sourceTitle = uniqueTitle;
+
+            if (nodeContext.GetWhenHeaders().Any())
+            {
+                // The node is in a node group. Its real title (as stored in the
+                // Program) will be different, to uniquely identify it. This
+                // value depends on whether it has an explicit subtitle or not.
+
+                var subtitle = nodeContext.GetHeader("subtitle")?.header_value?.Text;
+
+                // Calculate a new unique title for this node and update its title header
+                if (subtitle != null && string.IsNullOrEmpty(subtitle) == false)
+                {
+                    // The node's unique title is derived from its original
+                    // title and its subtitle.
+                    uniqueTitle = $"{uniqueTitle}.{subtitle}";
+                }
+                else
+                {
+                    // The node's unique title is derived from its original
+                    // title, the name of the file it's in, and the position
+                    // it's in in the file.
+                    string checksum = CRC32.GetChecksumString(
+                        (sourceFileName ?? "")
+                        + uniqueTitle
+                        + nodeContext.Start.Line.ToString());
+
+                    uniqueTitle = $"{uniqueTitle}.{checksum}";
+                }
+            }
+
+            if (uniqueTitle == null || sourceTitle == null)
+            {
+                uniqueTitle = sourceTitle = null;
+                return false;
+            }
+
+            return true;
+        }
     }
 
     public struct GraphingNode

YarnSpinner.Compiler/Visitors/NodeGroupVisitor.cs

@@ -1,8 +1,6 @@
 using Antlr4.Runtime;
 using Antlr4.Runtime.Misc;
-using System;
 using System.Linq;
-using Yarn.Utility;
 
 namespace Yarn.Compiler
 {
@@ -17,46 +15,45 @@ public NodeGroupVisitor(string sourceFile)
 
         public override int VisitNode([NotNull] YarnSpinnerParser.NodeContext context)
         {
-            var titleHeader = context.title_header().FirstOrDefault();
+            if (!context.GetWhenHeaders().Any())
+            {
+                // The node does not contain any 'when' headers. It's not part
+                // of a node group, and doesn't need to be modified.
+                return base.VisitNode(context);
+            }
 
-            if (titleHeader == null || titleHeader.title?.Text == null)
+            // Get the node's title information.
+            if (!Utility.TryGetNodeTitle(SourceFile, context, out var sourceTitle, out var uniqueTitle))
             {
-                // The node doesn't have a title. It can't be part of a node group.
+                // The node's title or nodegroup name can't be determined.
                 return base.VisitNode(context);
             }
 
-            if (context.GetWhenHeaders().Any())
+            var titleHeader = context.title_header().FirstOrDefault();
+
+            if (titleHeader == null || !context.GetWhenHeaders().Any())
             {
-                // This node contains at least one 'when' header. 
-                var title = titleHeader.title.Text;
-
-                // Add a new header to mark which group it's from.
-                var groupHeader = new YarnSpinnerParser.HeaderContext(context, 0)
-                {
-                    header_key = new CommonToken(YarnSpinnerParser.ID, Node.NodeGroupHeader),
-                    header_value = new CommonToken(YarnSpinnerParser.REST_OF_LINE, title)
-                };
-
-                context.AddChild(groupHeader);
-
-                var subtitle = context.GetHeader("subtitle")?.header_value ?? null;
-
-                // Calculate a new unique title for this node and update its title header
-                string? newTitle;
-                if (String.IsNullOrEmpty(subtitle?.Text) == false)
-                {
-                    newTitle = $"{title}.{subtitle?.Text}";
-                }
-                else
-                {
-                    newTitle = $"{title}.{CRC32.GetChecksumString(SourceFile + title + context.Start.Line.ToString())}";
-                }
-
-
-                // Update the title header to the new 'actual' title.
-                titleHeader.title = new CommonToken(YarnSpinnerParser.REST_OF_LINE, newTitle);
+                // The node either lacks a title header, or lacks 'when'
+                // headers.
+                return base.VisitNode(context);
             }
 
+            // This node contains a title header and at least one 'when' header.
+            // It's in a node group. We need to mark it as belonging to a node
+            // group and update its title.
+
+            // Add a new header to mark which group it's from.
+            var groupHeader = new YarnSpinnerParser.HeaderContext(context, 0)
+            {
+                header_key = new CommonToken(YarnSpinnerParser.ID, Node.NodeGroupHeader),
+                header_value = new CommonToken(YarnSpinnerParser.REST_OF_LINE, sourceTitle)
+            };
+
+            context.AddChild(groupHeader);
+
+            // Update the title header to the new 'actual' title.
+            titleHeader.title = new CommonToken(YarnSpinnerParser.REST_OF_LINE, uniqueTitle);
+
             return base.VisitNode(context);
         }
     }

YarnSpinner.LanguageServer.Tests/CodeActionTests.cs

@@ -44,8 +44,8 @@ public async Task Server_FixesJumpDestinationTypo()
         var codeActionHandler = new CodeActionHandler(workspace);
         var CodeActionParams = new CodeActionParams
         {
-            Context = new CodeActionContext { Diagnostics = Container.From(jumpToWarning!)}, 
-            TextDocument = new TextDocumentIdentifier(DocumentUri.FromFileSystemPath(filePath)) 
+            Context = new CodeActionContext { Diagnostics = Container.From(jumpToWarning!) },
+            TextDocument = new TextDocumentIdentifier(DocumentUri.FromFileSystemPath(filePath))
         };
 
         var commandOrCodeActions = await codeActionHandler.Handle(CodeActionParams, default);
@@ -66,9 +66,9 @@ public async Task Server_FixesJumpDestinationTypo()
 
         nodeInfo = await nodesChangedAfterRemovingNode;
 
-        var jumpToNode = nodeInfo.Nodes.Find(n => n.Title == "JumpToTest");
+        var jumpToNode = nodeInfo.Nodes.Find(n => n.UniqueTitle == "JumpToTest");
         nodeInfo.Nodes.Should().HaveCount(nodeCount, "because didn't change any nodes");
-        jumpToNode!.Jumps.Where(j=>j.DestinationTitle == "JumpToTest").Should().HaveCount(1, "because we fixed the typo and are jumping to the existing JumpToTest node");
+        jumpToNode!.Jumps.Where(j => j.DestinationTitle == "JumpToTest").Should().HaveCount(1, "because we fixed the typo and are jumping to the existing JumpToTest node");
     }
 
     [Fact]
@@ -97,7 +97,7 @@ public async Task Server_CreatesNewNodeBasedOnJumpTarget()
         };
         var commandOrCodeActions = await codeActionHandler.Handle(CodeActionParams, default);
 
-        var generateNodeFix = commandOrCodeActions.FirstOrDefault(c => 
+        var generateNodeFix = commandOrCodeActions.FirstOrDefault(c =>
         c.CodeAction?.Title?.Contains("Generate node 'Jump2Test'") ?? false);
 
         generateNodeFix.Should().NotBeNull("Expecting a code action to generate a new node");
@@ -109,7 +109,7 @@ public async Task Server_CreatesNewNodeBasedOnJumpTarget()
         var nodeCount = nodeInfo.Nodes.Count;
 
         // Remember how many jumps we had to JumpToTest we had before making the change
-        var jumpCount = nodeInfo.Nodes.Find(n => n.Title == "JumpToTest")?.Jumps.Count ?? 0;
+        var jumpCount = nodeInfo.Nodes.Find(n => n.UniqueTitle == "JumpToTest")?.Jumps.Count ?? 0;
 
         // Expect to receive a 'nodes changed' notification
         Task<NodesChangedParams> nodesChangedAfterRemovingNode = GetNodesChangedNotificationAsync(n => n.Uri.ToString().Contains(filePath));
@@ -118,12 +118,12 @@ public async Task Server_CreatesNewNodeBasedOnJumpTarget()
 
         nodeInfo = await nodesChangedAfterRemovingNode;
 
-        var jumpToNode = nodeInfo.Nodes.Find(n => n.Title == "JumpToTest");
-        var jump2Node = nodeInfo.Nodes.Find(n => n.Title == "Jump2Test");
+        var jumpToNode = nodeInfo.Nodes.Find(n => n.UniqueTitle == "JumpToTest");
+        var jump2Node = nodeInfo.Nodes.Find(n => n.UniqueTitle == "Jump2Test");
         jump2Node.Should().NotBeNull("because we have created a new node");
         nodeInfo.Nodes.Should().HaveCount(nodeCount + 1, "because we have added a single new node");
-        jumpToNode!.Jumps.Where(j=>j.DestinationTitle == "JumpToTest").Should().HaveCount(0, "because we are jumping to the new generated node, not the exisiting JumpToTest node");
-        jumpToNode!.Jumps.Where(j=>j.DestinationTitle == "Jump2Test").Should().HaveCount(1, "because we are jumping to the new generated node, not the exisiting JumpToTest node");
+        jumpToNode!.Jumps.Where(j => j.DestinationTitle == "JumpToTest").Should().HaveCount(0, "because we are jumping to the new generated node, not the exisiting JumpToTest node");
+        jumpToNode!.Jumps.Where(j => j.DestinationTitle == "Jump2Test").Should().HaveCount(1, "because we are jumping to the new generated node, not the exisiting JumpToTest node");
     }
 
 }

YarnSpinner.LanguageServer.Tests/CommandTests.cs

@@ -39,13 +39,13 @@ public async Task Server_CanListNodesInFile()
 
         foreach (var node in result)
         {
-            node.Title.Should().NotBeNullOrEmpty("because all nodes have a title");
+            node.UniqueTitle.Should().NotBeNullOrEmpty("because all nodes have a title");
             node.Headers.Should().NotBeNullOrEmpty("because all nodes have headers");
             node.BodyStartLine.Should().NotBe(0, "because bodies never start on the first line");
             node.HeaderStartLine.Should().NotBe(node.BodyStartLine, "because bodies never start at the same line as their headers");
 
             node.Headers.Should().Contain(h => h.Key == "title", "because all nodes have a header named 'title'")
-                .Which.Value.Should().Be(node.Title, "because the 'title' header populates the Title property");
+                .Which.Value.Should().Be(node.UniqueTitle, "because the 'title' header populates the Title property");
 
             if (node == result.First())
             {
@@ -57,13 +57,13 @@ public async Task Server_CanListNodesInFile()
             }
         }
 
-        result.Should().Contain(n => n.Title == "Node2")
+        result.Should().Contain(n => n.UniqueTitle == "Node2")
               .Which
               .Headers.Should().Contain(h => h.Key == "tags", "because Node2 has a 'tags' header")
               .Which
               .Value.Should().Be("wow incredible", "because Node2's 'tags' header has this value");
 
-        result.Should().Contain(n => n.Title == "Start")
+        result.Should().Contain(n => n.UniqueTitle == "Start")
             .Which
             .Jumps.Should().NotBeNullOrEmpty("because the Start node contains jumps")
             .And
@@ -107,7 +107,7 @@ public async Task Server_OnAddNodeCommand_ReturnsTextEdit()
 
         nodeInfo.Nodes.Should().HaveCount(count + 1, "because we added a node");
         nodeInfo.Nodes.Should()
-            .Contain(n => n.Title == "Node",
+            .Contain(n => n.UniqueTitle == "Node",
                 "because the new node should be called Title")
             .Which.Headers.Should()
             .Contain(h => h.Key == "position" && h.Value == "100,100",
@@ -165,7 +165,7 @@ public async Task Server_OnUpdateHeaderCommand_ReturnsTextEditCreatingHeader()
 
         nodeInfo
             .Nodes.Should()
-            .Contain(n => n.Title == "Start")
+            .Contain(n => n.UniqueTitle == "Start")
             .Which.Headers.Should()
             .NotContain(n => n.Key == "position",
                 "because this node doesn't have this header");
@@ -193,7 +193,7 @@ public async Task Server_OnUpdateHeaderCommand_ReturnsTextEditCreatingHeader()
         nodeInfo = await nodesChangedAfterChangingText;
 
         nodeInfo.Nodes.Should()
-            .Contain(n => n.Title == "Start")
+            .Contain(n => n.UniqueTitle == "Start")
             .Which.Headers.Should()
             .Contain(n => n.Key == "position",
                 "because we added this new header")
@@ -219,7 +219,7 @@ public async Task Server_OnUpdateHeaderCommand_ReturnsTextEditModifyingHeader()
 
         nodeInfo
             .Nodes.Should()
-            .Contain(n => n.Title == "Node2")
+            .Contain(n => n.UniqueTitle == "Node2")
             .Which.Headers.Should()
             .HaveCount(2)
             .And
@@ -248,7 +248,7 @@ public async Task Server_OnUpdateHeaderCommand_ReturnsTextEditModifyingHeader()
         nodeInfo = await nodesChangedAfterChangingText;
 
         nodeInfo.Nodes.Should()
-            .Contain(n => n.Title == "Node2")
+            .Contain(n => n.UniqueTitle == "Node2")
             .Which.Headers.Should()
             .HaveCount(2, "because we added no new headers")
             .And.Contain(n => n.Key == headerName,

YarnSpinner.LanguageServer.Tests/HoverTests.cs

@@ -105,7 +105,7 @@ public async Task Server_OnJumpToDefinition_GivesExpectedRange()
             var workspace = server.Services.GetService<Workspace>()!;
             var project = workspace.GetProjectsForUri(filePath).Single();
             var node = project.Nodes.Should()
-                .ContainSingle(n => n.Title == "Node2", "the project should contain a single node called Node2")
+                .ContainSingle(n => n.UniqueTitle == "Node2", "the project should contain a single node called Node2")
                 .Subject;
 
             var definition = definitionsResult.Should().ContainSingle().Subject;

YarnSpinner.LanguageServer.Tests/LanguageServerTests.cs

@@ -85,11 +85,11 @@ public async Task Server_OnOpeningDocument_SendsNodesChangedNotification()
             nodeInfo.Should().NotBeNull("because this notification always carries a parameters object");
             nodeInfo.Nodes.Should().NotBeNullOrEmpty("because this notification always contains a list of node infos, even if it's empty");
 
-            nodeInfo.Nodes.Should().Contain(ni => ni.Title == "Start", "because this file contains a node with this title");
+            nodeInfo.Nodes.Should().Contain(ni => ni.UniqueTitle == "Start", "because this file contains a node with this title");
 
             nodeInfo.Nodes.Should()
                 .Contain(
-                    ni => ni.Title == "Node2",
+                    ni => ni.UniqueTitle == "Node2",
                     "because this file contains a node with this title")
                 .Which.Headers.Should()
                 .Contain(
@@ -125,7 +125,7 @@ public async Task Server_OnChangingDocument_SendsNodesChangedNotification()
             nodeInfo = await nodesChanged;
 
             nodeInfo.Nodes.Should().HaveCount(6, "because we added a new node");
-            nodeInfo.Nodes.Should().Contain(n => n.Title == "Node3", "because the new node we added has this title");
+            nodeInfo.Nodes.Should().Contain(n => n.UniqueTitle == "Node3", "because the new node we added has this title");
         }
 
         [Fact(Timeout = 2000)]

YarnSpinner.LanguageServer.Tests/ReferenceTests.cs

@@ -35,7 +35,7 @@ public async Task Workspace_FindsReferencesToNodes()
             (IEnumerable<Location> References, IEnumerable<NodeJump> Jumps) GetReferencesAndJumps(string name)
             {
                 IEnumerable<Location> references = ReferencesHandler.GetReferences(project, name, YarnSymbolType.Node);
-                IEnumerable<NodeJump> jumps = project.Nodes.Single(n => n.Title == name).Jumps;
+                IEnumerable<NodeJump> jumps = project.Nodes.Single(n => n.UniqueTitle == name).Jumps;
                 return (references, jumps);
             }
 

YarnSpinner.LanguageServer.Tests/WorkspaceTests.cs

@@ -51,7 +51,7 @@ public void Workspaces_CanOpen()
             // The node NotIncludedInProject is inside a file that is not
             // included in a .yarnproject; because we have opened a workspace
             // that includes .yarnprojects, the file will not be included
-            workspace.Projects.Should().AllSatisfy(p => p.Nodes.Should().NotContain(n => n.Title == "NotIncludedInProject"));
+            workspace.Projects.Should().AllSatisfy(p => p.Nodes.Should().NotContain(n => n.UniqueTitle == "NotIncludedInProject"));
 
             workspace.Projects.Should().AllSatisfy(p => p.Uri!.Should().NotBeNull());
 
@@ -76,7 +76,7 @@ public void Workspaces_WithNoProjects_HaveImplicitProject()
             // Then
             var project = workspace.Projects.Should().ContainSingle().Subject;
             var file = project.Files.Should().ContainSingle().Subject;
-            file.NodeInfos.Should().Contain(n => n.Title == "NotIncludedInProject");
+            file.NodeInfos.Should().Contain(n => n.UniqueTitle == "NotIncludedInProject");
             project.Diagnostics.Should().NotContain(d => d.Severity == Yarn.Compiler.Diagnostic.DiagnosticSeverity.Error);
 
         }

YarnSpinner.LanguageServer/src/Server/Handlers/CompletionHandler.cs

@@ -371,19 +371,19 @@ private static void GetNodeNameCompletions(Project project, CompletionParams req
         {
             foreach (var node in project.Nodes)
             {
-                if (node.Title == null || node.File == null)
+                if (node.UniqueTitle == null || node.File == null)
                 {
                     continue;
                 }
 
                 results.Add(new CompletionItem
                 {
-                    Label = node.Title,
+                    Label = node.UniqueTitle,
                     Kind = CompletionItemKind.Method,
                     Detail = System.IO.Path.GetFileName(node.File.Uri.AbsolutePath),
                     TextEdit = new TextEditOrInsertReplaceEdit(new TextEdit
                     {
-                        NewText = node.Title,
+                        NewText = node.UniqueTitle,
                         Range = new Range
                         {
                             Start = indexTokenRange.Start,