Rework variable hover to have a more compact layout (#435)

closes #422
MarkusAmshove · Nov 29, 2023 · 62ec70d · 62ec70d
2 parents eeb4ce0 + e3972e2
commit 62ec70d
Show file tree

Hide file tree

Showing 3 changed files with 183 additions and 32 deletions.
diff --git a/libs/natls/src/main/java/org/amshove/natls/hover/HoverProvider.java b/libs/natls/src/main/java/org/amshove/natls/hover/HoverProvider.java
@@ -2,8 +2,6 @@
 
 import org.amshove.natls.markupcontent.IMarkupContentBuilder;
 import org.amshove.natls.markupcontent.MarkupContentBuilderFactory;
-import org.amshove.natparse.IPosition;
-import org.amshove.natparse.NodeUtil;
 import org.amshove.natparse.lexing.SyntaxKind;
 import org.amshove.natparse.natural.*;
 import org.amshove.natparse.natural.builtin.BuiltInFunctionTable;
@@ -222,10 +220,6 @@ private Hover hoverVariable(IVariableNode variable, HoverContext context)
 		var contentBuilder = MarkupContentBuilderFactory.newBuilder();
 		var declaration = formatVariableDeclaration(context.file().module(), variable);
 		contentBuilder.appendCode(declaration.declaration);
-		if (!declaration.comment.isEmpty())
-		{
-			contentBuilder.appendSection("comment", nestedBuilder -> nestedBuilder.appendCode(declaration.comment));
-		}
 
 		if (variable.isArray())
 		{
@@ -238,15 +232,9 @@ private Hover hoverVariable(IVariableNode variable, HoverContext context)
 			});
 		}
 
-		if (variable.level() > 1)
-		{
-			var owner = NodeUtil.findLevelOneParentOf(variable);
-			contentBuilder.appendItalic("member of:");
-			contentBuilder.appendNewline();
-			contentBuilder.appendCode("%s %d %s".formatted(owner.scope().toString(), owner.level(), owner.name()));
-		}
+		var pathToVariableHover = VariableContextHover.create(context, variable);
+		pathToVariableHover.addVariableContext(contentBuilder);
 
-		addSourceFileIfNeeded(contentBuilder, variable.declaration(), context);
 		return new Hover(contentBuilder.build());
 	}
 
@@ -277,16 +265,6 @@ private VariableDeclarationFormat formatVariableDeclaration(INaturalModule modul
 		return new VariableDeclarationFormat(declaration, comment);
 	}
 
-	private static void addSourceFileIfNeeded(IMarkupContentBuilder contentBuilder, IPosition hoveredPosition, HoverContext context)
-	{
-		if (!hoveredPosition.filePath().equals(context.file().getPath()))
-		{
-			contentBuilder.appendItalic("source:");
-			contentBuilder.appendNewline();
-			contentBuilder.append("- %s.%s", context.file().getLibrary().name(), hoveredPosition.fileNameWithoutExtension());
-		}
-	}
-
 	private void addModuleParameter(IMarkupContentBuilder contentBuilder, INaturalModule module)
 	{
 		if (!(module instanceof IHasDefineData hasDefineData) || hasDefineData.defineData() == null)

diff --git a/libs/natls/src/main/java/org/amshove/natls/hover/VariableContextHover.java b/libs/natls/src/main/java/org/amshove/natls/hover/VariableContextHover.java
@@ -0,0 +1,107 @@
+package org.amshove.natls.hover;
+
+import org.amshove.natls.markupcontent.IMarkupContentBuilder;
+import org.amshove.natparse.NodeUtil;
+import org.amshove.natparse.natural.IHasDefineData;
+import org.amshove.natparse.natural.INaturalModule;
+import org.amshove.natparse.natural.IUsingNode;
+import org.amshove.natparse.natural.IVariableNode;
+
+class VariableContextHover
+{
+	private final IVariableNode variable;
+	private INaturalModule declaringModule;
+	private IUsingNode usingNode;
+	private String usingComment;
+
+	private VariableContextHover(IVariableNode variable)
+	{
+		this.variable = variable;
+	}
+
+	public void addVariableContext(IMarkupContentBuilder builder)
+	{
+		var variableContext = new StringBuilder();
+
+		appendUsingComment(variableContext);
+		if (variable.level() > 1)
+		{
+			var group = NodeUtil.findLevelOneParentOf(variable);
+			appendVariableComment(group, variableContext, true);
+		}
+
+		appendVariableComment(variable, variableContext, !variableContext.isEmpty());
+
+		if (!variableContext.isEmpty())
+		{
+			builder.appendSection("context", nestedBuilder -> nestedBuilder.appendCode(variableContext.toString()));
+		}
+	}
+
+	private void appendUsingComment(StringBuilder chain)
+	{
+		if (usingNode == null)
+		{
+			return;
+		}
+
+		var usingSource = "%s USING %s".formatted(usingNode.scope().toSyntaxKind().name(), usingNode.target().symbolName());
+		appendComment("%s %s".formatted(usingSource, usingComment), chain);
+	}
+
+	private void appendVariableComment(IVariableNode variable, StringBuilder chain, boolean alwaysInclude)
+	{
+		var source = "%d %s".formatted(variable.level(), variable.name());
+		var comment = declaringModule.extractLineComment(variable.position().line());
+		if (alwaysInclude || !comment.isEmpty())
+		{
+			appendComment("%s %s".formatted(source, comment), chain);
+		}
+	}
+
+	private void appendComment(String comment, StringBuilder chain)
+	{
+		if (comment != null && !comment.isEmpty())
+		{
+			if (!chain.isEmpty())
+			{
+				chain.append(System.lineSeparator());
+			}
+			chain.append(comment);
+		}
+	}
+
+	static VariableContextHover create(HoverContext hoverContext, IVariableNode variable)
+	{
+		var variableContext = new VariableContextHover(variable);
+		if (variable.position().filePath().equals(hoverContext.file().getPath()))
+		{
+			variableContext.declaringModule = hoverContext.file().module();
+		}
+		else
+		{
+			var using = findUsingImportingVariable(variable, ((IHasDefineData) hoverContext.file().module()));
+			if (using != null)
+			{
+				variableContext.usingNode = using;
+				variableContext.usingComment = hoverContext.file().module().extractLineComment(using.position().line());
+				variableContext.declaringModule = hoverContext.file().findNaturalModule(using.target().symbolName());
+			}
+		}
+
+		return variableContext;
+	}
+
+	private static IUsingNode findUsingImportingVariable(IVariableNode variable, IHasDefineData module)
+	{
+		for (var using : module.defineData().usings())
+		{
+			if (using.defineData() != null && using.defineData().findVariable(variable.qualifiedName()) != null)
+			{
+				return using;
+			}
+		}
+
+		return null;
+	}
+}
diff --git a/libs/natls/src/test/java/org/amshove/natls/hovering/VariableHoverTests.java b/libs/natls/src/test/java/org/amshove/natls/hovering/VariableHoverTests.java
@@ -98,15 +98,15 @@ void inlineCommentsShouldBeIncluded()
 				LOCAL 1 #MYVAR (A10)
 				```
 
-				*comment:*
+				*context:*
 				```natural
-				/* Inline comment
+				1 #MYVAR /* Inline comment
 				```"""
 		);
 	}
 
 	@Test
-	void theSourceFileOfTheVariableShouldBeAddedIfItDiffersFromTheHoveringFile()
+	void theUsingDataAreaShouldBeIncludedInTheContext()
 	{
 		createOrSaveFile("LIBONE", "MYLDA.NSL", """
 			DEFINE DATA
@@ -127,9 +127,75 @@ void theSourceFileOfTheVariableShouldBeAddedIfItDiffersFromTheHoveringFile()
 LOCAL 1 #MYVAR (A10)
 ```
 
-*source:*
+*context:*
+```natural
+LOCAL USING MYLDA
+1 #MYVAR
+```"""
+		);
+	}
+
+	@Test
+	void commentsOnTheUsingOfDataAreasShouldBeIncludedForGroupMembers()
+	{
+		createOrSaveFile("LIBONE", "MYLDA.NSL", """
+			DEFINE DATA LOCAL
+			1 #GRP
+			2 #MYVAR (A10)
+			END-DEFINE
+			""");
+
+		assertHover(
+			"""
+			DEFINE DATA
+			LOCAL USING MYLDA /* My using
+			END-DEFINE
+			
+			WRITE #MY${}$VAR
+			END""",
+			"""
+```natural
+LOCAL 2 #MYVAR (A10)
+```
+
+*context:*
+```natural
+LOCAL USING MYLDA /* My using
+1 #GRP
+2 #MYVAR
+```"""
+		);
+	}
+
+	@Test
+	void allRelevantCommentsEncounteredOnTheWayToTheVariableShouldBeIncluded()
+	{
+		createOrSaveFile("LIBONE", "MYLDA.NSL", """
+			DEFINE DATA LOCAL
+			1 #GRP /* Important group
+			2 #MYVAR (A10) /* The Variable
+			END-DEFINE
+			""");
 
-- LIBONE.MYLDA"""
+		assertHover(
+			"""
+			DEFINE DATA
+			LOCAL USING MYLDA /* My using
+			END-DEFINE
+			
+			WRITE #MY${}$VAR
+			END""",
+			"""
+```natural
+LOCAL 2 #MYVAR (A10)
+```
+
+*context:*
+```natural
+LOCAL USING MYLDA /* My using
+1 #GRP /* Important group
+2 #MYVAR /* The Variable
+```"""
 		);
 	}
 
@@ -150,10 +216,10 @@ void theLevelOneVariableShouldBeAddedIfHoveringANestedVariable()
 LOCAL 2 #VARINGROUP (N4)
 ```
 
-*member of:*
-
+*context:*
 ```natural
-LOCAL 1 #MYGROUP
+1 #MYGROUP
+2 #VARINGROUP
 ```"""
 		);
 	}