Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hover when it should separate multiple summaries #8005

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Fix hover when it should separate multiple summaries #8005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b0cc0c
Fix hover when it should separate multiple summaries
maryamariyan Dec 8, 2022
b419c9e
Single line separation
maryamariyan Dec 8, 2022
cc8af8e
Update tests due to fix
maryamariyan Feb 23, 2023
ddf6e52
Adds an explanatory comment for the change
maryamariyan Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .../Microsoft.AspNetCore.Razor.LanguageServer/Tooltip/DefaultVSLSPTagHelperTooltipFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,9 +418,20 @@ static void ClassifyExistingTextRun(List<ClassifiedTextRun> runs, StringBuilder

private static ContainerElement CombineClassifiedTextRuns(IReadOnlyList<DescriptionClassification> descriptionClassifications, ImageElement glyph)
{
var isFirstElement = true;
var classifiedElementContainer = new List<ContainerElement>();
foreach (var classification in descriptionClassifications)
{
// Adds blank lines between multiple classified elements
if (isFirstElement)
maryamariyan marked this conversation as resolved.
Show resolved Hide resolved
{
isFirstElement = false;
}
else
{
classifiedElementContainer.Add(new ContainerElement(ContainerElementStyle.Wrapped, new ClassifiedTextElement()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could get a separation line back, but from a quick look at the legacy code I'd guess it may not be compatible with LSP. Also the accessibility on the old separator is TERRIBLE in dark mode. I've got perfect vision (when corrected) and I almost don't notice that it exists, so I feel like it's not a huge deal if we don't get the separator back.

}

classifiedElementContainer.Add(new ContainerElement(ContainerElementStyle.Wrapped, glyph, new ClassifiedTextElement(classification.Type)));

if (classification.Documentation.Count > 0)
Expand Down
24 changes: 18 additions & 6 deletions ...spNetCore.Razor.LanguageServer.Test/Completion/DefaultVSLSPTagHelperTooltipFactoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ public void TryCreateTooltip_ContainerElement_Attribute_MultipleAssociatedTagHel
// [Class Glyph] Microsoft.AspNetCore.OtherTagHelper
// Also uses List<string>s
Assert.Equal(ContainerElementStyle.Stacked, container.Style);
Assert.Equal(4, containerElements.Count);
Assert.Equal(5, containerElements.Count);

// [Class Glyph] Microsoft.AspNetCore.SomeTagHelper
var innerContainer = ((ContainerElement)containerElements[0]).Elements.ToList();
Expand All @@ -516,8 +516,14 @@ public void TryCreateTooltip_ContainerElement_Attribute_MultipleAssociatedTagHel
run => AssertExpectedClassification(run, ">", VSPredefinedClassificationTypeNames.Punctuation),
run => AssertExpectedClassification(run, "s", VSPredefinedClassificationTypeNames.Text));

// [Class Glyph] Microsoft.AspNetCore.OtherTagHelper
// new line
innerContainer = ((ContainerElement)containerElements[2]).Elements.ToList();
classifiedTextElement = (ClassifiedTextElement)innerContainer[0];
Assert.Single(innerContainer);
Assert.Empty(classifiedTextElement.Runs);

// [Class Glyph] Microsoft.AspNetCore.OtherTagHelper
innerContainer = ((ContainerElement)containerElements[3]).Elements.ToList();
classifiedTextElement = (ClassifiedTextElement)innerContainer[1];
Assert.Equal(2, innerContainer.Count);
Assert.Equal(ClassGlyph, innerContainer[0]);
Expand All @@ -529,7 +535,7 @@ public void TryCreateTooltip_ContainerElement_Attribute_MultipleAssociatedTagHel
run => AssertExpectedClassification(run, "OtherTagHelper", VSPredefinedClassificationTypeNames.Type));

// Also uses List<string>s
innerContainer = ((ContainerElement)containerElements[3]).Elements.ToList();
innerContainer = ((ContainerElement)containerElements[4]).Elements.ToList();
classifiedTextElement = (ClassifiedTextElement)innerContainer[0];
Assert.Single(innerContainer);
Assert.Collection(classifiedTextElement.Runs,
Expand Down Expand Up @@ -590,7 +596,7 @@ public void TryCreateTooltip_ContainerElement_Attribute_MultipleAssociatedAttrib
// [Property Glyph] bool? Microsoft.AspNetCore.SomeTagHelpers.AnotherTypeName.AnotherProperty
// Uses List<string>s
Assert.Equal(ContainerElementStyle.Stacked, container.Style);
Assert.Equal(4, containerElements.Count);
Assert.Equal(5, containerElements.Count);

// [TagHelper Glyph] string Microsoft.AspNetCore.SomeTagHelpers.SomeTypeName.SomeProperty
var innerContainer = ((ContainerElement)containerElements[0]).Elements.ToList();
Expand Down Expand Up @@ -622,8 +628,14 @@ public void TryCreateTooltip_ContainerElement_Attribute_MultipleAssociatedAttrib
run => AssertExpectedClassification(run, ">", VSPredefinedClassificationTypeNames.Punctuation),
run => AssertExpectedClassification(run, "s", VSPredefinedClassificationTypeNames.Text));

// [TagHelper Glyph] bool? Microsoft.AspNetCore.SomeTagHelpers.AnotherTypeName.AnotherProperty
// new line
innerContainer = ((ContainerElement)containerElements[2]).Elements.ToList();
classifiedTextElement = (ClassifiedTextElement)innerContainer[0];
Assert.Single(innerContainer);
Assert.Empty(classifiedTextElement.Runs);

// [TagHelper Glyph] bool? Microsoft.AspNetCore.SomeTagHelpers.AnotherTypeName.AnotherProperty
innerContainer = ((ContainerElement)containerElements[3]).Elements.ToList();
classifiedTextElement = (ClassifiedTextElement)innerContainer[1];
Assert.Equal(2, innerContainer.Count);
Assert.Equal(PropertyGlyph, innerContainer[0]);
Expand All @@ -642,7 +654,7 @@ public void TryCreateTooltip_ContainerElement_Attribute_MultipleAssociatedAttrib
run => AssertExpectedClassification(run, "AnotherProperty", VSPredefinedClassificationTypeNames.Identifier));

// Uses List<string>s
innerContainer = ((ContainerElement)containerElements[3]).Elements.ToList();
innerContainer = ((ContainerElement)containerElements[4]).Elements.ToList();
classifiedTextElement = (ClassifiedTextElement)innerContainer[0];
Assert.Single(innerContainer);
Assert.Collection(classifiedTextElement.Runs,
Expand Down