Skip to content

Show which pane and which command line are selected, plus Ctrl+arrow navigation and a real Alt+⏎ newline - #9

Merged
HarryCordewener merged 1 commit into
mainfrom
feat/focus-and-pane-nav
Jul 30, 2026
Merged

Show which pane and which command line are selected, plus Ctrl+arrow navigation and a real Alt+⏎ newline#9
HarryCordewener merged 1 commit into
mainfrom
feat/focus-and-pane-nav

Conversation

@HarryCordewener

@HarryCordewener HarryCordewener commented Jul 30, 2026

Copy link
Copy Markdown
Member

Three reports from the maintainer, all in the input area and pane chrome, so one change.

1. Make it obvious which input bar and which pane is selected

"When you are on the input window it should make it easier to see which input window you have selected... It should be super obvious."

The armed band was #33394c and the idle one #262b3a — thirteen points per channel. And nothing rendered pane focus at all: Layout.FocusedPane drove the scrollback keys, the ⌃B commands, ⌃F and freeze, and the one pane every keystroke was aimed at looked exactly like the ones it was not.

Both bands are now theme-derived through WorkspacePalette, whose new constants are measured off a ScreenPalette pair the way its existing ones are — the focus step is CursorBg ÷ EditBg, the tone the settings screens already use to mean "the keyboard is here". The armed band takes that step and leans toward Theme.Prompt, so the pair differs in hue as well as brightness: more than double the luma step that was reported as too close, and still distinct after xterm-256 quantisation on every shipped theme.

The focused pane gets three cues, and spends no cells on any of them:

cue survives
its own plane (WorkspacePalette.Focus) colour-blindness (luminance)
its active tab's chip — the same band the armed command line is painted in
a in the tab title a monochrome terminal

Cells matter here: per-pane NAWS is derived from the pane rectangle, so a border or marker column would announce a new terminal size to every connected server on every focus change and reflow the game's own output.

2. Ctrl+arrows move between panes — and into the command lines

"There should be some easy way to switch between input / panes that isn't just using the launch-command. CTRL-left/right/up/down?"

Directional and tmux-style, answered from the arranged pane rectangles by a new pure Core.Workspaces.PaneNavigation — geometry rather than a tree walk, because "the pane to my left" is a question about what is on screen. Vertically the panes and the bars are one ladder: ⌃↓ off the last pane arms the second command line, ⌃↑ leaves it. No neighbour reports on the status row rather than doing nothing.

It moves pane selection, never keyboard focus, so the focus pin is untouched — typing still lands in the command line from wherever you have navigated to, which is why "move into the pane" needs no third piece of state.

Precedence (in HandleWindowKey): after DispatchMacro, so MacroKeys.Verdict reporting a macro on Ctrl+Left as live stays true; before the scrollback keys, history recall and the command line, which would otherwise eat them (TryRecallKey ignores modifiers — the same thing that once swallowed Shift+↑).

Word movement moves from Ctrl+←/→ to Alt+←/→: respelt, not dropped, and now advertised where it never was.

3. A newline chord that exists and can be found

"I still need a way to add a newline in the input window by using Shift-enter or ctrl-enter or something."
…and then: "^L is not good enough. It need to be <a modifier key> + ENTER"

⌃L already worked and was documented only in a code comment. The chord asked for is modifier+Enter, so Alt+⏎ is now it: the parser emits ESC followed by a control byte as two key events, so TryAltEnter pairs the Escape and Enter back together inside the framework's own 50 ms ESC window. Safe because Escape in the command line is a genuine no-op and every other meaning of Escape is handled earlier; reliable because a terminal writes ESC CR in one write, so both halves land in one read, one parse and one dispatch batch — the observed gap is microseconds.

Shift+⏎ and Ctrl+⏎ are advertised nowhere, and that is deliberate: this terminal reports both as a bare Enter, so a hint naming them would point at the send key. They stay accepted for the Windows Console.ReadKey path — accepting is not advertising. Properly distinguishing them needs the Kitty keyboard protocol, which cannot be done consumer-side; the findings and the upstream shape are written up in CLAUDE.md.

Discoverability

--help, the ⌃P surface (four directional entries, a newline entry, and the ⌃B chords — which were never advertised either), and a contextual status hint that appears only when the keys can move something. Held to the honesty rule in both directions by AdvertisedKeyHonestyTests.

Also fixed on the way past

  • PaneCommands' doc claimed to be the whole ⌃B keymap; b, m and i live in the app.
  • TabTitles' doc promised a focused-pane border that never existed.
  • The status row's right cluster could overflow its width and wrap the sticky band, costing a row of output.

Verification

Release build clean (2 pre-existing AngleSharp NU1902 from the local SharpConsoleUI clone).

suite before after
Core 545 562
Graphics 83 83
Scripting 42 42
Web 30 37
Tui 937 1039

Tui run three times — no flakes. New focus/focus-moved snapshot views render the split-plus-two-bars geometry at 100×28 and 160×48; frames rendered, decoded cell by cell, and looked at.

The test worth keeping above the others is MovingFocusDoesNotMoveAnyPaneRectangle: every pane's output rectangle is identical before and after a focus move, at four sizes, and no connected session is told a new NAWS size. That is what stops the indicator being "improved" into a border later.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

Summary by CodeRabbit

  • New Features
    • Added directional pane focus navigation with a dedicated focused-pane indicator.
    • Added layout focus actions (focus left/right/up/down) and focus cycling.
    • Updated Enter/newline handling: Ctrl/Shift/Alt+Enter inserts a newline; plain Enter sends.
    • Moved word navigation to Alt+Left/Right.
  • Documentation
    • Refreshed in-app help and shortcut listings to match current bindings, including focus and typing sections.
  • Bug Fixes
    • Improved focus visuals and theme-aware colors for focused panes and active/armed command lines, without disturbing pane geometry.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e7aed6c3-6f8f-4f84-92b6-9ddb36ee3a7e

📥 Commits

Reviewing files that changed from the base of the PR and between 55e7624 and 9096824.

📒 Files selected for processing (14)
  • CLAUDE.md
  • src/SharpMUTerm.Core/Commands/CommandCatalog.cs
  • src/SharpMUTerm.Core/Workspace/PaneCommands.cs
  • src/SharpMUTerm.Core/Workspace/PaneNavigation.cs
  • src/SharpMUTerm.Tui/Glyphs.cs
  • src/SharpMUTerm.Tui/InputBarControl.cs
  • src/SharpMUTerm.Tui/Program.cs
  • src/SharpMUTerm.Tui/SharpMUTermApp.cs
  • src/SharpMUTerm.Tui/TabTitles.cs
  • src/SharpMUTerm.Tui/WorkspacePalette.cs
  • tests/SharpMUTerm.Core.Tests/Workspace/PaneNavigationTests.cs
  • tests/SharpMUTerm.Tui.Tests/AdvertisedKeyHonestyTests.cs
  • tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs
  • tests/SharpMUTerm.Tui.Tests/WorkspacePaletteTests.cs

Walkthrough

The change adds directional pane focus navigation, theme-derived focus and command-line styling, focus markers, modifier-aware input behavior, new layout commands, generated usage text, and tests covering rendering, terminal sizing, key routing, and advertised shortcut accuracy.

Changes

Pane focus and keyboard interaction

Layer / File(s) Summary
Directional pane navigation
src/SharpMUTerm.Core/Workspace/PaneNavigation.cs, tests/SharpMUTerm.Core.Tests/Workspace/PaneNavigationTests.cs
Adds directional neighbor resolution using pane geometry and deterministic candidate scoring, with coverage for splits, nesting, zoom, collapsed panes, edges, and ties.
Focus-plane rendering and palette
src/SharpMUTerm.Tui/WorkspacePalette.cs, src/SharpMUTerm.Tui/SharpMUTermApp.cs, src/SharpMUTerm.Tui/TabTitles.cs, src/SharpMUTerm.Tui/Glyphs.cs, tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs, tests/SharpMUTerm.Tui.Tests/WorkspacePaletteTests.cs
Adds theme-derived focus and command-line colors, cached pane repainting, focused tab markers, internal rendering introspection, and tests for visual separation and unchanged pane geometry.
Input routing and command integration
src/SharpMUTerm.Tui/InputBarControl.cs, src/SharpMUTerm.Tui/SharpMUTermApp.cs, src/SharpMUTerm.Core/Commands/CommandCatalog.cs, src/SharpMUTerm.Core/Workspace/PaneCommands.cs, src/SharpMUTerm.Tui/Program.cs, tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs
Moves word navigation to Alt+arrows, supports modifier-based newline insertion and ESC+Enter reconstruction, adds focus and cycle commands, updates status and snapshot flows, and generates expanded usage text.
Advertised key contracts and documentation
CLAUDE.md, tests/SharpMUTerm.Tui.Tests/AdvertisedKeyHonestyTests.cs
Documents terminal input constraints and verifies that help, catalogs, headers, status hints, macros, and advertised chords match delivered behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Terminal
  participant SharpMUTermApp
  participant PaneNavigation
  participant InputBarControl
  participant WorkspaceRenderer

  Terminal->>SharpMUTermApp: Ctrl+arrow or ESC+Enter
  SharpMUTermApp->>PaneNavigation: resolve directional neighbour
  PaneNavigation-->>SharpMUTermApp: pane id or refusal
  SharpMUTermApp->>WorkspaceRenderer: refresh focus plane and tab marker
  SharpMUTermApp->>InputBarControl: route newline or preserve armed bar
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main visible focus, pane navigation, and Alt+Enter newline changes in the PR.
Docstring Coverage ✅ Passed Docstring coverage is 85.84% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

…ow navigation

Three reports, all in the input area and pane chrome, so one change.

**1. Make the selection obvious.** The armed input band was #33394c and the idle
one #262b3a — thirteen points per channel, which is genuinely almost invisible.
And nothing rendered pane focus at all: the one pane every workspace key was
aimed at looked exactly like the ones it was not.

Both bands now come from the theme through WorkspacePalette, whose new constants
are derived from a ScreenPalette pair the way its existing ones are — the focus
step is CursorBg ÷ EditBg, the tone the settings screens already use to say "the
keyboard is here". The armed band takes that step and leans toward Theme.Prompt,
so the pair differs in hue as well as brightness: more than double the luma step
that was reported as too close, and distinct after xterm-256 quantisation on
every shipped theme (truecolor is not guaranteed).

The focused pane gets three cues and spends no cells on any of them: its own
plane, its active tab's chip colour (the same band the armed command line is
painted in — one colour means "you are here" in both places), and a ▌ in the tab
title, which is what carries the signal on a monochrome terminal. Cells matter
here: per-pane NAWS is derived from the pane rectangle, so a border or a marker
column would announce a new terminal size to every connected server on every
focus change and reflow the game's own output.

**2. Ctrl+arrows move between panes, and into the command lines.** Directional,
tmux-style, answered from the arranged pane rectangles by a new pure
Core.Workspaces.PaneNavigation — geometry rather than a tree walk, because "the
pane to my left" is a question about what is on screen. Vertically the panes and
the bars are one ladder: ⌃↓ off the last pane arms the second command line and ⌃↑
leaves it. No neighbour reports on the status row rather than doing nothing.

It moves pane *selection*, never keyboard focus, so the focus pin is untouched
and typing still lands in the command line from wherever you navigated to — which
is why "move into the pane" needs no third piece of state. Routed from
PreviewKeyPressed after DispatchMacro (so MacroKeys.Verdict reporting a macro on
Ctrl+Left as live stays true) and before the scrollback keys, history recall and
the command line, which would otherwise eat them. Word movement moves from
Ctrl+←/→ to Alt+←/→ to make room: respelt, not dropped, and now advertised.

**3. A newline chord that exists and can be found.** ⌃L already worked and was
documented only in a code comment, which is why it was reported as missing. The
chord asked for was modifier+Enter, so Alt+⏎ is now it: the parser emits ESC
followed by a control byte as two key events, so TryAltEnter pairs the Escape and
Enter back together inside the framework's own 50 ms ESC window. Safe because
Escape in the command line is a genuine no-op and every other meaning of Escape
is handled earlier; reliable because a terminal writes ESC CR in one write, so
both halves land in one read, one parse and one dispatch batch.

Shift+⏎ and Ctrl+⏎ are deliberately advertised nowhere: this terminal reports
both as a bare Enter, so a hint naming them would point at the send key. They
stay accepted for the Windows Console.ReadKey path. Properly distinguishing them
needs the Kitty keyboard protocol, which cannot be done consumer-side — the
findings are written up in CLAUDE.md.

All three are discoverable: --help, the ⌃P surface (four directional entries, a
newline entry, and the ⌃B chords which were never advertised either), and a
contextual status hint that appears only when the keys can move something. Held
to the honesty rule in both directions by AdvertisedKeyHonestyTests.

Also fixed on the way past: PaneCommands' doc claimed to be the whole ⌃B keymap
when b, m and i live in the app; TabTitles' doc promised a focused-pane border
that never existed; the status row's right cluster could overflow its width and
wrap the sticky band, costing a row of output.

Verified: Release build clean (2 pre-existing AngleSharp NU1902 from the local
SharpConsoleUI clone). Core 562, Graphics 83, Scripting 42, Web 30, Tui 1000 —
Tui run three times, no flakes. New `focus`/`focus-moved` snapshot views render
the split-plus-two-bars geometry at two sizes; frames read and checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
@HarryCordewener
HarryCordewener force-pushed the feat/focus-and-pane-nav branch from 55e7624 to 9096824 Compare July 30, 2026 05:14
@HarryCordewener
HarryCordewener merged commit 80cc952 into main Jul 30, 2026
1 of 3 checks passed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CLAUDE.md`:
- Around line 230-242: Broaden the input-protocol discussion to state that
Ctrl+Enter and Shift+Enter require a modifier-preserving terminal protocol and
corresponding parser support. Present Kitty as one option, while also mentioning
supported xterm-style modifyOtherKeys/CSI-u modes, and avoid implying Kitty is
the only solution.

In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 4309-4326: Update PaintTabChips so an unfocused pane still
visually distinguishes the active tab from inactive tabs: retain Surface(_theme)
for inactive chips, but use Focus(_theme) or the theme foreground-derived color
for the active chip’s background or foreground. Preserve the existing
focused-pane palette behavior and add a rendered headless snapshot assertion
covering a two-tab unfocused pane.

In `@src/SharpMUTerm.Tui/WorkspacePalette.cs`:
- Line 70: Update the Focus(theme) color-scaling logic around FocusScale and
Channel so bright user-supplied theme backgrounds/status backgrounds use an
inverse scale when the lift would clamp at 255, preserving a visible focus delta
and preventing Focus from collapsing onto Surface or ArmedBand onto IdleBand.
Update Scale’s summary to describe scaling by a factor rather than claiming it
always moves colors toward black, and add coverage for bright inline themes in
WorkspacePaletteTests.

In `@tests/SharpMUTerm.Core.Tests/Workspace/PaneNavigationTests.cs`:
- Around line 113-126: Update the pane geometry in the navigation test around
PaneNavigation.Neighbour so p1’s centre is strictly closer to p2 than p3,
avoiding reliance on the string.CompareOrdinal tie-breaker; use an odd-sized
split such as making p2 height 11 and starting p3 at y 12, then adjust p3’s
height to preserve the layout and keep the existing navigation assertions.

In `@tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs`:
- Around line 685-700: Update AltEnterInsertsANewlineRatherThanSending and its
App setup to use a controllable TimeProvider instead of TimeProvider.System,
advancing it only a few milliseconds between the Escape and Enter simulations so
recognition is deterministic. Add coverage for an Enter occurring beyond
TryAltEnter’s 50 ms window, asserting it sends rather than inserts.
- Around line 109-121: Update the SGR parameter handling in the visible “m” case
to split parameters on semicolons and compare complete tokens, rather than using
substring matching for "0" and "49". Preserve clearing current for empty
parameters or exact reset/background-reset tokens, while continuing to detect
the "48;2;" background sequence without treating unrelated values such as
foreground color components as resets.

In `@tests/SharpMUTerm.Tui.Tests/WorkspacePaletteTests.cs`:
- Around line 246-282: The Xterm256 helper claims full 256-colour quantisation
while omitting indices 0–15. Either add the standard 16 system-colour entries to
the candidates considered by Xterm256, noting their terminal-configurable
nature, or narrow its summary to state that it only models the 6×6×6 cube and
grey ramp.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 31d41cdc-0f83-4918-a391-c36e58f7787a

📥 Commits

Reviewing files that changed from the base of the PR and between 041e3ee and 55e7624.

📒 Files selected for processing (14)
  • CLAUDE.md
  • src/SharpMUTerm.Core/Commands/CommandCatalog.cs
  • src/SharpMUTerm.Core/Workspace/PaneCommands.cs
  • src/SharpMUTerm.Core/Workspace/PaneNavigation.cs
  • src/SharpMUTerm.Tui/Glyphs.cs
  • src/SharpMUTerm.Tui/InputBarControl.cs
  • src/SharpMUTerm.Tui/Program.cs
  • src/SharpMUTerm.Tui/SharpMUTermApp.cs
  • src/SharpMUTerm.Tui/TabTitles.cs
  • src/SharpMUTerm.Tui/WorkspacePalette.cs
  • tests/SharpMUTerm.Core.Tests/Workspace/PaneNavigationTests.cs
  • tests/SharpMUTerm.Tui.Tests/AdvertisedKeyHonestyTests.cs
  • tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs
  • tests/SharpMUTerm.Tui.Tests/WorkspacePaletteTests.cs

Comment thread CLAUDE.md
Comment on lines +230 to +242
spelling. **Getting `Ctrl+⏎`/`Shift+⏎` properly needs the Kitty keyboard protocol, and that cannot
be done consumer-side** — see below.
- **The input stack cannot be extended from here.** Enabling the Kitty keyboard protocol is trivial
(`IConsoleDriver.WriteClipboardOsc52` is a de-facto public raw-escape emitter, and `Start`/`Stop`
already pair `CSI ?2004h`/`l` for bracketed paste). *Decoding* it is the wall: `AnsiInputParser`,
`UnixStdinReader`, `InputEvent` and `TerminalRawMode` are all `internal`; `NetConsoleDriver` has
**zero** virtual members, a private `WriteOutput`, field-like events a subclass cannot raise, and it
constructs its parser and reader as *locals* inside `Start()`. So enabling reporting without a
matching decoder makes the affected keys **vanish silently** (`DispatchCsi`'s `default:` emits
`UnknownSequenceEvent`, which `UnixStdinReader` drops). Owning input means a from-scratch
`IConsoleDriver` (~900–1400 lines re-authoring internal termios + parser logic). The cheap unblock is
upstream: make `AnsiInputParser`/`InputEvent` public and add an `UnknownSequenceHandler` hook, or add
an input-reader factory to `NetConsoleDriverOptions`. ~15 lines there; do not try it from here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== CLAUDE.md around target lines ==\n'
sed -n '220,250p' CLAUDE.md | cat -n

printf '\n== Search for Kitty / modifyOtherKeys / CSI-u mentions ==\n'
rg -n -i 'kitty|modifyOtherKeys|CSI-u|csi-u|xterm' CLAUDE.md . --glob '!**/.git/**'

Repository: SharpMUSH/SharpMUTerm

Length of output: 38530


🌐 Web query:

xterm modifyOtherKeys CSI-u modifier-aware sequences documentation

💡 Result:

xterm's modifyOtherKeys is a resource that allows the terminal to report key combinations that are typically ambiguous or unmappable using standard terminal escape sequences [1]. It addresses the historical limitation where certain modified keys (like Ctrl+I) are indistinguishable from others (like Tab) [1]. Mechanism When enabled, xterm sends "CSI u" encoded sequences for "other" keys—typically printable keys like letters, numbers, and punctuation—when they are modified by keys such as Control, Alt, or Meta [2][3]. The general structure for this encoding is CSI [codepoint]; [modifiers] u [4]. Modes for modifyOtherKeys The behavior is controlled by the modifyOtherKeys resource, which accepts the following values [5]: 0: Disabled [5]. 1: Enabled for most keys, but excludes keys with well-known legacy behaviors (e.g., Tab, Backspace, Esc, and certain X11-defined control sequences like Ctrl+Space or Ctrl+3) [5][3]. 2: Enabled for all keys, including the exceptions listed in mode 1. This mode ignores legacy X11 library special cases and ensures modified keys send distinct sequences [5][3]. 3: (Introduced in xterm patch #398) Extends the reporting to all keys, including unmodified ones, effectively forcing every keypress to send a sequence [6]. Runtime Control (XTMODKEYS) Applications can toggle this behavior at runtime using the XTMODKEYS control sequence [7][8]: CSI > 4; Pv m Where Pv corresponds to the mode (0, 1, 2, or 3) [7][6]. To disable the feature, an application can send CSI > 4; 0 m [7][6]. Relationship to CSI-u and Modern Protocols The "CSI u" encoding was a precursor to more modern keyboard protocols, such as the Kitty keyboard protocol [1][4]. While the Kitty protocol is richer and designed for modern, greenfield applications, modifyOtherKeys and the CSI u encoding remain the de-facto standard for compatibility with many existing terminal user interfaces (TUIs) [7][4]. Official Documentation The definitive technical references for these sequences are: 1. The xterm man page (xterm(1)) [9][5]. 2. The ctlseqs.txt file maintained by the xterm author [8]. 3. The XTerm "Other" Modified Keys FAQ page [2][10].

Citations:


Broaden the input-protocol wording. Ctrl+⏎ / Shift+⏎ need a modifier-preserving terminal protocol plus matching parser support; Kitty is one option, but xterm-style modifyOtherKeys/CSI-u modes are another where supported.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CLAUDE.md` around lines 230 - 242, Broaden the input-protocol discussion to
state that Ctrl+Enter and Shift+Enter require a modifier-preserving terminal
protocol and corresponding parser support. Present Kitty as one option, while
also mentioning supported xterm-style modifyOtherKeys/CSI-u modes, and avoid
implying Kitty is the only solution.

Comment thread src/SharpMUTerm.Tui/SharpMUTermApp.cs
/// colour-blind reader, and a light theme lifts the same way a dark one does.
/// </para>
/// </summary>
private const double FocusScale = 1.595;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

A purely multiplicative lift has no fallback when the base tone is already near white.

FocusScale = 1.595 relies on headroom in every channel; Channel clamps at 255, so a theme whose Background/StatusBackground is bright (a user-supplied inline Theme, not just the shipped three) collapses Focus(theme) onto Surface(theme) and ArmedBand toward IdleBand — losing the focus cue entirely, with no assertion covering it since WorkspacePaletteTests only exercises ThemeLibrary entries. Consider inverting the step (scale down) when the scaled result would clamp, so the lift is always a visible delta in one direction or the other.

Separately, Scale's summary still reads "Scales a colour toward black", which is no longer true for the factors introduced here.

Also applies to: 96-108

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/SharpMUTerm.Tui/WorkspacePalette.cs` at line 70, Update the Focus(theme)
color-scaling logic around FocusScale and Channel so bright user-supplied theme
backgrounds/status backgrounds use an inverse scale when the lift would clamp at
255, preserving a visible focus delta and preventing Focus from collapsing onto
Surface or ArmedBand onto IdleBand. Update Scale’s summary to describe scaling
by a factor rather than claiming it always moves colors toward black, and add
coverage for bright inline themes in WorkspacePaletteTests.

Comment thread tests/SharpMUTerm.Core.Tests/Workspace/PaneNavigationTests.cs
Comment on lines +109 to +121
case "m":
if (parameters.Length == 0 || parameters == "0" || parameters.Contains("49"))
{
current = null;
}

if (parameters.Contains("48;2;"))
{
current = parameters[parameters.IndexOf("48;2;", StringComparison.Ordinal)..];
}

break;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

parameters.Contains("49") matches substrings of unrelated SGR parameters.

A foreground sequence such as \x1b[38;2;49;10;10m contains "49", so current is cleared and — with no 48;2; in the same sequence — the following cells are recorded as having no background. That silently deflates CellsPainted* counts and can mask exactly the regression these tests exist to catch. Split on ; and compare tokens instead.

🧪 Proposed fix
-                    if (parameters.Length == 0 || parameters == "0" || parameters.Contains("49"))
+                    var codes = parameters.Split(';');
+                    if (parameters.Length == 0 || codes.Contains("0") || codes.Contains("49"))
                     {
                         current = null;
                     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case "m":
if (parameters.Length == 0 || parameters == "0" || parameters.Contains("49"))
{
current = null;
}
if (parameters.Contains("48;2;"))
{
current = parameters[parameters.IndexOf("48;2;", StringComparison.Ordinal)..];
}
break;
}
case "m":
var codes = parameters.Split(';');
if (parameters.Length == 0 || codes.Contains("0") || codes.Contains("49"))
{
current = null;
}
if (parameters.Contains("48;2;"))
{
current = parameters[parameters.IndexOf("48;2;", StringComparison.Ordinal)..];
}
break;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs` around lines 109 - 121,
Update the SGR parameter handling in the visible “m” case to split parameters on
semicolons and compare complete tokens, rather than using substring matching for
"0" and "49". Preserve clearing current for empty parameters or exact
reset/background-reset tokens, while continuing to detect the "48;2;" background
sequence without treating unrelated values such as foreground color components
as resets.

Comment on lines +685 to +700
[Test]
public async Task AltEnterInsertsANewlineRatherThanSending()
{
var app = App();
app.RenderSnapshot();
Type(app, "one");

app.SimulateKey(Plain('\x1b', ConsoleKey.Escape));
app.SimulateKey(Plain('\r', ConsoleKey.Enter));
foreach (var c in "two")
{
app.SimulateKey(Plain(c, ConsoleKey.NoName));
}

await Assert.That(app.ArmedInputText).IsEqualTo("one\ntwo");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

This test depends on wall-clock time between two calls.

TryAltEnter compares _time.GetUtcNow() against a 50 ms window, and App() leaves time at TimeProvider.System, so the pairing here is only recognised because the two SimulateKey calls happen to be close together — a GC pause or a loaded CI agent between them turns this green test red. The app already accepts a TimeProvider; passing a controllable one (advancing it by a few ms between the halves) makes the claim exact and would also let you assert the negative case — an Enter after the window sends rather than inserting, which nothing currently covers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cs` around lines 685 - 700,
Update AltEnterInsertsANewlineRatherThanSending and its App setup to use a
controllable TimeProvider instead of TimeProvider.System, advancing it only a
few milliseconds between the Escape and Enter simulations so recognition is
deterministic. Add coverage for an Enter occurring beyond TryAltEnter’s 50 ms
window, asserting it sends rather than inserts.

Comment on lines +246 to +282
/// <summary>
/// The nearest xterm-256 palette entry to a colour — the 6×6×6 cube (16–231) and the 24-step grey
/// ramp (232–255), which is what a 256-colour terminal has to choose from. Nearest by squared
/// distance, which is what every terminal and library that does this uses.
/// </summary>
private static int Xterm256(SharpMUTerm.Core.Text.Rgb rgb)
{
var levels = new[] { 0, 95, 135, 175, 215, 255 };
var best = 0;
var bestDistance = int.MaxValue;

for (var i = 0; i < 216; i++)
{
var r = levels[i / 36];
var g = levels[i / 6 % 6];
var b = levels[i % 6];
Consider(16 + i, r, g, b);
}

for (var i = 0; i < 24; i++)
{
var grey = 8 + (i * 10);
Consider(232 + i, grey, grey, grey);
}

return best;

void Consider(int index, int r, int g, int b)
{
var distance = ((rgb.R - r) * (rgb.R - r)) + ((rgb.G - g) * (rgb.G - g)) + ((rgb.B - b) * (rgb.B - b));
if (distance < bestDistance)
{
bestDistance = distance;
best = index;
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The modelled palette omits the 16 system colours, so the quantisation claim is weaker than it reads.

Real 256-colour terminals also choose from indices 0–15, and for near-black or near-white tones a system colour is frequently the nearest entry — two tones could collapse onto colour 0/8/15 while this helper reports two distinct cube indices, letting FocusSurvivesA256ColourTerminal pass over exactly the collision it exists to rule out. Either include the standard 16 (with the caveat that they are terminal-configurable) or narrow the summary to say the check covers the cube and grey ramp only.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/SharpMUTerm.Tui.Tests/WorkspacePaletteTests.cs` around lines 246 - 282,
The Xterm256 helper claims full 256-colour quantisation while omitting indices
0–15. Either add the standard 16 system-colour entries to the candidates
considered by Xterm256, noting their terminal-configurable nature, or narrow its
summary to state that it only models the 6×6×6 cube and grey ramp.

@HarryCordewener
HarryCordewener deleted the feat/focus-and-pane-nav branch July 31, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant