Fix rendering bug when splitting lines

The bug might occur if there are wide characters such as emojis
at the end of a line. The SplitLines method mixed cell width
and text length, which might give incorrect results. This commit
makes sure that comparison and calculation is done using cell width
where it's appropriate.
This commit is contained in:
Patrik Svensson 2020-10-24 01:45:41 +02:00
parent 041bd016a2
commit c9c0ad733f
11 changed files with 197 additions and 57 deletions

View File

@ -1,3 +1,4 @@
using System.Text;
using Shouldly;
using Spectre.Console.Rendering;
using Xunit;
@ -6,6 +7,16 @@ namespace Spectre.Console.Tests.Unit
{
public sealed class SegmentTests
{
[Fact]
public void Lol()
{
var context = new RenderContext(Encoding.UTF8, false);
var result = new Segment(" ").CellCount(context);
result.ShouldBe(4);
}
public sealed class TheSplitMethod
{
[Fact]
@ -31,7 +42,10 @@ namespace Spectre.Console.Tests.Unit
[Fact]
public void Should_Split_Segment()
{
var context = new RenderContext(Encoding.UTF8, false);
var lines = Segment.SplitLines(
context,
new[]
{
new Segment("Foo"),
@ -61,7 +75,9 @@ namespace Spectre.Console.Tests.Unit
[Fact]
public void Should_Split_Segments_With_Linebreak_In_Text()
{
var context = new RenderContext(Encoding.UTF8, false);
var lines = Segment.SplitLines(
context,
new[]
{
new Segment("Foo\n"),

View File

@ -57,7 +57,7 @@ namespace Spectre.Console.Internal
return;
}
var width = Segment.CellLength(context, segments);
var width = Segment.CellCount(context, segments);
if (width >= maxWidth)
{
return;

View File

@ -16,6 +16,11 @@ namespace Spectre.Console.Internal
public static int CellLength(this string text, RenderContext context)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
return Cell.GetCellLength(context, text);
}

View File

@ -21,6 +21,17 @@ namespace Spectre.Console.Internal
}
}
// TODO: We need to figure out why Segment.SplitLines fails
// if we let wcwidth (which returns -1 instead of 1)
// calculate the size for new line characters.
// That is correct from a Unicode perspective, but the
// algorithm was written before wcwidth was added and used
// to work with string length and not cell length.
if (rune == '\n')
{
return 1;
}
return UnicodeCalculator.GetWidth(rune);
});
}

View File

@ -79,11 +79,37 @@ namespace Spectre.Console.Rendering
/// </summary>
/// <param name="context">The render context.</param>
/// <returns>The number of cells that this segment occupies in the console.</returns>
public int CellLength(RenderContext context)
public int CellCount(RenderContext context)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
return Text.CellLength(context);
}
/// <summary>
/// Gets the number of cells that the segments occupies in the console.
/// </summary>
/// <param name="context">The render context.</param>
/// <param name="segments">The segments to measure.</param>
/// <returns>The number of cells that the segments occupies in the console.</returns>
public static int CellCount(RenderContext context, IEnumerable<Segment> segments)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
if (segments is null)
{
throw new ArgumentNullException(nameof(segments));
}
return segments.Sum(segment => segment.CellCount(context));
}
/// <summary>
/// Returns a new segment without any trailing line endings.
/// </summary>
@ -124,35 +150,41 @@ namespace Spectre.Console.Rendering
return new Segment(Text, Style);
}
/// <summary>
/// Gets the number of cells that the segments occupies in the console.
/// </summary>
/// <param name="context">The render context.</param>
/// <param name="segments">The segments to measure.</param>
/// <returns>The number of cells that the segments occupies in the console.</returns>
public static int CellLength(RenderContext context, IEnumerable<Segment> segments)
{
return segments.Sum(segment => segment.CellLength(context));
}
/// <summary>
/// Splits the provided segments into lines.
/// </summary>
/// <param name="context">The render context.</param>
/// <param name="segments">The segments to split.</param>
/// <returns>A collection of lines.</returns>
public static List<SegmentLine> SplitLines(IEnumerable<Segment> segments)
public static List<SegmentLine> SplitLines(RenderContext context, IEnumerable<Segment> segments)
{
return SplitLines(segments, int.MaxValue);
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
if (segments is null)
{
throw new ArgumentNullException(nameof(segments));
}
return SplitLines(context, segments, int.MaxValue);
}
/// <summary>
/// Splits the provided segments into lines with a maximum width.
/// </summary>
/// <param name="context">The render context.</param>
/// <param name="segments">The segments to split into lines.</param>
/// <param name="maxWidth">The maximum width.</param>
/// <returns>A list of lines.</returns>
public static List<SegmentLine> SplitLines(IEnumerable<Segment> segments, int maxWidth)
public static List<SegmentLine> SplitLines(RenderContext context, IEnumerable<Segment> segments, int maxWidth)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
if (segments is null)
{
throw new ArgumentNullException(nameof(segments));
@ -167,9 +199,10 @@ namespace Spectre.Console.Rendering
{
var segment = stack.Pop();
if (line.Width + segment.Text.Length > maxWidth)
// Does this segment make the line exceed the max width?
if (line.CellCount(context) + segment.CellCount(context) > maxWidth)
{
var diff = -(maxWidth - (line.Width + segment.Text.Length));
var diff = -(maxWidth - (line.Length + segment.Text.Length));
var offset = segment.Text.Length - diff;
var (first, second) = segment.Split(offset);
@ -186,11 +219,13 @@ namespace Spectre.Console.Rendering
continue;
}
// Does the segment contain a newline?
if (segment.Text.Contains("\n"))
{
// Is it a new line?
if (segment.Text == "\n")
{
if (line.Width > 0 || segment.IsLineBreak)
if (line.Length != 0 || segment.IsLineBreak)
{
lines.Add(line);
line = new SegmentLine();
@ -213,7 +248,7 @@ namespace Spectre.Console.Rendering
if (parts.Length > 1)
{
if (line.Width > 0)
if (line.Length > 0)
{
lines.Add(line);
line = new SegmentLine();
@ -247,16 +282,21 @@ namespace Spectre.Console.Rendering
/// <param name="segment">The segment to split.</param>
/// <param name="overflow">The overflow strategy to use.</param>
/// <param name="context">The render context.</param>
/// <param name="width">The maximum width.</param>
/// <param name="maxWidth">The maximum width.</param>
/// <returns>A list of segments that has been split.</returns>
public static List<Segment> SplitOverflow(Segment segment, Overflow? overflow, RenderContext context, int width)
public static List<Segment> SplitOverflow(Segment segment, Overflow? overflow, RenderContext context, int maxWidth)
{
if (segment is null)
{
throw new ArgumentNullException(nameof(segment));
}
if (segment.CellLength(context) <= width)
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
if (segment.CellCount(context) <= maxWidth)
{
return new List<Segment>(1) { segment };
}
@ -275,7 +315,7 @@ namespace Spectre.Console.Rendering
var index = totalLength - lengthLeft;
// How many characters should we take?
var take = Math.Min(width, totalLength - index);
var take = Math.Min(maxWidth, totalLength - index);
if (take <= 0)
{
// This shouldn't really occur, but I don't like
@ -289,24 +329,24 @@ namespace Spectre.Console.Rendering
}
else if (overflow == Overflow.Crop)
{
if (Math.Max(0, width - 1) == 0)
if (Math.Max(0, maxWidth - 1) == 0)
{
result.Add(new Segment(string.Empty, segment.Style));
}
else
{
result.Add(new Segment(segment.Text.Substring(0, width), segment.Style));
result.Add(new Segment(segment.Text.Substring(0, maxWidth), segment.Style));
}
}
else if (overflow == Overflow.Ellipsis)
{
if (Math.Max(0, width - 1) == 0)
if (Math.Max(0, maxWidth - 1) == 0)
{
result.Add(new Segment("…", segment.Style));
}
else
{
result.Add(new Segment(segment.Text.Substring(0, width - 1) + "…", segment.Style));
result.Add(new Segment(segment.Text.Substring(0, maxWidth - 1) + "…", segment.Style));
}
}
@ -337,14 +377,14 @@ namespace Spectre.Console.Rendering
var totalWidth = 0;
foreach (var segment in segments)
{
var segmentWidth = segment.CellLength(context);
if (totalWidth + segmentWidth > maxWidth)
var segmentCellWidth = segment.CellCount(context);
if (totalWidth + segmentCellWidth > maxWidth)
{
break;
}
result.Add(segment);
totalWidth += segmentWidth;
totalWidth += segmentCellWidth;
}
if (result.Count == 0 && segments.Any())
@ -368,12 +408,17 @@ namespace Spectre.Console.Rendering
/// <returns>A new truncated segment, or <c>null</c>.</returns>
public static Segment? Truncate(RenderContext context, Segment segment, int maxWidth)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
if (segment is null)
{
return null;
}
if (segment.CellLength(context) <= maxWidth)
if (segment.CellCount(context) <= maxWidth)
{
return segment;
}
@ -381,7 +426,8 @@ namespace Spectre.Console.Rendering
var builder = new StringBuilder();
foreach (var character in segment.Text)
{
if (Cell.GetCellLength(context, builder.ToString()) >= maxWidth)
var accumulatedCellWidth = builder.ToString().CellLength(context);
if (accumulatedCellWidth >= maxWidth)
{
break;
}
@ -399,6 +445,11 @@ namespace Spectre.Console.Rendering
internal static IEnumerable<Segment> Merge(IEnumerable<Segment> segments)
{
if (segments is null)
{
throw new ArgumentNullException(nameof(segments));
}
var result = new List<Segment>();
var previous = (Segment?)null;
@ -432,6 +483,33 @@ namespace Spectre.Console.Rendering
internal static Segment TruncateWithEllipsis(string text, Style style, RenderContext context, int maxWidth)
{
if (text is null)
{
throw new ArgumentNullException(nameof(text));
}
if (style is null)
{
throw new ArgumentNullException(nameof(style));
}
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
var overflow = SplitOverflow(new Segment(text, style), Overflow.Ellipsis, context, maxWidth);
if (overflow.Count == 0)
{
if (maxWidth > 0)
{
return new Segment(text, style);
}
// We got space for an ellipsis
return new Segment("…", style);
}
return SplitOverflow(
new Segment(text, style),
Overflow.Ellipsis,
@ -441,7 +519,17 @@ namespace Spectre.Console.Rendering
internal static List<Segment> TruncateWithEllipsis(IEnumerable<Segment> segments, RenderContext context, int maxWidth)
{
if (CellLength(context, segments) <= maxWidth)
if (segments is null)
{
throw new ArgumentNullException(nameof(segments));
}
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}
if (CellCount(context, segments) <= maxWidth)
{
return new List<Segment>(segments);
}
@ -459,6 +547,11 @@ namespace Spectre.Console.Rendering
internal static List<Segment> TrimEnd(IEnumerable<Segment> segments)
{
if (segments is null)
{
throw new ArgumentNullException(nameof(segments));
}
var stack = new Stack<Segment>();
var checkForWhitespace = true;
foreach (var segment in segments.Reverse())
@ -481,6 +574,11 @@ namespace Spectre.Console.Rendering
internal static List<List<SegmentLine>> MakeSameHeight(int cellHeight, List<List<SegmentLine>> cells)
{
if (cells is null)
{
throw new ArgumentNullException(nameof(cells));
}
foreach (var cell in cells)
{
if (cell.Count < cellHeight)

View File

@ -13,16 +13,21 @@ namespace Spectre.Console.Rendering
/// <summary>
/// Gets the width of the line.
/// </summary>
public int Width => this.Sum(line => line.Text.Length);
public int Length => this.Sum(line => line.Text.Length);
/// <summary>
/// Gets the cell width of the segment line.
/// Gets the number of cells the segment line occupies.
/// </summary>
/// <param name="context">The render context.</param>
/// <returns>The cell width of the segment line.</returns>
public int CellWidth(RenderContext context)
public int CellCount(RenderContext context)
{
return this.Sum(line => line.CellLength(context));
if (context is null)
{
throw new System.ArgumentNullException(nameof(context));
}
return Segment.CellCount(context, this);
}
/// <summary>
@ -31,6 +36,11 @@ namespace Spectre.Console.Rendering
/// <param name="segment">The segment to prepend.</param>
public void Prepend(Segment segment)
{
if (segment is null)
{
throw new System.ArgumentNullException(nameof(segment));
}
Insert(0, segment);
}
}

View File

@ -66,7 +66,7 @@ namespace Spectre.Console
}
var child = _child.Render(context, maxWidth - paddingWidth);
foreach (var (_, _, _, line) in Segment.SplitLines(child).Enumerate())
foreach (var (_, _, _, line) in Segment.SplitLines(context, child).Enumerate())
{
// Left padding
if (Padding.Left != 0)
@ -83,7 +83,7 @@ namespace Spectre.Console
}
// Missing space on right side?
var lineWidth = line.CellWidth(context);
var lineWidth = line.CellCount(context);
var diff = width - lineWidth - Padding.Left - Padding.Right;
if (diff > 0)
{

View File

@ -94,7 +94,7 @@ namespace Spectre.Console
// Split the child segments into lines.
var childSegments = ((IRenderable)child).Render(context, childWidth);
foreach (var line in Segment.SplitLines(childSegments, childWidth))
foreach (var line in Segment.SplitLines(context, childSegments, childWidth))
{
if (line.Count == 1 && line[0].IsWhiteSpace)
{
@ -109,7 +109,7 @@ namespace Spectre.Console
content.AddRange(line);
// Do we need to pad the panel?
var length = line.Sum(segment => segment.CellLength(context));
var length = line.Sum(segment => segment.CellCount(context));
if (length < childWidth)
{
var diff = childWidth - length;
@ -148,7 +148,7 @@ namespace Spectre.Console
var headerWidth = panelWidth - (EdgeWidth * 2);
var header = Segment.TruncateWithEllipsis(Header.Text, Header.Style ?? borderStyle, context, headerWidth);
var excessWidth = headerWidth - header.CellLength(context);
var excessWidth = headerWidth - header.CellCount(context);
if (excessWidth > 0)
{
switch (Header.Alignment ?? Justify.Left)

View File

@ -119,8 +119,8 @@ namespace Spectre.Console
return new Measurement(0, 0);
}
var min = _lines.Max(line => line.Max(segment => segment.CellLength(context)));
var max = _lines.Max(x => x.CellWidth(context));
var min = _lines.Max(line => line.Max(segment => segment.CellCount(context)));
var max = _lines.Max(x => x.CellCount(context));
return new Measurement(min, Math.Min(max, maxWidth));
}
@ -187,7 +187,7 @@ namespace Spectre.Console
return new List<SegmentLine>();
}
if (_lines.Max(x => x.CellWidth(context)) <= maxWidth)
if (_lines.Max(x => x.CellCount(context)) <= maxWidth)
{
return Clone();
}
@ -231,7 +231,7 @@ namespace Spectre.Console
continue;
}
var length = current.CellLength(context);
var length = current.CellCount(context);
if (length > maxWidth)
{
// The current segment is longer than the width of the console,
@ -239,7 +239,7 @@ namespace Spectre.Console
var segments = Segment.SplitOverflow(current, Overflow, context, maxWidth);
if (segments.Count > 0)
{
if (line.CellWidth(context) + segments[0].CellLength(context) > maxWidth)
if (line.CellCount(context) + segments[0].CellCount(context) > maxWidth)
{
lines.Add(line);
line = new SegmentLine();
@ -259,7 +259,7 @@ namespace Spectre.Console
}
else
{
if (line.CellWidth(context) + length > maxWidth)
if (line.CellCount(context) + length > maxWidth)
{
line.Add(Segment.Empty);
lines.Add(line);

View File

@ -52,7 +52,7 @@ namespace Spectre.Console
// Get the title and make sure it fits.
var title = GetTitleSegments(context, Title, maxWidth - 6);
if (Segment.CellLength(context, title) > maxWidth - 6)
if (Segment.CellCount(context, title) > maxWidth - 6)
{
// Truncate the title
title = Segment.TruncateWithEllipsis(title, context, maxWidth - 6);
@ -88,13 +88,13 @@ namespace Spectre.Console
{
var alignment = Alignment ?? Justify.Center;
var titleLength = Segment.CellLength(context, title);
var titleLength = Segment.CellCount(context, title);
if (alignment == Justify.Left)
{
var left = new Segment(new string('─', 2) + " ", Style ?? Style.Plain);
var rightLength = maxWidth - titleLength - left.CellLength(context) - 1;
var rightLength = maxWidth - titleLength - left.CellCount(context) - 1;
var right = new Segment(" " + new string('─', rightLength), Style ?? Style.Plain);
return (left, right);
@ -104,7 +104,7 @@ namespace Spectre.Console
var leftLength = ((maxWidth - titleLength) / 2) - 1;
var left = new Segment(new string('─', leftLength) + " ", Style ?? Style.Plain);
var rightLength = maxWidth - titleLength - left.CellLength(context) - 1;
var rightLength = maxWidth - titleLength - left.CellCount(context) - 1;
var right = new Segment(" " + new string('─', rightLength), Style ?? Style.Plain);
return (left, right);
@ -113,7 +113,7 @@ namespace Spectre.Console
{
var right = new Segment(" " + new string('─', 2), Style ?? Style.Plain);
var leftLength = maxWidth - titleLength - right.CellLength(context) - 1;
var leftLength = maxWidth - titleLength - right.CellCount(context) - 1;
var left = new Segment(new string('─', leftLength) + " ", Style ?? Style.Plain);
return (left, right);

View File

@ -217,7 +217,7 @@ namespace Spectre.Console
var justification = _columns[columnIndex].Alignment;
var childContext = context.WithJustification(justification);
var lines = Segment.SplitLines(cell.Render(childContext, rowWidth));
var lines = Segment.SplitLines(context, cell.Render(childContext, rowWidth));
cellHeight = Math.Max(cellHeight, lines.Count);
cells.Add(lines);
}
@ -261,7 +261,7 @@ namespace Spectre.Console
rowResult.AddRange(cell[cellRowIndex]);
// Pad cell content right
var length = cell[cellRowIndex].Sum(segment => segment.CellLength(context));
var length = cell[cellRowIndex].Sum(segment => segment.CellCount(context));
if (length < columnWidths[cellIndex])
{
rowResult.Add(new Segment(new string(' ', columnWidths[cellIndex] - length)));
@ -295,7 +295,7 @@ namespace Spectre.Console
Aligner.Align(context, rowResult, Alignment, actualMaxWidth);
// Is the row larger than the allowed max width?
if (Segment.CellLength(context, rowResult) > actualMaxWidth)
if (Segment.CellCount(context, rowResult) > actualMaxWidth)
{
result.AddRange(Segment.Truncate(context, rowResult, actualMaxWidth));
}