Send one line terminator, not two

src/SharpMUTerm.Core/Configuration/PreferenceSettings.cs

@@ -32,6 +32,31 @@ public sealed class TextSettings
     /// <summary>Honour the blink attribute rather than dropping it.</summary>
     public bool AllowBlink { get; set; }
 
+    /// <summary>
+    /// How many spaces a tab in server output is drawn as. Zero removes tabs entirely.
+    /// <para>
+    /// A tab used to travel the pipeline as one character, so the wrap, the pane width and
+    /// <c>MarkupWidth</c> all counted it as <b>one cell</b> 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. <c>StyledText.ExpandTabs</c> substitutes the spaces before anything measures the line.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// </summary>
+    public int TabWidth { get; set; } = DefaultTabWidth;
+
+    /// <summary>Four, the width a tab is written to mean in most MU* output and most editors.</summary>
+    public const int DefaultTabWidth = 4;
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    public const int MaxTabWidth = 16;
+
     /// <summary>Underline MXP/Pueblo/web links so they read as clickable.</summary>
     public bool UnderlineHyperlinks { get; set; } = true;
 

src/SharpMUTerm.Core/Session/WorldSession.cs

@@ -324,6 +324,16 @@ private void OnOutputReceived(object? sender, TelnetOutputEventArgs e)
         }
     }
 
+    /// <summary>
+    /// The configured tab width, clamped into range. Clamped at the point of use rather than in the
+    /// setter because <c>config.json</c> is hand-edited: a negative number there would otherwise throw
+    /// out of <see cref="StyledText.ExpandTabs"/> on the read loop, on a line of ordinary output.
+    /// </summary>
+    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)

src/SharpMUTerm.Core/Telnet/TelnetSession.cs

@@ -681,10 +681,26 @@ private async Task ReadLoopAsync(CancellationToken cancellationToken)
         }
     }
 
+    /// <summary>
+    /// Sends one line to the world.
+    /// <para>
+    /// <b>The terminator is the library's, not ours.</b> <c>TelnetInterpreter.SendAsync</c> is a
+    /// <em>line</em> send: it appends the line ending itself. Adding <c>\r\n</c> here as well put
+    /// <c>&lt;text&gt;\r\n\r\n</c> 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.
+    /// </para>
+    /// <para>
+    /// It is bytes rather than a string because the encoding is this session's decision and not the
+    /// interpreter's (see <see cref="CurrentEncoding"/>), and it goes through <see cref="SendAsync"/> so
+    /// the library still escapes a literal <c>IAC</c> in user text as data.
+    /// </para>
+    /// </summary>
     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);
     }
 

src/SharpMUTerm.Core/Text/StyledText.cs

@@ -100,6 +100,68 @@ public static StyledLine StripColour(StyledLine line)
         return new StyledLine(spans, line.RuleColor);
     }
 
+    /// <summary>
+    /// Replaces every tab in <paramref name="line"/> with <paramref name="width"/> spaces, keeping each
+    /// run's style and interaction.
+    /// <para>
+    /// 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, <c>MarkupWidth</c> — counts it as <b>one cell</b>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// This is <em>not</em> 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 <c>a\tb</c> and a line of <c>aaaa\tb</c> therefore
+    /// do not align, and that is the accepted cost of not tracking a column.
+    /// </para>
+    /// <para>
+    /// Applied per line rather than at parse time, beside <see cref="StripColour"/>, 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.
+    /// </para>
+    /// </summary>
+    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<StyledSpan>(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);
+    }
+
     /// <summary>Rebuilds a line from a plain string and a parallel per-character style array.</summary>
     public static StyledLine Coalesce(string text, TextStyle[] styles)
     {

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)),

tests/SharpMUTerm.Core.Tests/Session/WorldSessionPreferenceTests.cs

@@ -32,6 +32,88 @@ private static (WorldSession Session, FakeTelnetSession Telnet) Create(
         return (session, telnet);
     }
 
+    // ---- F7: tab width ----
+
+    /// <summary>
+    /// A tab from the server reaches scrollback as spaces. Asserted end to end rather than on
+    /// <c>ExpandTabs</c> alone, because the unit is only worth anything if the session actually calls it.
+    /// </summary>
+    [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");
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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");
+    }
+
+    /// <summary>
+    /// <c>config.json</c> is hand-edited, so a nonsense width must not reach <c>ExpandTabs</c> and throw
+    /// out of the read loop on an ordinary line of output. It is clamped at the point of use.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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]

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;
+
+/// <summary>
+/// One line sent is one line on the wire.
+/// <para>
+/// <see cref="TelnetSession.SendLineAsync"/> used to append <c>\r\n</c> to the text before handing it to
+/// <c>TelnetInterpreter.SendAsync</c> — which is itself a <em>line</em> send and appends the terminator.
+/// Every line this client sent therefore went out as <c>&lt;text&gt;\r\n\r\n</c>: a spurious empty command
+/// after every real one, on every connection, for every typed command, every macro, every timer and the
+/// login line.
+/// </para>
+/// <para>
+/// <b>It was visible and had been misread as the server's doing.</b> 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.
+/// </para>
+/// <para>
+/// These assert on the <em>bytes the transport received</em> rather than on a return value, because
+/// <see cref="TelnetSession.SendLineAsync"/> returns as soon as the send is handed off: "it sent" can be
+/// true while nothing correct reached the wire.
+/// </para>
+/// </summary>
+public class LineTerminatorTests
+{
+    private const string Line = "connect Mannaz hunter2";
+
+    /// <summary>Everything written after the opening negotiation, decoded.</summary>
+    private static async Task<string> WireAfter(Func<TelnetSession, Task> 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");
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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");
+    }
+}