From 0b11113c72aba12d6162c6ce877b99fdd8c479c7 Mon Sep 17 00:00:00 2001 From: Harry Cordewener Date: Thu, 30 Jul 2026 21:36:55 -0500 Subject: [PATCH 1/3] Draw a tab in server output as spaces, four by default A tab travelled the whole pipeline as one character, so the wrap, the pane's width and MarkupWidth all 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. Measured: 'a\tb' claimed three cells. TextSettings.TabWidth, default 4, clamped 0-16 at the point of use rather than in the setter, because config.json is hand-edited and a negative number would otherwise throw out of the read loop on an ordinary line. Fixed spaces, not tab stops. A real tab's width depends on the column it starts in; tracking that buys alignment nobody asked for on output where a tab is a crude separator. A test says so out loud so it is not later 'fixed' into stop-tracking. Applied beside StripIncomingColour and before the triggers, so a pattern matches the spaces the reader sees rather than a tab they cannot know is there, and so a width change takes effect on the next line rather than the next restart. A line with no tab is returned unrebuilt - this runs on every line from every session. --- .../Configuration/PreferenceSettings.cs | 25 ++++ src/SharpMUTerm.Core/Session/WorldSession.cs | 15 +++ src/SharpMUTerm.Core/Text/StyledText.cs | 62 ++++++++++ .../Session/WorldSessionPreferenceTests.cs | 82 +++++++++++++ .../Text/TabExpansionTests.cs | 112 ++++++++++++++++++ 5 files changed, 296 insertions(+) create mode 100644 tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs 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/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/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/Text/TabExpansionTests.cs b/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs new file mode 100644 index 0000000..ac43367 --- /dev/null +++ b/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs @@ -0,0 +1,112 @@ +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 stated rather than implied. + [Test] + public async Task TheDefaultIsFour() + { + await Assert.That(new TextSettings().TabWidth).IsEqualTo(4); + await Assert.That(TextSettings.DefaultTabWidth).IsEqualTo(4); + await Assert.That(TextSettings.MaxTabWidth).IsEqualTo(16); + } +} From c6fcd18ceff108498ad9cd4e07c4cc8307f32747 Mon Sep 17 00:00:00 2001 From: Harry Cordewener Date: Thu, 30 Jul 2026 21:42:10 -0500 Subject: [PATCH 2/3] Give F7 a tab width row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setting had no way to reach it from the UI. It is a count rather than a checkbox and it is the only thing on this screen that changes how wide a line *is* rather than how it looks, so it gets its own WHITESPACE section instead of a fifth row under COLOUR. Floor 0, not 1: zero is a real answer, it deletes tabs. Three tests moved with it. All three addressed rows positionally - the F7 screen is four checkboxes, so emoji was navigable row 3 and HasEditableRow was false. Emoji is row 4 now and the screen has an editable row, which puts '⏎ edit' back in its header. The doc comment asserting the old shape was corrected rather than left to mislead. --- src/SharpMUTerm.Tui/OptionsScreenRenderer.cs | 10 ++++++++ .../ScreenCursorTests.cs | 5 ++-- .../SharpMUTerm.Tui.Tests/ScreenModelTests.cs | 23 +++++++++++-------- 3 files changed, 26 insertions(+), 12 deletions(-) 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.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] From cb6ad7b40d9b1edda87618b6c9d355cc1cea49c4 Mon Sep 17 00:00:00 2001 From: Harry Cordewener Date: Thu, 30 Jul 2026 22:48:05 -0500 Subject: [PATCH 3/3] Send one line terminator, not two TelnetSession.SendLineAsync appended \r\n to the text and then handed it to TelnetInterpreter.SendAsync, which is itself a line send and appends the terminator. Every line this client has ever sent went out as \r\n\r\n: a spurious empty command after every real one - each typed command, each macro, each timer, and the login line. It was visible and had been read 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. Measured, not reasoned about: LineTerminatorTests asserts on the bytes the transport received, at both seams - SendLineAsync directly, and the login line through WorldSession, which is the one send a user cannot see or retype. Both fail on main and pass here. Identical on TelnetNegotiationCore 2.6.5 and 2.7.0, so this predates the 2.7.0 bump rather than coming from it. Also fixes the two TUnitAssertions0005 warnings in TabExpansionTests: a const compared to a literal is folded away and pins nothing. The values are now read through TextSettings, which pins the fact that can drift. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM --- src/SharpMUTerm.Core/Telnet/TelnetSession.cs | 18 +++- .../Telnet/LineTerminatorTests.cs | 91 +++++++++++++++++++ .../Text/TabExpansionTests.cs | 14 ++- 3 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 tests/SharpMUTerm.Core.Tests/Telnet/LineTerminatorTests.cs 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/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 index ac43367..c7d774d 100644 --- a/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs +++ b/tests/SharpMUTerm.Core.Tests/Text/TabExpansionTests.cs @@ -101,12 +101,18 @@ await Assert.That(() => StyledText.ExpandTabs(Line("a\tb"), -1)) .Throws(); } - /// The default is four, and the ceiling is stated rather than implied. + /// + /// 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 TheDefaultIsFour() + public async Task TheDefaultIsFourAndTheCeilingIsSixteen() { + await Assert.That(new TextSettings().TabWidth).IsEqualTo(TextSettings.DefaultTabWidth); await Assert.That(new TextSettings().TabWidth).IsEqualTo(4); - await Assert.That(TextSettings.DefaultTabWidth).IsEqualTo(4); - await Assert.That(TextSettings.MaxTabWidth).IsEqualTo(16); + await Assert.That(new TextSettings { TabWidth = TextSettings.MaxTabWidth }.TabWidth).IsEqualTo(16); } }