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

SLVS-1600 Fix single quotation marks not formatted correctly in diff view of rule help description #5949

Closed
wants to merge 5 commits into from
Closed

SLVS-1600 Fix single quotation marks not formatted correctly in diff view of rule help description #5949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0574b6f
SLVS-1600 CaYC: cleanup touched files on save
gabriela-trutan-sonarsource Jan 10, 2025
eed465f
SLVS-1600 Group all consecutive changes sub-pieces in the same span
gabriela-trutan-sonarsource Jan 10, 2025
1304fab
SLVS-1600 Fix single quotation marks in diff view are not displayed c…
gabriela-trutan-sonarsource Jan 10, 2025
d5f4c21
SLVS-1600 Move html encoding inside DiffTranslator to have more contr…
gabriela-trutan-sonarsource Jan 10, 2025
47ada12
SLVS-1600 Remove no longer used variables
gabriela-trutan-sonarsource Jan 10, 2025
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
26 changes: 22 additions & 4 deletions src/Education.UnitTests/XamlGenerator/DiffTranslatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using SonarLint.VisualStudio.Education.XamlGenerator;
using SonarLint.VisualStudio.TestInfrastructure;

Expand Down Expand Up @@ -123,8 +121,28 @@ public void GetDiffXaml_LineHasNoSubPieces_StillCorrectResult()

(string result1, string result2) = testSubject.GetDiffXaml(input1, input2);

result1.Should().BeEquivalentTo("<Span Style=\"{DynamicResource NonCompliant_Diff}\">\n <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\">} </Span>\n <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\">if</Span>\n <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\"> (</Span>\n <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\">condition2</Span>\n <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\">) {</Span>\n</Span>\n");
result2.Should().BeEquivalentTo("<Span Style=\"{DynamicResource Compliant_Diff}\">\n <Span Style=\"{DynamicResource Sub_Compliant_Diff}\">}</Span>\n</Span>\n<Span Style=\"{DynamicResource Compliant_Diff}\">if (condition2) {</Span>");
result1.Should().BeEquivalentTo("<Span Style=\"{DynamicResource NonCompliant_Diff}\">\n <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\">} if (condition2) {</Span>\n</Span>\n");
result2.Should().BeEquivalentTo(
"<Span Style=\"{DynamicResource Compliant_Diff}\">\n <Span Style=\"{DynamicResource Sub_Compliant_Diff}\">}</Span>\n</Span>\n<Span Style=\"{DynamicResource Compliant_Diff}\">if (condition2) {</Span>");
}

/// <summary>
/// Making sure that all consecutive changed sub-pieces are grouped together is needed to ensure that html escape characters (e.g. &#39;)
/// that end up in different sup-pieces will be rendered correctly in the UI
/// </summary>
[TestMethod]
public void GetDiffXaml_LineHasHasConsecutiveChangedSubPieces_GroupsConsecutiveSubPiecesInOneSpan()
{
var input1 = "var a; // Noncompliant var b=2;";
var input2 = "var a; ";

var testSubject = CreateTestSubject();

(string result1, string result2) = testSubject.GetDiffXaml(input1, input2);

result1.Should().BeEquivalentTo(
"<Span Style=\"{DynamicResource NonCompliant_Diff}\">var a; <Span Style=\"{DynamicResource Sub_NonCompliant_Diff}\">// Noncompliant var b=2;</Span></Span>");
result2.Should().BeEquivalentTo("<Span Style=\"{DynamicResource Compliant_Diff}\">var a; </Span>");
}

private static DiffTranslator CreateTestSubject(IXamlWriterFactory xamlWriterFactory = null)
Expand Down
49 changes: 33 additions & 16 deletions src/Education.UnitTests/XamlGenerator/RuleHelpXamlTranslatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
*/

using System.Text;
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;
using SonarLint.VisualStudio.Education.XamlGenerator;
using SonarLint.VisualStudio.TestInfrastructure;
Expand All @@ -34,7 +32,7 @@ public class RuleHelpXamlTranslatorTests
public void Factory_MefCtor_CheckExports()
{
MefTestHelpers.CheckTypeCanBeImported<RuleHelpXamlTranslatorFactory, IRuleHelpXamlTranslatorFactory>
(MefTestHelpers.CreateExport<IXamlWriterFactory>(),
(MefTestHelpers.CreateExport<IXamlWriterFactory>(),
MefTestHelpers.CreateExport<IDiffTranslator>());
}

Expand Down Expand Up @@ -220,7 +218,8 @@ public void TranslateHtmlToXaml_TwoCompliantCode_DoesNotHighlightCode()

diffTranslator.Setup(d => d.GetDiffXaml(nonCompliantText1, compliantText)).Returns((noncompliantXaml, compliantXaml));

var htmlText = $"<pre data-diff-type=\"compliant\" data-diff-id=\"1\">{compliantText}</pre>\n<pre data-diff-type =\"noncompliant\" data-diff-id=\"1\">{nonCompliantText1}</pre>\n<pre data-diff-type =\"noncompliant\" data-diff-id=\"1\">{nonCompliantText2}</pre>";
var htmlText
= $"<pre data-diff-type=\"compliant\" data-diff-id=\"1\">{compliantText}</pre>\n<pre data-diff-type =\"noncompliant\" data-diff-id=\"1\">{nonCompliantText1}</pre>\n<pre data-diff-type =\"noncompliant\" data-diff-id=\"1\">{nonCompliantText2}</pre>";

var expectedText = @"<Section xml:space=""preserve"" Style=""{DynamicResource Pre_Section}"">
<Paragraph>Same text 1
Expand Down Expand Up @@ -270,7 +269,8 @@ public void TranslateHtmlToXaml_TwoDiffs_HighlightsCodeCorrectly()
diffTranslator.Setup(d => d.GetDiffXaml(noncompliantText1, compliantText1)).Returns((noncompliantXaml1, compliantXaml1));
diffTranslator.Setup(d => d.GetDiffXaml(noncompliantText2, compliantText2)).Returns((noncompliantXaml2, compliantXaml2));

var htmlText = $"<pre data-diff-type=\"compliant\" data-diff-id=\"1\">{compliantText1}</pre><pre data-diff-type=\"compliant\" data-diff-id=\"2\">{compliantText2}</pre><pre data-diff-type=\"noncompliant\" data-diff-id=\"1\">{noncompliantText1}</pre><pre data-diff-type=\"noncompliant\" data-diff-id=\"2\">{noncompliantText2}</pre>";
var htmlText
= $"<pre data-diff-type=\"compliant\" data-diff-id=\"1\">{compliantText1}</pre><pre data-diff-type=\"compliant\" data-diff-id=\"2\">{compliantText2}</pre><pre data-diff-type=\"noncompliant\" data-diff-id=\"1\">{noncompliantText1}</pre><pre data-diff-type=\"noncompliant\" data-diff-id=\"2\">{noncompliantText2}</pre>";

var expectedText = @"<Section xml:space=""preserve"" Style=""{DynamicResource Pre_Section}"">
<Paragraph>Same text 1
Expand Down Expand Up @@ -354,19 +354,9 @@ same 1</Paragraph>
[TestMethod]
public void TranslateHtmlToXaml_DataDiffWithAngleBracket_XMLParsable()
{
var diffTranslator = new Mock<IDiffTranslator>();

IRuleHelpXamlTranslator testSubject = CreateTestSubject(diffTranslator: diffTranslator.Object);

var testSubject = CreateTestSubject();
var compliantText = "#include &lt;vector&gt;";
var nonCompliantText = "#include &lt;vector&gt;";

var compliantXaml = @"#include &lt;vector&gt;";

var noncompliantXaml = @"#include &lt;vector&gt;";

diffTranslator.Setup(d => d.GetDiffXaml(compliantText, nonCompliantText)).Returns((noncompliantXaml, compliantXaml));

var htmlText = $"<pre data-diff-type=\"compliant\" data-diff-id=\"1\">{compliantText}</pre>\n<pre data-diff-type =\"noncompliant\" data-diff-id=\"1\">{nonCompliantText}</pre>";

var expectedText = @"<Section xml:space=""preserve"" Style=""{DynamicResource Pre_Section}"">
Expand All @@ -379,6 +369,33 @@ public void TranslateHtmlToXaml_DataDiffWithAngleBracket_XMLParsable()
result.Replace("\r\n", "\n").Should().Be(expectedText.Replace("\r\n", "\n"));
}

[TestMethod]
public void TranslateHtmlToXaml_SingleQuotesPresentInDiffCode_AreEscapedCorrectly()
{
var testSubject = CreateTestSubject();

var htmlText = @"
<pre data-diff-id=""1"" data-diff-type=""noncompliant"">
function f(a, g){} // Noncompliant: 'f' returns 'b' on two different return statements
</pre>
<pre data-diff-id=""1"" data-diff-type=""compliant"">
function f(a, g){}
</pre>
";

var expectedText
= @"
<Section xml:space=""preserve"" Style=""{DynamicResource Pre_Section}""><Paragraph><Span Style=""{DynamicResource NonCompliant_Diff}"">function f(a, g<Span Style=""{DynamicResource Sub_NonCompliant_Diff}"">){} // Noncompliant: &#39;f&#39; returns &#39;b&#39; on two different return statements</Span></Span>
</Paragraph></Section>
<Section xml:space=""preserve"" Style=""{DynamicResource Pre_Section}""><Paragraph><Span Style=""{DynamicResource Compliant_Diff}"">function f(a, g<Span Style=""{DynamicResource Sub_Compliant_Diff}"">){}</Span></Span>
</Paragraph></Section>
";

var result = testSubject.TranslateHtmlToXaml(htmlText);

result.Should().Be(expectedText);
}

private static IRuleHelpXamlTranslator CreateTestSubject(IXamlWriterFactory xamlWriterFactory = null, IDiffTranslator diffTranslator = null)
{
xamlWriterFactory ??= new XamlWriterFactory();
Expand Down
60 changes: 50 additions & 10 deletions src/Education/XamlGenerator/DiffTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Collections.Generic;
using System.ComponentModel.Composition;
using System.Text;
using System.Web;
using System.Xml;
using DiffPlex.DiffBuilder;
using DiffPlex.DiffBuilder.Model;
Expand Down Expand Up @@ -52,7 +52,9 @@ public DiffTranslator(IXamlWriterFactory xamlWriterFactory)

public (string noncompliantXaml, string compliantXaml) GetDiffXaml(string noncompliantHtml, string compliantHtml)
{
var resultDiff = SideBySideDiffBuilder.Diff(oldText: noncompliantHtml, newText: compliantHtml, ignoreWhiteSpace: false);
var encodedNonCompliantHtml = HttpUtility.HtmlEncode(noncompliantHtml);
var encodedCompliantHtml = HttpUtility.HtmlEncode(compliantHtml);
var resultDiff = SideBySideDiffBuilder.Diff(oldText: encodedNonCompliantHtml, newText: encodedCompliantHtml, ignoreWhiteSpace: false);

var highlightedNonCompliant = HighlightLines(resultDiff.OldText.Lines, StyleResourceNames.NonCompliant_Diff, StyleResourceNames.Sub_NonCompliant_Diff);
var highlightedCompliant = HighlightLines(resultDiff.NewText.Lines, StyleResourceNames.Compliant_Diff, StyleResourceNames.Sub_Compliant_Diff);
Expand All @@ -69,7 +71,10 @@ private string HighlightLines(List<DiffPiece> lines, StyleResourceNames style, S
{
var line = lines[i];

if (string.IsNullOrEmpty(line.Text)) continue;
if (string.IsNullOrEmpty(line.Text))
{
continue;
}

if (line.Type != ChangeType.Unchanged)
{
Expand All @@ -82,7 +87,7 @@ private string HighlightLines(List<DiffPiece> lines, StyleResourceNames style, S
}
else
{
writer.WriteString(line.Text);
writer.WriteRaw(line.Text);
}

// The differ removes the next line element as it returns a list of lines.
Expand All @@ -100,30 +105,65 @@ private string HighlightLines(List<DiffPiece> lines, StyleResourceNames style, S
return sb.ToString();
}

private void WriteSubPieces(XmlWriter writer, StyleResourceNames style, DiffPiece line)
private static void WriteSubPieces(XmlWriter writer, StyleResourceNames style, DiffPiece line)
{
if (line.SubPieces.Count == 0)
{
writer.WriteString(line.Text);
writer.WriteRaw(line.Text);
return;
}

foreach (var subPiece in line.SubPieces)
for (var i = 0; i < line.SubPieces.Count; i++)
{
if (string.IsNullOrEmpty(subPiece.Text)) continue;
var subPiece = line.SubPieces[i];
if (IsSubPieceEmpty(subPiece))
{
continue;
}

if (subPiece.Type != ChangeType.Unchanged)
{
writer.WriteStartElement("Span");
writer.ApplyStyleToElement(style);
writer.WriteString(subPiece.Text);
WriteSubPiece(writer, subPiece);
// group all consecutive changed sub-pieces in the same span
// this is needed to prevent html escape characters (e.g. &#39;) which end up in different sup-pieces to be split into different spans,
// which then won't be rendered correctly in the UI
while (IsNextSubPieceChanged(line, i))
{
i++;
var nextSupPiece = line.SubPieces[i];
WriteSubPiece(writer, nextSupPiece);
}

writer.WriteEndElement();
}
else
{
writer.WriteString(subPiece.Text);
WriteSubPiece(writer, subPiece);
}
}
}

/// <summary>
/// When writing a sub-piece, it's important to use the WriteRaw method to avoid escaping the special characters.
/// The string is expected to already be html encoded, which means that when special characters (e.g. &) are escaped in html escape characters (e.g. &#39;),
/// we end up with invalid characters that can't be interpreted (e.g. amp;#39;).
/// </summary>
/// <param name="writer"></param>
/// <param name="subPiece"></param>
private static void WriteSubPiece(XmlWriter writer, DiffPiece subPiece) => writer.WriteRaw(subPiece.Text);

private static bool IsSubPieceEmpty(DiffPiece subPiece) => string.IsNullOrEmpty(subPiece.Text);

private static bool IsNextSubPieceChanged(DiffPiece line, int currentPosition)
{
if (currentPosition >= line.SubPieces.Count - 1)
{
return false;
}
var nextSupPiece = line.SubPieces[currentPosition + 1];
return !IsSubPieceEmpty(nextSupPiece) && nextSupPiece.Type != ChangeType.Unchanged;
}
}
}
13 changes: 3 additions & 10 deletions src/Education/XamlGenerator/RuleHelpXamlTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System;
using System.Collections.Generic;
using System.ComponentModel.Composition;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Web;
Expand Down Expand Up @@ -387,11 +384,7 @@ private void ProcessElement()

private static XmlReader CreateXmlReader(string data)
{
var settings = new XmlReaderSettings
{
ConformanceLevel = ConformanceLevel.Fragment,
IgnoreWhitespace = false
};
var settings = new XmlReaderSettings { ConformanceLevel = ConformanceLevel.Fragment, IgnoreWhitespace = false };

var stream = new StringReader(data);
return XmlReader.Create(stream, settings);
Expand Down Expand Up @@ -420,7 +413,7 @@ private void WriteText(string text)
}

Debug.Assert(textToken is IRuleCrossRef || textToken is ISimpleText,
$"Unknown text token type. Type: {textToken.GetType().FullName}");
$"Unknown text token type. Type: {textToken.GetType().FullName}");
}
}

Expand Down Expand Up @@ -608,7 +601,7 @@ private void ReplaceDiffs(StringBuilder sb)
continue;
}

var diffXaml = diffTranslator.GetDiffXaml(HttpUtility.HtmlEncode(diffCodes[noncompliantKey]), HttpUtility.HtmlEncode(diffCodes[compliantKey]));
var diffXaml = diffTranslator.GetDiffXaml(diffCodes[noncompliantKey], diffCodes[compliantKey]);

sb.Replace(noncompliantKey, diffXaml.noncompliantXaml);
sb.Replace(compliantKey, diffXaml.compliantXaml);
Expand Down
Loading