Merge pull request #1900 from 333fred/quickinfo-linebreaks

Ensure that all quickinfo sections have linebreaks between them, and don't add unecessary duplicate linebreaks.

src/OmniSharp.Roslyn.CSharp/Helpers/MarkdownHelpers.cs

@@ -33,11 +33,18 @@ public static string Escape(string markdown)
         /// </summary>
         private const string ContainerEnd = nameof(ContainerEnd);
 
+        public static bool StartsWithNewline(this ImmutableArray<TaggedText> taggedParts)
+        {
+            return !taggedParts.IsDefaultOrEmpty
+                   && taggedParts[0].Tag switch { TextTags.LineBreak => true, ContainerStart => true, _ => false };
+        }
+
         public static void TaggedTextToMarkdown(
             ImmutableArray<TaggedText> taggedParts,
             StringBuilder stringBuilder,
             FormattingOptions formattingOptions,
-            MarkdownFormat markdownFormat)
+            MarkdownFormat markdownFormat,
+            out bool endedWithLineBreak)
         {
             bool isInCodeBlock = false;
             bool brokeLine = true;
@@ -136,6 +143,7 @@ public static void TaggedTextToMarkdown(
                         Debug.Assert(i == taggedParts.Length);
                         stringBuilder.Append(formattingOptions.NewLine);
                         stringBuilder.Append("```");
+                        endedWithLineBreak = false;
                         return;
                     }
                 }
@@ -204,6 +212,7 @@ public static void TaggedTextToMarkdown(
                 stringBuilder.Append("_");
             }
 
+            endedWithLineBreak = brokeLine;
             return;
 
             void addText(string text)

src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs

@@ -429,7 +429,7 @@ public async Task<CompletionResolveResponse> Handle(CompletionResolveRequest req
             var description = await completionService.GetDescriptionAsync(document, lastCompletionItem);
 
             StringBuilder textBuilder = new StringBuilder();
-            MarkdownHelpers.TaggedTextToMarkdown(description.TaggedParts, textBuilder, _formattingOptions, MarkdownFormat.FirstLineAsCSharp);
+            MarkdownHelpers.TaggedTextToMarkdown(description.TaggedParts, textBuilder, _formattingOptions, MarkdownFormat.FirstLineAsCSharp, out _);
 
             request.Item.Documentation = textBuilder.ToString();
 

src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs

@@ -4,7 +4,6 @@
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.QuickInfo;
-using Microsoft.CodeAnalysis.Text;
 using Microsoft.Extensions.Logging;
 using OmniSharp.Extensions;
 using OmniSharp.Mef;
@@ -71,19 +70,11 @@ public async Task<QuickInfoResponse> Handle(QuickInfoRequest request)
 
             var finalTextBuilder = new StringBuilder();
 
+            bool lastSectionHadLineBreak = true;
             var description = quickInfo.Sections.FirstOrDefault(s => s.Kind == QuickInfoSectionKinds.Description);
             if (description is object)
             {
                 appendSection(description, MarkdownFormat.AllTextAsCSharp);
-
-                // The description doesn't include a set of newlines at the end, regardless
-                // of whether there are more sections, so if there are more sections we need
-                // to ensure they're separated.
-                if (quickInfo.Sections.Length > 1)
-                {
-                    finalTextBuilder.Append(_formattingOptions.NewLine);
-                    finalTextBuilder.Append(_formattingOptions.NewLine);
-                }
             }
 
             var summary = quickInfo.Sections.FirstOrDefault(s => s.Kind == QuickInfoSectionKinds.DocumentationComments);
@@ -127,7 +118,12 @@ public async Task<QuickInfoResponse> Handle(QuickInfoRequest request)
 
             void appendSection(QuickInfoSection section, MarkdownFormat format)
             {
-                MarkdownHelpers.TaggedTextToMarkdown(section.TaggedParts, finalTextBuilder, _formattingOptions, format);
+                if (!lastSectionHadLineBreak && !section.TaggedParts.StartsWithNewline())
+                {
+                    finalTextBuilder.Append(_formattingOptions.NewLine);
+                    finalTextBuilder.Append(_formattingOptions.NewLine);
+                }
+                MarkdownHelpers.TaggedTextToMarkdown(section.TaggedParts, finalTextBuilder, _formattingOptions, format, out lastSectionHadLineBreak);
             }
         }
     }

tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs

@@ -230,7 +230,7 @@ public async Task DisplayFormatFor_TypeSymbol_ComplexType_DifferentNamespace()
         public async Task DisplayFormatFor_TypeSymbol_WithGenerics()
         {
             var response = await GetTypeLookUpResponse(line: 15, column: 36);
-            Assert.Equal("```csharp\ninterface System.Collections.Generic.IDictionary<TKey, TValue>\n```\n\n\n\n```csharp\nTKey is string\nTValue is IEnumerable<int>\n```", response.Markdown);
+            Assert.Equal("```csharp\ninterface System.Collections.Generic.IDictionary<TKey, TValue>\n```\n\n```csharp\nTKey is string\nTValue is IEnumerable<int>\n```", response.Markdown);
         }
 
         [Fact]
@@ -338,7 +338,7 @@ class testissue
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\n\n\nReturns:\n\n  Returns true if object is tagged with tag\\.", response.Markdown);
+            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nReturns:\n\n  Returns true if object is tagged with tag\\.", response.Markdown);
         }
 
         [Fact]
@@ -369,7 +369,7 @@ class testissue
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\n\n\nExceptions:\n\n  A\n\n  B", response.Markdown);
+            Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nExceptions:\n\n  A\n\n  B", response.Markdown);
         }
 
         [Fact]
@@ -665,7 +665,7 @@ void M2()
     }
 }";
             var response = await GetTypeLookUpResponse(content);
-            Assert.Equal("```csharp\nvoid C.M1<'a>('a t)\n```\n\n\n\nAnonymous Types:\n\n```csharp\n    'a is new { int X, int Y }\n```", response.Markdown);
+            Assert.Equal("```csharp\nvoid C.M1<'a>('a t)\n```\n\nAnonymous Types:\n\n```csharp\n    'a is new { int X, int Y }\n```", response.Markdown);
         }
 
         [Fact]
@@ -726,6 +726,26 @@ public static void A(string s)
             Assert.Equal("```csharp\n(parameter) string s\n```\n\n_'s' is not null here\\._", response.Markdown);
         }
 
+        [Fact]
+        public async Task NullableFieldWithComments()
+        {
+            string content = @"
+#nullable enable
+class Program
+{
+    /// <summary>Interesting content.</summary>
+    public string? _s;
+
+    public static void A()
+    {
+        _ = _s$$;
+    }
+
+}";
+            var response = await GetTypeLookUpResponse(content);
+            Assert.Equal("```csharp\n(field) string? Program._s\n```\n\nInteresting content\\.\n\n_'\\_s' is not null here\\._", response.Markdown);
+        }
+
         private async Task<QuickInfoResponse> GetTypeLookUpResponse(string content)
         {
             TestFile testFile = new TestFile("dummy.cs", content);