diff --git a/src/SharpMUTerm.Core/Configuration/PreferenceSettings.cs b/src/SharpMUTerm.Core/Configuration/PreferenceSettings.cs index 09d7495..3ad4034 100644 --- a/src/SharpMUTerm.Core/Configuration/PreferenceSettings.cs +++ b/src/SharpMUTerm.Core/Configuration/PreferenceSettings.cs @@ -32,6 +32,31 @@ public sealed class TextSettings /// Honour the blink attribute rather than dropping it. public bool AllowBlink { get; set; } + /// + /// How many spaces a tab in server output is drawn as. Zero removes tabs entirely. + /// + /// A tab used to travel the pipeline as one character, so the wrap, the pane width and + /// MarkupWidth all counted it as one cell while the terminal painted it as a jump to + /// the next tab stop — a disagreement of up to seven columns per tab between the layout and the + /// screen. StyledText.ExpandTabs substitutes the spaces before anything measures the line. + /// + /// + /// It is a fixed run of spaces, not a tab stop: a real tab's width depends on the column it starts + /// in, and tracking that buys alignment nobody asked for on output where a tab is a crude separator + /// rather than a layout instruction. + /// + /// + public int TabWidth { get; set; } = DefaultTabWidth; + + /// Four, the width a tab is written to mean in most MU* output and most editors. + public const int DefaultTabWidth = 4; + + /// + /// The widest a tab may be set to. A tab is a separator here, and one wider than this pushes the + /// text after it off a narrow pane rather than lining anything up. + /// + public const int MaxTabWidth = 16; + /// Underline MXP/Pueblo/web links so they read as clickable. public bool UnderlineHyperlinks { get; set; } = true; diff --git a/src/SharpMUTerm.Core/Session/WorldSession.cs b/src/SharpMUTerm.Core/Session/WorldSession.cs index afb96a3..b67719e 100644 --- a/src/SharpMUTerm.Core/Session/WorldSession.cs +++ b/src/SharpMUTerm.Core/Session/WorldSession.cs @@ -324,6 +324,16 @@ private void OnOutputReceived(object? sender, TelnetOutputEventArgs e) } } + /// + /// The configured tab width, clamped into range. Clamped at the point of use rather than in the + /// setter because config.json is hand-edited: a negative number there would otherwise throw + /// out of on the read loop, on a line of ordinary output. + /// + private int TabWidth => Math.Clamp( + _text?.TabWidth ?? TextSettings.DefaultTabWidth, + 0, + TextSettings.MaxTabWidth); + private void ProcessOutputLine(StyledLine line) { // Colour is stripped from what the *server* sent, before the triggers run: a highlight rule @@ -333,6 +343,11 @@ private void ProcessOutputLine(StyledLine line) line = StyledText.StripColour(line); } + // Before the triggers, so a pattern matches the spaces the reader sees rather than a tab they + // have no way to know is there. Read per line like the setting above it, so changing the width + // takes effect on the next line of output rather than on the next restart. + line = StyledText.ExpandTabs(line, TabWidth); + var result = Triggers.Process(line); foreach (var invocation in result.ScriptInvocations) diff --git a/src/SharpMUTerm.Core/Telnet/TelnetSession.cs b/src/SharpMUTerm.Core/Telnet/TelnetSession.cs index 323d857..876eec0 100644 --- a/src/SharpMUTerm.Core/Telnet/TelnetSession.cs +++ b/src/SharpMUTerm.Core/Telnet/TelnetSession.cs @@ -681,10 +681,26 @@ private async Task ReadLoopAsync(CancellationToken cancellationToken) } } + /// + /// Sends one line to the world. + /// + /// The terminator is the library's, not ours. TelnetInterpreter.SendAsync is a + /// line send: it appends the line ending itself. Adding \r\n here as well put + /// <text>\r\n\r\n on the wire for every line this client has ever sent — a spurious + /// empty command after each real one. On a MU* connect screen that empty line is a command like any + /// other, and the servers that redisplay their banner for an unrecognised one did so on every single + /// connect: the doubled welcome screen in the transcripts is this bug, not the server's. + /// + /// + /// It is bytes rather than a string because the encoding is this session's decision and not the + /// interpreter's (see ), and it goes through so + /// the library still escapes a literal IAC in user text as data. + /// + /// public ValueTask SendLineAsync(string text, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(text); - var bytes = CurrentEncoding.Encoding.GetBytes(text + "\r\n"); + var bytes = CurrentEncoding.Encoding.GetBytes(text); return SendAsync(bytes, cancellationToken); } diff --git a/src/SharpMUTerm.Core/Text/StyledText.cs b/src/SharpMUTerm.Core/Text/StyledText.cs index 12654d6..9456d60 100644 --- a/src/SharpMUTerm.Core/Text/StyledText.cs +++ b/src/SharpMUTerm.Core/Text/StyledText.cs @@ -100,6 +100,68 @@ public static StyledLine StripColour(StyledLine line) return new StyledLine(spans, line.RuleColor); } + /// + /// Replaces every tab in with spaces, keeping each + /// run's style and interaction. + /// + /// A tab arrives from the server and travels the whole pipeline as one character, so everything that + /// measures a line — the wrap, the pane's width, MarkupWidth — counts it as one cell + /// while the terminal paints it as a jump to the next tab stop. The two disagree by up to seven + /// columns on every tab, which is the same class of defect as chrome that grows on wire data: the + /// layout is computed against a width the screen does not use. + /// + /// + /// This is not tab-stop expansion. A real tab advances to the next multiple of the stop, + /// so its width depends on the column it starts in; this substitutes a fixed run of spaces, which is + /// what was asked for and what MU* output — where a tab is a crude column separator rather than a + /// layout instruction — actually needs. A line of a\tb and a line of aaaa\tb therefore + /// do not align, and that is the accepted cost of not tracking a column. + /// + /// + /// Applied per line rather than at parse time, beside , so changing the + /// setting takes effect on the next line instead of on the next restart — and so the parser stays + /// ignorant of user preferences. Lines already in scrollback keep the width they were expanded at. + /// + /// + public static StyledLine ExpandTabs(StyledLine line, int width) + { + ArgumentNullException.ThrowIfNull(line); + + if (width < 0) + { + throw new ArgumentOutOfRangeException(nameof(width), width, "A tab cannot be a negative number of spaces."); + } + + var hasTab = false; + foreach (var span in line.Spans) + { + if (span.Text.Contains('\t', StringComparison.Ordinal)) + { + hasTab = true; + break; + } + } + + // The overwhelmingly common case: no tab, so the line is returned as it stands rather than + // rebuilt. Every line of output passes through here. + if (!hasTab) + { + return line; + } + + var replacement = new string(' ', width); + var spans = new List(line.Spans.Count); + foreach (var span in line.Spans) + { + spans.Add(new StyledSpan( + span.Text.Replace("\t", replacement, StringComparison.Ordinal), + span.Style, + span.Interaction)); + } + + return new StyledLine(spans, line.RuleColor); + } + /// Rebuilds a line from a plain string and a parallel per-character style array. public static StyledLine Coalesce(string text, TextStyle[] styles) { diff --git a/src/SharpMUTerm.Tui/OptionsScreenRenderer.cs b/src/SharpMUTerm.Tui/OptionsScreenRenderer.cs index 50f01a6..8902322 100644 --- a/src/SharpMUTerm.Tui/OptionsScreenRenderer.cs +++ b/src/SharpMUTerm.Tui/OptionsScreenRenderer.cs @@ -266,6 +266,16 @@ internal static OptionsScreen TextAnsiScreen(TextSettings? text = null) new("underline hyperlinks", null, settings.UnderlineHyperlinks, null, ScreenToggle.Bind(() => settings.UnderlineHyperlinks, v => settings.UnderlineHyperlinks = v)), new(string.Empty, null, null), + new("├ WHITESPACE", null, null), + + // A count, not a checkbox, and its own section rather than an extra row under COLOUR: it is + // the only thing on this screen that changes how wide a line *is* rather than how it looks. + // Zero is a real answer — it deletes tabs — so the field's floor is 0, not 1. + new("tab width (spaces)", settings.TabWidth.ToString(CultureInfo.InvariantCulture), null, null, null, + ScreenField.Integer( + "tab width (spaces)", () => settings.TabWidth, v => settings.TabWidth = v, + 0, TextSettings.MaxTabWidth)), + new(string.Empty, null, null), new("├ UNICODE", null, null), new("emoji substitution", null, settings.EmojiSubstitution, null, ScreenToggle.Bind(() => settings.EmojiSubstitution, v => settings.EmojiSubstitution = v)), diff --git a/tests/SharpMUTerm.Core.Tests/Session/WorldSessionPreferenceTests.cs b/tests/SharpMUTerm.Core.Tests/Session/WorldSessionPreferenceTests.cs index b6cd4b2..a1e0d86 100644 --- a/tests/SharpMUTerm.Core.Tests/Session/WorldSessionPreferenceTests.cs +++ b/tests/SharpMUTerm.Core.Tests/Session/WorldSessionPreferenceTests.cs @@ -32,6 +32,88 @@ private static (WorldSession Session, FakeTelnetSession Telnet) Create( return (session, telnet); } + // ---- F7: tab width ---- + + /// + /// A tab from the server reaches scrollback as spaces. Asserted end to end rather than on + /// ExpandTabs alone, because the unit is only worth anything if the session actually calls it. + /// + [Test] + public async Task TabWidth_ExpandsATabInServerOutput() + { + var (session, telnet) = Create(World(), new TextSettings()); + await session.ConnectAsync(); + + telnet.EmitLine("name\tvalue"); + + var line = session.Scrollback.Snapshot().First(l => l.Text.StartsWith("name", StringComparison.Ordinal)); + await Assert.That(line.Text).IsEqualTo("name value"); + await Assert.That(line.Text).DoesNotContain("\t"); + } + + /// + /// Live, like every other preference here: the width is read per line, so a session already + /// connected picks up the change on its next line rather than on a reconnect. + /// + [Test] + public async Task TabWidth_TakesEffectOnTheNextLine_NotTheNextSession() + { + var text = new TextSettings { TabWidth = 2 }; + var (session, telnet) = Create(World(), text); + await session.ConnectAsync(); + + telnet.EmitLine("a\tb"); + await Assert.That(session.Scrollback.Snapshot().First(l => l.Text.StartsWith('a')).Text).IsEqualTo("a b"); + + text.TabWidth = 8; + telnet.EmitLine("c\td"); + await Assert.That(session.Scrollback.Snapshot().First(l => l.Text.StartsWith('c')).Text).IsEqualTo("c d"); + } + + /// + /// config.json is hand-edited, so a nonsense width must not reach ExpandTabs and throw + /// out of the read loop on an ordinary line of output. It is clamped at the point of use. + /// + [Test] + [Arguments(-5, "ab")] + [Arguments(0, "ab")] + [Arguments(999, "a b")] + public async Task TabWidth_OutOfRange_IsClampedRatherThanThrowing(int width, string expected) + { + var (session, telnet) = Create(World(), new TextSettings { TabWidth = width }); + await session.ConnectAsync(); + + telnet.EmitLine("a\tb"); + + await Assert.That(session.Scrollback.Snapshot().First(l => l.Text.StartsWith('a')).Text).IsEqualTo(expected); + } + + /// + /// The expansion runs before the trigger engine, so a pattern matches the spaces the reader sees + /// rather than a tab they have no way to know is there. + /// + [Test] + public async Task TabWidth_ExpandsBeforeTriggersSeeTheLine() + { + var set = new TriggerSet + { + Name = "t", + Triggers = + { + new Trigger { Pattern = "name value", Actions = new TriggerActions { Gag = true } }, + }, + }; + + var (session, telnet) = Create(World(), new TextSettings(), set: set); + await session.ConnectAsync(); + + telnet.EmitLine("name\tvalue"); + + await Assert.That(session.Scrollback.Snapshot().Any(l => l.Text.Contains("value", StringComparison.Ordinal))) + .IsFalse() + .Because("the rule matched the expanded text, so the line was gagged"); + } + // ---- F7: strip incoming ANSI colour ---- [Test] diff --git a/tests/SharpMUTerm.Core.Tests/Telnet/LineTerminatorTests.cs b/tests/SharpMUTerm.Core.Tests/Telnet/LineTerminatorTests.cs new file mode 100644 index 0000000..055971e --- /dev/null +++ b/tests/SharpMUTerm.Core.Tests/Telnet/LineTerminatorTests.cs @@ -0,0 +1,91 @@ +using System.Text; +using Microsoft.Extensions.Logging.Abstractions; +using SharpMUTerm.Core.Configuration; +using SharpMUTerm.Core.Session; +using SharpMUTerm.Core.Telnet; + +namespace SharpMUTerm.Core.Tests.Telnet; + +/// +/// One line sent is one line on the wire. +/// +/// used to append \r\n to the text before handing it to +/// TelnetInterpreter.SendAsync — which is itself a line send and appends the terminator. +/// Every line this client sent therefore went out as <text>\r\n\r\n: a spurious empty command +/// after every real one, on every connection, for every typed command, every macro, every timer and the +/// login line. +/// +/// +/// It was visible and had been misread as the server's doing. A MU* connect screen treats the empty +/// line as a command like any other, and a server that redisplays its banner for an unrecognised one did so +/// on every connect — which is why the welcome screen appears twice in the session transcripts before a +/// login that in fact succeeded. +/// +/// +/// These assert on the bytes the transport received rather than on a return value, because +/// returns as soon as the send is handed off: "it sent" can be +/// true while nothing correct reached the wire. +/// +/// +public class LineTerminatorTests +{ + private const string Line = "connect Mannaz hunter2"; + + /// Everything written after the opening negotiation, decoded. + private static async Task WireAfter(Func act) + { + var transport = new ScriptedTransport(); + await using var session = new TelnetSession(transport, NullLogger.Instance); + await session.ConnectAsync(); + await act(session); + + // The send is handed to the interpreter, which writes on its own path; give it a moment to land. + for (var i = 0; i < 50 && !Encoding.ASCII.GetString(transport.Sent).Contains(Line, StringComparison.Ordinal); i++) + { + await Task.Delay(20); + } + + return Encoding.ASCII.GetString(transport.Sent); + } + + [Test] + public async Task ALineIsTerminatedOnceAndNotTwice() + { + var wire = await WireAfter(session => session.SendLineAsync(Line).AsTask()); + + await Assert.That(wire).EndsWith(Line + "\r\n") + .Because("the interpreter appends the terminator; adding a second one sends an empty command after it"); + await Assert.That(wire).DoesNotContain(Line + "\r\n\r\n"); + } + + /// + /// And the same at the seam that matters most: the login line, which is the one send a user cannot + /// see, cannot retype, and is sent at a connect screen where a stray empty line is a command. + /// + [Test] + public async Task TheLoginLineIsTerminatedOnceAndNotTwice() + { + var transport = new ScriptedTransport(); + var world = new WorldDefinition { Name = "Convergence MUSH", Host = "game.example.org", Port = 10000 }; + var character = new CharacterDefinition { Name = "Mannaz", Password = "hunter2" }; + world.Characters.Add(character); + + await using var session = new WorldSession( + world, + character, + sessionFactory: _ => new TelnetSession(transport, NullLogger.Instance)); + + await session.ConnectAsync(); + + const string login = "connect Mannaz hunter2"; + for (var i = 0; i < 50 && !Encoding.ASCII.GetString(transport.Sent).Contains(login, StringComparison.Ordinal); i++) + { + await Task.Delay(20); + } + + var wire = Encoding.ASCII.GetString(transport.Sent); + await Assert.That(wire).Contains(login + "\r\n"); + await Assert.That(wire).DoesNotContain(login + "\r\n\r\n") + .Because("an empty line after the login is a command the connect screen answers, and it made every server redisplay its banner"); + } +} diff --git a/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs b/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs new file mode 100644 index 0000000..c7d774d --- /dev/null +++ b/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs @@ -0,0 +1,118 @@ +using SharpMUTerm.Core.Configuration; +using SharpMUTerm.Core.Text; + +namespace SharpMUTerm.Core.Tests.Text; + +/// +/// A tab in server output is drawn as spaces. It used to travel the whole pipeline as a single +/// character, so everything that measures a line counted it as one cell while the terminal painted it as +/// a jump to the next tab stop — the layout computed against a width the screen does not use, which is +/// the same defect class as chrome that grows on wire data. +/// +public class TabExpansionTests +{ + private static StyledLine Line(string text) => StyledLine.FromText(text, TextStyle.Default); + + /// The claim, and the measurement that motivates it. + [Test] + public async Task ATabBecomesSpacesAndTheLineMeasuresWhatItPaints() + { + var raw = Line("a\tb"); + await Assert.That(raw.Text.Length).IsEqualTo(3).Because("this is the bug: three cells claimed"); + + var expanded = StyledText.ExpandTabs(raw, 4); + + await Assert.That(expanded.Text).IsEqualTo("a b"); + await Assert.That(expanded.Text).DoesNotContain("\t"); + await Assert.That(expanded.Text.Length).IsEqualTo(6); + } + + [Test] + [Arguments(0, "ab")] + [Arguments(1, "a b")] + [Arguments(2, "a b")] + [Arguments(4, "a b")] + [Arguments(8, "a b")] + public async Task TheWidthIsWhateverItIsSetTo(int width, string expected) + { + await Assert.That(StyledText.ExpandTabs(Line("a\tb"), width).Text).IsEqualTo(expected); + } + + /// + /// Fixed spaces, not tab stops. Real tabbing would align both bs in the same column; this + /// deliberately does not, and the test says so out loud so nobody "fixes" it into stop-tracking. + /// + [Test] + public async Task ItIsNotTabStopAlignment() + { + var shortRun = StyledText.ExpandTabs(Line("a\tb"), 4).Text; + var longRun = StyledText.ExpandTabs(Line("aaaa\tb"), 4).Text; + + await Assert.That(shortRun.IndexOf('b', StringComparison.Ordinal)).IsEqualTo(5); + await Assert.That(longRun.IndexOf('b', StringComparison.Ordinal)) + .IsEqualTo(8) + .Because("a fixed run does not align columns, and is not meant to"); + } + + /// Every tab goes, including runs of them and ones at either end. + [Test] + [Arguments("\tlead", " lead")] + [Arguments("trail\t", "trail ")] + [Arguments("a\t\tb", "a b")] + [Arguments("\t", " ")] + public async Task EveryTabIsReplaced(string input, string expected) + { + await Assert.That(StyledText.ExpandTabs(Line(input), 4).Text).IsEqualTo(expected); + } + + /// + /// Styles and interactions survive. A tab inside a coloured or clickable run must not split it or + /// drop its link — the span is rewritten, not rebuilt from plain text. + /// + [Test] + public async Task StyleAndInteractionSurviveTheSubstitution() + { + var styled = new TextStyle(TerminalColor.FromIndex(11), TerminalColor.Default, TextAttributes.Bold); + var line = new StyledLine([new StyledSpan("a\tb", styled)]); + + var expanded = StyledText.ExpandTabs(line, 4); + + await Assert.That(expanded.Spans.Count).IsEqualTo(1); + await Assert.That(expanded.Spans[0].Text).IsEqualTo("a b"); + await Assert.That(expanded.Spans[0].Style).IsEqualTo(styled); + } + + /// + /// A line with no tab — almost every line — is returned as it stands rather than rebuilt. This runs + /// on every line of output from every connected session. + /// + [Test] + public async Task ALineWithNoTabIsNotRebuilt() + { + var line = Line("nothing to do here"); + + await Assert.That(ReferenceEquals(StyledText.ExpandTabs(line, 4), line)).IsTrue(); + } + + [Test] + public async Task ANegativeWidthIsRefusedRatherThanThrowingSomethingUseless() + { + await Assert.That(() => StyledText.ExpandTabs(Line("a\tb"), -1)) + .Throws(); + } + + /// + /// The default is four, and the ceiling is sixteen — both read through the property rather than + /// off the constants. Comparing a const to a literal is a comparison the compiler folds + /// away (TUnitAssertions0005): it pins nothing, and it warned. Going through + /// pins the two facts that can actually drift — that the property's + /// default is the constant, and what the two constants are worth. + /// + [Test] + public async Task TheDefaultIsFourAndTheCeilingIsSixteen() + { + await Assert.That(new TextSettings().TabWidth).IsEqualTo(TextSettings.DefaultTabWidth); + await Assert.That(new TextSettings().TabWidth).IsEqualTo(4); + await Assert.That(new TextSettings { TabWidth = TextSettings.MaxTabWidth }.TabWidth).IsEqualTo(16); + } +} diff --git a/tests/SharpMUTerm.Tui.Tests/ScreenCursorTests.cs b/tests/SharpMUTerm.Tui.Tests/ScreenCursorTests.cs index e72b2ff..f4196a9 100644 --- a/tests/SharpMUTerm.Tui.Tests/ScreenCursorTests.cs +++ b/tests/SharpMUTerm.Tui.Tests/ScreenCursorTests.cs @@ -112,9 +112,10 @@ public async Task Keypad_BarsTheFocusedBinding() [Test] public async Task Options_BarsTheFocusedOptionAndNeverASectionHeader() { - // Navigable row 3 is "emoji substitution" — the two section headers and the spacer are skipped. + // Navigable row 4 is "emoji substitution" — the three section headers and the two spacers are + // skipped, and "tab width (spaces)" sits at 3 between the colour rows and this one. var screen = OptionsScreenRenderer.TextAnsiScreen(); - var lines = OptionsScreenRenderer.BodyColumn(screen.Rows, new ScreenFocus(0, 3)); + var lines = OptionsScreenRenderer.BodyColumn(screen.Rows, new ScreenFocus(0, 4)); await Assert.That(Barred(lines)).IsEqualTo(1); await Assert.That(lines.Single(l => l.Contains(Bar, StringComparison.Ordinal))).Contains("emoji substitution"); diff --git a/tests/SharpMUTerm.Tui.Tests/ScreenModelTests.cs b/tests/SharpMUTerm.Tui.Tests/ScreenModelTests.cs index c4d569a..b79c600 100644 --- a/tests/SharpMUTerm.Tui.Tests/ScreenModelTests.cs +++ b/tests/SharpMUTerm.Tui.Tests/ScreenModelTests.cs @@ -364,18 +364,18 @@ public async Task Options_NavigableRowsSkipSectionHeadersAndSpacers() var screen = OptionsScreenRenderer.TextAnsiScreen(); var model = OptionsScreenRenderer.Model(screen); - // 7 display rows: 2 section headers + 1 spacer + 4 options. It was 8/5 while F7 still carried - // "ambiguous width"; that row named a measurement policy the framework does not expose, so it - // went, and both the display count and the navigable count drop by exactly the one row. - await Assert.That(screen.Rows.Count).IsEqualTo(7); + // 10 display rows: 3 section headers + 2 spacers + 5 options. It was 7/4 before WHITESPACE and + // its "tab width (spaces)" row, which brought a header and a spacer with it. + await Assert.That(screen.Rows.Count).IsEqualTo(10); await Assert.That(model.PaneCount).IsEqualTo(1); - await Assert.That(model.Sizes[0]).IsEqualTo(4); + await Assert.That(model.Sizes[0]).IsEqualTo(5); } /// - /// F7 is four checkboxes and nothing else now — every row is a toggle, so there is no value row - /// for ⏎ to open. Asserted here rather than only in the renderer test, because it is what makes - /// HasEditableRow false for this screen and so what removes ⏎ edit from its header. + /// F7 is four checkboxes and one count. The count is tab width (spaces), and it is what puts + /// ⏎ edit back in this screen's header — HasEditableRow was false for as long as every + /// row here was a toggle. Asserted here rather than only in the renderer test, because the header + /// advertising a key the screen has no use for is exactly the rule these screens are held to. /// [Test] public async Task Options_TextAnsiRowsWriteBackToTheTextSettings() @@ -389,10 +389,13 @@ public async Task Options_TextAnsiRowsWriteBackToTheTextSettings() model.ToggleAt(0, 2)!.Value.Flip(); await Assert.That(text.UnderlineHyperlinks).IsFalse(); - model.ToggleAt(0, 3)!.Value.Flip(); + model.ToggleAt(0, 4)!.Value.Flip(); await Assert.That(text.EmojiSubstitution).IsFalse(); - await Assert.That(model.HasEditableRow).IsFalse(); + // Row 3 is the tab width — a count, so it is a field rather than a toggle, and it is what makes + // this screen carry an editable row at all. + await Assert.That(model.ToggleAt(0, 3)).IsNull(); + await Assert.That(model.HasEditableRow).IsTrue(); } [Test]