diff options
| author | user <user@clank> | 2026-08-13 18:46:58 +0200 |
|---|---|---|
| committer | user <user@clank> | 2026-08-13 18:46:58 +0200 |
| commit | 2f0750285b1b174a552113c4aed9288f44985ae3 (patch) | |
| tree | e8033ba0b3d3ac04e4e49fb4fa53f7049ac21499 | |
| parent | docs: handover (diff) | |
| download | sl0ppi-2f0750285b1b174a552113c4aed9288f44985ae3.tar.gz sl0ppi-2f0750285b1b174a552113c4aed9288f44985ae3.tar.xz sl0ppi-2f0750285b1b174a552113c4aed9288f44985ae3.zip | |
sl0p: make the split button visible, and clickable where it is drawn
The `+` shipped in 0.1.7 and 0.1.8 without ever appearing on a live session,
while rendering in 265 snapshots. Four faults, each enough on its own:
- title_line_without_middle computed the right side only inside the left
side's and_then, so a pane with no title (hide_default_pane_names, the live
default) got none at all;
- title_line_with_middle discarded the right side whenever the left was
absent -- that is every frame in a session with a second client, which the
live box always had, and "MY FOCUS AND:" on every frame was saying so;
- the title was budgeted before the button, so a title longer than the frame
(any shell showing a path) consumed its columns. The button is anchored and
hit-tested by pure geometry, so it cannot be the part that loses;
- the click hit-tested position_and_size() while the frame is drawn inset by
the pane gap, putting the clickable columns two right of the glyph and a row
above it at the top of the screen. Hence "the click works" and "nothing is
drawn" both being true, and that click test proving nothing.
Adds Pane::frame_rect(), the rect the frame is really drawn in, which the
status bar buttons had already had to work around by hand.
Also fixes render_my_focus building its short form from separators that
Vec::append had already drained: 4 cells wide while reporting 6, a title line
two cells short of its pane. Found by asserting the width in the new test.
Verification, since unit tests have now been insufficient here four times:
split_button_is_drawn_where_it_is_hit_tested covers the combinations the live
config produces, and packaging/checks/split-button-live.py (make check-live)
reads the glyph off a real session, attaches a second client to force the
middle builder, and clicks the columns it found the glyph in. Both confirmed
to fail on the parent commit.
| -rw-r--r-- | Makefile | 4 | ||||
| -rw-r--r-- | docs/HANDOVER.md | 101 | ||||
| -rw-r--r-- | docs/roadmap.md | 31 | ||||
| -rwxr-xr-x | packaging/checks/split-button-live.py | 242 | ||||
| -rw-r--r-- | sl0ppij/zellij-server/src/panes/plugin_pane.rs | 8 | ||||
| -rw-r--r-- | sl0ppij/zellij-server/src/panes/terminal_pane.rs | 8 | ||||
| -rw-r--r-- | sl0ppij/zellij-server/src/tab/mod.rs | 11 | ||||
| -rw-r--r-- | sl0ppij/zellij-server/src/tab/mouse_handler.rs | 23 | ||||
| -rw-r--r-- | sl0ppij/zellij-server/src/tab/unit/snapshots/zellij_server__tab__tab_integration_tests__ctrl_scroll_up_merging_stacks_preserves_all_panes.snap | 5 | ||||
| -rw-r--r-- | sl0ppij/zellij-server/src/ui/pane_boundaries_frame.rs | 275 |
10 files changed, 615 insertions, 93 deletions
@@ -98,6 +98,10 @@ check: ## Cheap compile gate test: cli-test ## All tests: the CLI, plus the crates we actually modify cd $(FORK) && cargo test -p zellij-server --lib && cargo test -p zellij-utils --lib +.PHONY: check-live +check-live: dev ## Drive a real session and check the mouse affordances are drawn where they are clicked + packaging/checks/split-button-live.py $(FORK)/target/fastdev/$(BIN_NAME) + ##@ Package .PHONY: vendor diff --git a/docs/HANDOVER.md b/docs/HANDOVER.md index 467c29b..6f7f89b 100644 --- a/docs/HANDOVER.md +++ b/docs/HANDOVER.md @@ -2,54 +2,64 @@ Written at the end of a long session. Start here. -## Pick this up first: the `+` split button does not render +## Done: the `+` split button renders (and clicks where it is drawn) **Feature:** a `+` at the right end of every pane's title line; clicking it -splits that pane. Shipped in 0.1.7 (hover-gated) and 0.1.8 (always drawn). +splits that pane. Shipped invisible in 0.1.7 and 0.1.8; fixed after both. -**Status: the click works, the button is invisible on a live session.** +It was never one bug. The title line has three builders, and the button was +falling out of two of them, while the click was hit-testing a rect the frame is +not drawn in: -- clicking those 3 columns splits the pane — verified by injecting real SGR - mouse events into an attached client's pty (`1 -> 2 panes`; clicking the title - or mid-frame correctly does nothing) -- the button renders in **265 unit snapshots** -- it has never once been seen on a real session +1. **No title, no right side.** `title_line_without_middle` only computed the + right side *inside* the left side's `and_then`, so a pane with an empty title + got none. Live sets `hide_default_pane_names true`, so unnamed panes have + exactly that. +2. **A "MY FOCUS" middle dropped the right side entirely.** + `title_line_with_middle` fell through to `middle_only_title_line` whenever + the left side was absent. Any second client in the session puts a middle + there — including the phantom client of open item 1 below, which is why the + live box *always* took this path (`MY FOCUS AND:` on every frame was the + clue, and it was in the bug report all along). +3. **A long title ate the button.** The title was budgeted first and the right + side got the remainder; shell titles are long paths. The anchored button is + now reserved before the title, since its hit-test is pure geometry and cannot + ask the renderer what fitted. +4. **The click hit-tested `position_and_size()`**, the pane's allocation, while + the frame is drawn inset by the pane gap (`gap 1` → 2 columns, 1 row). The + clickable columns sat out in the blank gap, two right of the drawn `+`. This + is why "clicking works" and "nothing is drawn" were both true, and why that + click test proved nothing. There is now `Pane::frame_rect()` — the rect the + frame is actually drawn in — and both the button and (already) the status bar + buttons derive from it. -Two wrong diagnoses so far, both mine: +The centre-aligned path, the strong lead in the previous handover, was fine. -1. "It's hover-gated and hover is broken" → removed the gate in 0.1.8, still - invisible. (`mouse_is_hovering_over_pane` does appear to be false live, but - that was not the only problem.) -2. "The click test proves the feature" → it does not. The hit-test is purely - geometric and never consults the renderer, so it kept working while nothing - was drawn. +**What was missing was the reproduction, not the analysis.** Two things now +exist so this cannot come back: -**The strong lead, untested:** every snapshot uses zellij's DEFAULT frame -config. The live config does not: +- `split_button_is_drawn_where_it_is_hit_tested` — asserts the button occupies + exactly `split_button_columns()` across alignments × rounded × titled/untitled + × one/two clients × 4 widths. Positional, not `contains("+")`: the short + multi-client indication is itself a `+`. +- `packaging/checks/split-button-live.py` (`make check-live`) — spawns a real + session at a pty, reads the glyph off the screen, attaches a second client to + force the middle builder, then clicks the columns it *found the glyph in* and + confirms the split. Verified to fail on the pre-fix commit for both reasons + (button absent with 2 clients; click in the gap does nothing). -```kdl -pane_frames { rounded_corners true; title_alignment "center"; gap 1; padding 0 0 0 0 } -``` - -`render_title_right_side` is called from **three** different title builders -(`zellij-server/src/ui/pane_boundaries_frame.rs`, lines ~889, ~915, ~983) with -different `max_length` budgets. The snapshots only ever exercised the -left-aligned path; `title_alignment "center"` takes another one. - -**Ten-second check before writing any code:** set `title_alignment "left"` in -`~/.config/sl0ppij/config.kdl` on the test box, re-attach, look for the `+`. If -it appears, that is the bug confirmed. - -**Then:** add a frame unit test using the live config's alignment/rounded/gap -settings — the reproduction that was missing all along — and fix whichever -builder drops the button. +One more bug fell out of the width assertion: `render_my_focus` built its full +and short forms from the same separator vectors, and `Vec::append` drains — so +the short "ME" form rendered 4 cells wide while reporting 6, two cells short of +the pane. Fixed. Code involved: | what | where | |---|---| -| render + geometry | `ui/pane_boundaries_frame.rs`: `render_split_button`, `split_button_columns`, `SPLIT_BUTTON_*` | +| render + geometry | `ui/pane_boundaries_frame.rs`: `render_split_button`, `split_button_columns`, `anchored_right_side_len`, `SPLIT_BUTTON_*` | | click | `tab/mouse_handler.rs`: `intercept_split_button_click` | +| where the frame really is | `tab/mod.rs`: `Pane::frame_rect` (+ the two impls) | Not drawn on floating panes on purpose: the PIN control owns that corner and is hit-tested by its drawn position, so anything to its right moves it out from @@ -75,8 +85,7 @@ URL afterwards. ## Open, in the order I would do them -1. **The `+` rendering** (above). -2. **Phantom client on headless `sl0ppi up`.** `attach --create-background` +1. **Phantom client on headless `sl0ppi up`.** `attach --create-background` leaves one client in the session forever, so every pane frame shows `MY FOCUS AND:`. Worked around by creating the session *by attaching* when there is a tty; the headless path still leaks. Ruled out: the CLI-action @@ -86,14 +95,15 @@ URL afterwards. `ScreenInstruction::RemoveClient` on a debug build to find where it is lost. Cheap alternative: give the headless path its own pty (`script -qec`) and kill the client afterwards — killed clients demonstrably do not leak. -3. **bun is the AVX2 build.** Caused `Illegal instruction` on a plain KVM guest. +2. **bun is the AVX2 build.** Caused `Illegal instruction` on a plain KVM guest. One line in `packaging/fetch-vendor.sh`: `bun-linux-x64-baseline.zip`. -4. **`pir` needs python3 + fzf**, neither shipped. The fix is folding it into +3. **`pir` needs python3 + fzf**, neither shipped. The fix is folding it into `sl0ppi agent --resume` (already designed in `docs/cli-design.md`). -5. **Tab overflow.** `sl0ppi open <name>` wants to be a fuzzy picker once a root +4. **Tab overflow.** `sl0ppi open <name>` wants to be a fuzzy picker once a root has a dozen projects. -6. Button polish: shift/right-click for the other split direction; reuse - `status_bar_button_style` so it looks pressable. +5. Button polish: shift/right-click for the other split direction; reuse + `status_bar_button_style` so it looks pressable. Extend `make check-live` + rather than trusting a snapshot. Fixed this session, for context: client ids were recycled (silently ate ~25% of `zellij action` output), suspended panes spammed resize errors, responsive @@ -123,6 +133,15 @@ bar. See `docs/roadmap.md` for each with its measurements. template into each tab instead of emitting `default_tab_template`. - **The tab test harness does no rendering**, so `panes_to_hide` is always empty and stack-related bugs cannot be reproduced in it. Use a scripted pty. +- **A hit-test that never consults the renderer is not evidence of anything.** + The split button "worked" for two releases while being invisible. If an + affordance is drawn, the check has to *find the glyph* and click where it + found it — `packaging/checks/split-button-live.py` is the pattern. +- **`os.read` on a settled pty blocks forever.** `os.set_blocking(fd, False)` + and catch `BlockingIOError`, or a live check hangs instead of failing. +- **The pane's geometry is not where its frame is drawn.** With a gap, the frame + is inset from the allocation: use `Pane::frame_rect()` for anything the user + can see, `position_and_size()` only for the layout solver. - **Clean up test sessions.** A background-created session I left behind is what produced a "MY FOCUS" bug report from the user. diff --git a/docs/roadmap.md b/docs/roadmap.md index bc8aa6f..98c8949 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -215,6 +215,37 @@ Two panes sharing one rect means a stack; distinct rects mean it really expanded. Check both directions (create narrow then widen, create wide then narrow) and add panes afterwards. +## Fixed: the `+` split button was drawn nowhere and clicked in the gap + +Shipped in 0.1.7 and 0.1.8 without ever being visible on a live session, while +rendering in 265 unit snapshots. Four separate faults, each individually enough +to hide it: + +- `title_line_without_middle` computed the right side only if the pane had a + title -- and `hide_default_pane_names true` means unnamed panes have none; +- `title_line_with_middle` dropped the right side whenever the left was absent, + which is every frame in a session with a second client (`MY FOCUS AND:`); +- the title was budgeted before the button, so a title longer than the frame + (i.e. any shell showing a path) consumed its columns; +- the click hit-tested the pane's allocation while the frame is drawn inset by + the pane gap, putting the clickable columns two to the right of the glyph. + +The first three are fixed in `ui/pane_boundaries_frame.rs` by reserving the +anchored button before anything else and never discarding the right side; the +fourth by `Pane::frame_rect()`, the rect the frame is actually drawn in. + +**How to re-run the check** -- unit tests were necessary and insufficient here +for the fourth time, so this one reads the screen: + +```bash +make check-live # packaging/checks/split-button-live.py +``` + +It starts a session at a pty with the live frame config, finds the `+` on the +frame, attaches a second client (the middle builder), and clicks the columns the +glyph was found in, expecting `2 -> 3` panes. Confirmed to fail on the commit +before the fix, for both the render and the click. + ## Standing risks **`sl0ppij/target` is a symlink to `~/lol/zellij/target`.** Introduced when the diff --git a/packaging/checks/split-button-live.py b/packaging/checks/split-button-live.py new file mode 100755 index 0000000..51b8e25 --- /dev/null +++ b/packaging/checks/split-button-live.py @@ -0,0 +1,242 @@ +#!/usr/bin/env python3 +"""Live check: is the `+` split button DRAWN, and does clicking the drawn glyph +split the pane? + +The unit tests can only prove the title line composes correctly. This drives a +real session with the live config's frame settings -- centred titles, rounded +corners, a pane gap, no default pane names -- because every one of those takes a +different path through the title builders, and the gap moves the drawn frame +away from the pane's allocation. + +It then attaches a SECOND client, because that is what the live box always had +(the phantom client a headless `sl0ppi up` leaks) and it switches the frame onto +a third title builder again -- the one with a "MY FOCUS" middle, which is where +the button was being dropped for two releases. + +Usage: split-button-live.py [path-to-sl0ppij] +Exits non-zero with a description of what it saw. +""" + +import fcntl +import os +import pty +import re +import struct +import subprocess +import sys +import tempfile +import termios +import time + +BIN = os.path.abspath( + sys.argv[1] + if len(sys.argv) > 1 + else os.path.join(os.path.dirname(__file__), "../../sl0ppij/target/fastdev/sl0ppij") +) +ROWS, COLS = 30, 100 +SESSION = f"sl0ptest-splitbutton-{os.getpid()}" + +CONFIG = """ +ui { + pane_frames { + rounded_corners true + title_alignment "center" + gap 1 + padding 0 0 0 0 + gap_aspect 2 + hide_default_pane_names true + } +} +pane_frames true +pane_frame_style "full" +default_mode "locked" +show_startup_tips false +""" + +ANSI = re.compile(r"\x1b\[[0-9;?]*[a-zA-Z]|\x1b[]P][^\x07\x1b]*(\x07|\x1b\\)?|\x1b[()][0-9A-B]") + + +def screen_rows(raw): + """Rebuild the visible grid from the pty stream: absolute cursor moves plus + printable runs. Enough for reading a frame; not a terminal emulator.""" + grid = {} + row = col = 0 + i = 0 + while i < len(raw): + ch = raw[i] + if ch == "\x1b": + m = re.match(r"\x1b\[([0-9;]*)([a-zA-Z])", raw[i:]) + if m: + args, verb = m.group(1), m.group(2) + nums = [int(n) for n in args.split(";") if n != ""] + if verb == "H": + row = (nums[0] - 1) if nums else 0 + col = (nums[1] - 1) if len(nums) > 1 else 0 + elif verb == "J": + grid.clear() + i += m.end() + continue + m = re.match(r"\x1b][^\x07\x1b]*(\x07|\x1b\\)", raw[i:]) + if m: + i += m.end() + continue + i += 1 + continue + if ch == "\r": + col = 0 + elif ch == "\n": + row += 1 + col = 0 + elif ch >= " ": + grid[(row, col)] = ch + col += 1 + i += 1 + out = [] + for r in range(ROWS): + line = "".join(grid.get((r, c), " ") for c in range(COLS)) + out.append(line.rstrip()) + return out + + +def spawn_client(env, args, rows=ROWS, cols=COLS): + pid, fd = pty.fork() + if pid == 0: + os.execvpe(BIN, [BIN] + args, env) + fcntl.ioctl(fd, termios.TIOCSWINSZ, struct.pack("HHHH", rows, cols, 0, 0)) + # non-blocking: a settled client produces nothing, and a blocking read on + # its pty would hang this check instead of failing it + os.set_blocking(fd, False) + return pid, fd + + +def read_screen(fd, buffered, seconds=2.5, until=None): + deadline = time.time() + seconds + while time.time() < deadline: + time.sleep(0.4) + try: + buffered += os.read(fd, 1 << 20).decode("utf8", "replace") + except BlockingIOError: + pass + except OSError: + break + if until and until(screen_rows(buffered)): + break + return buffered, screen_rows(buffered) + + +def find_button(rows): + """(row, 0-based column of the `+`) on a pane's top frame row.""" + for y, line in enumerate(rows): + if ("╭" in line or "┌" in line) and " + " in line: + return y, line.index(" + ") + 1 + return None + + +def main(): + tmp = tempfile.mkdtemp(prefix="sl0p-splitbutton-") + config_file = os.path.join(tmp, "config.kdl") + with open(config_file, "w") as f: + f.write(CONFIG) + env = dict(os.environ) + env.update( + { + "SL0PPIJ_SOCKET_DIR": os.path.join(tmp, "sock"), # isolate from live sessions + "SL0PPIJ_CONFIG_FILE": config_file, + "SL0PPIJ_CONFIG_DIR": tmp, + "SL0PPIJ_CACHE_DIR": os.path.join(tmp, "cache"), + "TERM": "xterm-256color", + "SHELL": "/bin/sh", + "PS1": "$ ", + } + ) + + pid, fd = spawn_client(env, ["-s", SESSION]) + second_pid = second_fd = None + + raw = "" + failures = [] + try: + raw, rows = read_screen(fd, raw, seconds=20, until=find_button) + + # 1. one client: is it drawn at all, on the pane's top frame row? + hit = find_button(rows) + if hit is None: + failures.append("one client: the split button was not drawn on any frame row") + print("\n".join(f"{i:02d}|{r}" for i, r in enumerate(rows))) + return failures + y, x = hit + print(f"one client: drawn at row {y}, column {x}") + print(f" {rows[y]}") + + # 2. two clients: the frame grows a "MY FOCUS" middle, which is a + # different title builder -- and the one the live box always used, + # because a headless `sl0ppi up` leaves a phantom client behind. + second_pid, second_fd = spawn_client(env, ["attach", SESSION]) + _, second_rows = read_screen(second_fd, "", seconds=10, until=find_button) + raw, rows = read_screen(fd, raw, seconds=5) + focus_middle = any("FOCUS" in r or "| ME |" in r for r in rows + second_rows) + if not focus_middle: + failures.append("two clients: no focus indication -- the second client did not attach") + hit2 = find_button(rows) + print(f"two clients: drawn at {hit2}") + if hit2 is None: + failures.append("two clients: the split button was dropped from the title line") + print("\n".join(f"{i:02d}|{r}" for i, r in enumerate(rows))) + elif hit2 != hit: + failures.append(f"two clients: the button moved, {hit} -> {hit2}") + if hit2: + print(f" {rows[hit2[0]]}") + + def pane_count(): + proc = subprocess.run( + [BIN, "-s", SESSION, "action", "list-panes", "--json"], + env=env, + capture_output=True, + text=True, + ) + if os.environ.get("DEBUG"): + print(f"list-panes: {proc.stdout[:2000]!r} {proc.stderr[:500]!r}") + return proc.stdout.count('"pane_x"') + + before = pane_count() + + # 2. clicking two columns right of the glyph -- where the OLD hit-test + # looked, out in the pane gap -- must do nothing + os.write(fd, f"\x1b[<0;{x + 3};{y + 1}M".encode()) + os.write(fd, f"\x1b[<0;{x + 3};{y + 1}m".encode()) + time.sleep(1.5) + if pane_count() != before: + failures.append("a click in the gap beside the button split the pane") + + # 3. clicking the drawn glyph splits + os.write(fd, f"\x1b[<0;{x + 1};{y + 1}M".encode()) + os.write(fd, f"\x1b[<0;{x + 1};{y + 1}m".encode()) + time.sleep(2) + after = pane_count() + print(f"panes: {before} -> {after}") + if os.environ.get("DEBUG"): + raw, rows = read_screen(fd, raw, seconds=1) + print("\n".join(f"{i:02d}|{r}" for i, r in enumerate(rows))) + if after != before + 1: + failures.append(f"clicking the drawn button did not split ({before} -> {after})") + return failures + finally: + subprocess.run( + [BIN, "-s", SESSION, "kill-session"], env=env, capture_output=True, text=True + ) + for p, f in ((pid, fd), (second_pid, second_fd)): + if f is None: + continue + try: + os.close(f) + except OSError: + pass + os.waitpid(p, os.WNOHANG) + + +if __name__ == "__main__": + problems = main() + for p in problems: + print(f"FAIL: {p}") + print("ok: the split button is drawn where it is clicked" if not problems else "") + sys.exit(1 if problems else 0) diff --git a/sl0ppij/zellij-server/src/panes/plugin_pane.rs b/sl0ppij/zellij-server/src/panes/plugin_pane.rs index 3c1816d..9e96323 100644 --- a/sl0ppij/zellij-server/src/panes/plugin_pane.rs +++ b/sl0ppij/zellij-server/src/panes/plugin_pane.rs @@ -366,6 +366,14 @@ impl Pane for PluginPane { fn current_geom(&self) -> PaneGeom { self.geom_override.unwrap_or(self.geom) } + fn frame_rect(&self) -> Viewport { + // must match render_frame(), which draws at exactly this rect + Viewport::from(self.current_geom()).inset_by(if self.draws_frame { + self.gap_offset + } else { + Offset::default() + }) + } fn geom_override(&self) -> Option<PaneGeom> { self.geom_override } diff --git a/sl0ppij/zellij-server/src/panes/terminal_pane.rs b/sl0ppij/zellij-server/src/panes/terminal_pane.rs index 473b11a..5b2e68a 100644 --- a/sl0ppij/zellij-server/src/panes/terminal_pane.rs +++ b/sl0ppij/zellij-server/src/panes/terminal_pane.rs @@ -473,6 +473,14 @@ impl Pane for TerminalPane { fn current_geom(&self) -> PaneGeom { self.geom_override.unwrap_or(self.geom) } + fn frame_rect(&self) -> Viewport { + // must match render_frame(), which draws at exactly this rect + Viewport::from(self.current_geom()).inset_by(if self.draws_frame { + self.gap_offset + } else { + Offset::default() + }) + } fn geom_override(&self) -> Option<PaneGeom> { self.geom_override } diff --git a/sl0ppij/zellij-server/src/tab/mod.rs b/sl0ppij/zellij-server/src/tab/mod.rs index 9ed4ce2..8ce4af6 100644 --- a/sl0ppij/zellij-server/src/tab/mod.rs +++ b/sl0ppij/zellij-server/src/tab/mod.rs @@ -450,6 +450,17 @@ pub trait Pane { } fn position_and_size(&self) -> PaneGeom; fn current_geom(&self) -> PaneGeom; + /// The rect this pane's FRAME is drawn in -- where its borders, title line + /// and the controls on that line actually land on screen. + /// + /// NOT the pane's allocation: this fork insets the drawn frame by the + /// configured pane gap, so with `gap 1` the top-left corner is a row and + /// two columns in from `position_and_size()`. Anything hit-testing a drawn + /// affordance has to use this, or it claims columns the user can see are + /// empty (and misses the ones they can see are not). + fn frame_rect(&self) -> Viewport { + Viewport::from(self.current_geom()) + } fn geom_override(&self) -> Option<PaneGeom>; fn should_render(&self) -> bool; fn set_should_render(&mut self, should_render: bool); diff --git a/sl0ppij/zellij-server/src/tab/mouse_handler.rs b/sl0ppij/zellij-server/src/tab/mouse_handler.rs index 6961fc7..6bb2a05 100644 --- a/sl0ppij/zellij-server/src/tab/mouse_handler.rs +++ b/sl0ppij/zellij-server/src/tab/mouse_handler.rs @@ -476,10 +476,17 @@ impl MouseHandler { /// sl0p fork: clicking the `+` on a pane's title line splits that pane. /// /// Spawning a pane was the one common operation with no mouse path at all -- - /// switching, renaming, reordering and resizing all have one. The button is - /// only drawn while the pointer is over the pane (see `render_split_button`), - /// and both sides derive its columns from `split_button_columns`, so what is - /// clickable is exactly what is visible. + /// switching, renaming, reordering and resizing all have one. Both sides + /// derive its columns from `split_button_columns` over the pane's DRAWN + /// frame rect, so what is clickable is exactly what is visible. + /// + /// `frame_rect()`, not `position_and_size()`: with a pane gap configured the + /// frame is drawn inset from the allocation, and hit-testing the allocation + /// put the clickable columns out in the blank gap -- two columns right of + /// the drawn `+`, and a row above it for panes at the top of the screen. + /// The click "worked" there only because this test never consults the + /// renderer, which is precisely why it could not be trusted as evidence the + /// button was drawn. /// /// Intercepted before the OSC-5577 status bar buttons: those are drawn by /// the program inside the pane on the row BELOW the content, so the two @@ -508,13 +515,13 @@ impl MouseHandler { if !pane.selectable() { return Ok(None); } - let geom = pane.position_and_size(); + let frame = pane.frame_rect(); let pane_id = pane.pid(); - // the title line is the pane's top row - if event.position.line() != geom.y as isize { + // the title line is the frame's top row + if event.position.line() != frame.y as isize { return Ok(None); } - let Some(columns) = split_button_columns(geom.x, geom.cols.as_usize()) else { + let Some(columns) = split_button_columns(frame.x, frame.cols) else { return Ok(None); }; if !columns.contains(&event.position.column()) { diff --git a/sl0ppij/zellij-server/src/tab/unit/snapshots/zellij_server__tab__tab_integration_tests__ctrl_scroll_up_merging_stacks_preserves_all_panes.snap b/sl0ppij/zellij-server/src/tab/unit/snapshots/zellij_server__tab__tab_integration_tests__ctrl_scroll_up_merging_stacks_preserves_all_panes.snap index 173a0a8..4878cb1 100644 --- a/sl0ppij/zellij-server/src/tab/unit/snapshots/zellij_server__tab__tab_integration_tests__ctrl_scroll_up_merging_stacks_preserves_all_panes.snap +++ b/sl0ppij/zellij-server/src/tab/unit/snapshots/zellij_server__tab__tab_integration_tests__ctrl_scroll_up_merging_stacks_preserves_all_panes.snap @@ -1,13 +1,12 @@ --- source: zellij-server/src/tab/./unit/tab_integration_tests.rs -assertion_line: 13079 expression: snapshot --- 00 (C): > │ 1: Pane #1 │ 01 (C): │ 2: Pane #3 │ 02 (C): │ 3: Pane #4 │ 03 (C): -04 (C): ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +04 (C): ┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + ┐ 05 (C): │ │ 06 (C): │ │ 07 (C): │ │ @@ -29,7 +28,7 @@ expression: snapshot 23 (C): │ 2: Pane #5 │ 24 (C): > │ 3: Pane #6 │ 25 (C): -26 (C): ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +26 (C): ┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + ┐ 27 (C): │ │ 28 (C): │ │ 29 (C): │ │ diff --git a/sl0ppij/zellij-server/src/ui/pane_boundaries_frame.rs b/sl0ppij/zellij-server/src/ui/pane_boundaries_frame.rs index adfaf25..7fffceb 100644 --- a/sl0ppij/zellij-server/src/ui/pane_boundaries_frame.rs +++ b/sl0ppij/zellij-server/src/ui/pane_boundaries_frame.rs @@ -12,12 +12,16 @@ use crate::ClientId; use zellij_utils::data::{client_id_to_colors, PaletteColor, PaneTitleAlignment, Style}; use zellij_utils::input::layout::{layout_status_bar, StatusBarButton}; -/// sl0p fork: the mouse "split this pane" button drawn at the right end of a -/// hovered pane's title line, and the geometry both the renderer and the mouse -/// handler derive it from. +/// sl0p fork: the mouse "split this pane" button drawn at the right end of +/// every tiled pane's title line, and the geometry both the renderer and the +/// mouse handler derive it from. /// /// Kept here, next to the code that draws it, so the two cannot drift: a button /// you can see but not click (or click but not see) is worse than no button. +/// Both halves of that drifted anyway, for two whole releases -- see +/// `render_split_button` and `intercept_split_button_click`. The check that +/// would have caught it is `packaging/checks/split-button-live.py`, which reads +/// the glyph off a real session and clicks the columns it was found in. pub const SPLIT_BUTTON_TEXT: &str = " + "; pub const SPLIT_BUTTON_WIDTH: usize = 3; /// Narrower than this and the title line has nothing to spare; the button is @@ -365,16 +369,24 @@ impl PaneFrame { /// /// Drawn on every frame, not only while hovered. It WAS hover-gated, which /// looked tidier and did not work: `mouse_is_hovering_over_pane` never - /// became true in a live session, so the button stayed invisible while its - /// hit-test -- geometric, and never consulting hover -- kept working. - /// Clickable but invisible is the worst of both, so visibility no longer - /// depends on state that is not reaching this renderer. + /// became true in a live session. Removing the gate did not make the button + /// appear either -- the actual reason it was invisible for two releases was + /// upstairs, in the title line builders: /// - /// Why that flag is false live is still open: the plumbing from - /// `mouse_hover_pane_id` through `FrameParams` reads correctly, and both - /// `advanced_mouse_actions` and `mouse_hover_effects` are set in the config - /// that reproduces it. Re-gate on hover only once that is understood AND - /// demonstrated on a real session, not just in a snapshot test. + /// - the right side was only computed at all if the pane had a TITLE, and + /// `hide_default_pane_names` (live) leaves unnamed panes with none; + /// - the builder that runs when a pane has a "MY FOCUS" middle -- i.e. + /// any session with a second client, including the phantom one a + /// headless `sl0ppi up` leaks -- dropped the right side entirely; + /// - a title longer than the frame ate the button's columns, and shell + /// titles are long paths. + /// + /// None of it reproduced in the 265 snapshots, which all had a titled pane, + /// one client and a left-aligned title. `split_button_is_drawn_where_it_is_hit_tested` + /// covers the combinations the live config actually produces. + /// + /// Re-gate on hover only once that flag is understood AND demonstrated on a + /// real session, not just in a snapshot test. fn render_split_button(&self, max_length: usize) -> Option<(Vec<TerminalCharacter>, usize)> { if !self.is_selectable { return None; @@ -478,23 +490,28 @@ impl PaneFrame { } } fn render_my_focus(&self, max_length: usize) -> Option<(Vec<TerminalCharacter>, usize)> { - let mut left_separator = foreground_color(boundary_type::VERTICAL_LEFT, self.color); - let mut right_separator = foreground_color(boundary_type::VERTICAL_RIGHT, self.color); + // cloned per use: `Vec::append` DRAINS its argument, so building the + // full indication first left the short one with no separators at all -- + // 4 characters wide while reporting 6, which is a title line two cells + // short of the pane and a corrupted grid row. Only bit on very narrow + // panes, which is where the short form is the one that gets used. + let left_separator = || foreground_color(boundary_type::VERTICAL_LEFT, self.color); + let right_separator = || foreground_color(boundary_type::VERTICAL_RIGHT, self.color); let full_indication_text = "MY FOCUS"; let mut full_indication = vec![]; - full_indication.append(&mut left_separator); + full_indication.append(&mut left_separator()); full_indication.push(EMPTY_TERMINAL_CHARACTER); full_indication.append(&mut foreground_color(full_indication_text, self.color)); full_indication.push(EMPTY_TERMINAL_CHARACTER); - full_indication.append(&mut right_separator); + full_indication.append(&mut right_separator()); let full_indication_len = full_indication_text.width() + 4; // 2 for separators 2 for padding let short_indication_text = "ME"; let mut short_indication = vec![]; - short_indication.append(&mut left_separator); + short_indication.append(&mut left_separator()); short_indication.push(EMPTY_TERMINAL_CHARACTER); short_indication.append(&mut foreground_color(short_indication_text, self.color)); short_indication.push(EMPTY_TERMINAL_CHARACTER); - short_indication.append(&mut right_separator); + short_indication.append(&mut right_separator()); let short_indication_len = short_indication_text.width() + 4; // 2 for separators 2 for padding if full_indication_len <= max_length { Some((full_indication, full_indication_len)) @@ -718,15 +735,18 @@ impl PaneFrame { self.get_corner(boundary_type::TOP_RIGHT), self.color, )); - } else if col == left_side_start_position { + // the `> 0` guards let a caller pass an EMPTY segment (an unnamed + // pane has no left side) without the loop stalling on a segment + // that advances `col` by nothing. + } else if col == left_side_start_position && *left_side_len > 0 { title_line.append(&mut left_side); col += left_side_len; continue; - } else if col == middle_start_position { + } else if col == middle_start_position && *middle_len > 0 { title_line.append(&mut middle); col += middle_len; continue; - } else if col == right_side_start_position { + } else if col == right_side_start_position && *right_side_len > 0 { title_line.append(&mut right_side); col += right_side_len; continue; @@ -886,7 +906,12 @@ impl PaneFrame { let total_title_length = self.geom.cols.saturating_sub(2); // 2 for the left and right corners let length_of_each_side = total_title_length.saturating_sub(*middle_len + 2) / 2; let left_side = self.render_title_left_side(length_of_each_side); - let right_side = self.render_title_right_side(length_of_each_side); + // sl0p fork: at least the anchored button, even when half the leftover + // line is narrower than it. `render_title` has already kept the middle + // short enough that this cannot reach the middle: the middle is centred, + // so its budget was reduced by the button's width at BOTH ends. + let right_side = + self.render_title_right_side(length_of_each_side.max(self.anchored_right_side_len())); match (left_side, right_side) { (Some((left_side, left_side_len)), Some((right_side, right_side_len))) => self @@ -901,7 +926,20 @@ impl PaneFrame { (Some((left_side, left_side_len)), None) => { self.left_and_middle_title_line(left_side, &left_side_len, middle, middle_len) }, - _ => self.middle_only_title_line(middle, middle_len), + // sl0p fork: an untitled pane still has a right side, and it used + // to be dropped here along with the (absent) title -- which is how + // the split button could render in 265 snapshots and never once on + // a live session, where `hide_default_pane_names` leaves unnamed + // panes with no title at all. + (None, Some((right_side, right_side_len))) => self.three_part_title_line( + vec![], + &0, + middle, + middle_len, + right_side, + &right_side_len, + ), + (None, None) => self.middle_only_title_line(middle, middle_len), } } /// Compose a title line with the pane title placed at `alignment` rather than @@ -971,17 +1009,32 @@ impl PaneFrame { title_line } fn title_line_without_middle(&self) -> Vec<TerminalCharacter> { - // Left keeps the original code path untouched, so the default config - // renders byte-for-byte as before. + // Left keeps the original code path, so the default config renders as + // before -- except that the anchored button is reserved up front. if self.style.pane_title_alignment != PaneTitleAlignment::Left { return self.aligned_title_line(self.style.pane_title_alignment); } + // sl0p fork: the title is budgeted AFTER the anchored button, which + // upstream's order could not do -- and shell titles are long paths, so + // a title that filled the line used to leave the button nothing. Only + // the button is reserved: the indications to its left keep the + // historical "whatever the title leaves" budget. let total_title_length = self.geom.cols.saturating_sub(2); // 2 for the left and right corners - let left_side = self.render_title_left_side(total_title_length); - let right_side = left_side.as_ref().and_then(|(_left_side, left_side_len)| { - let space_left = total_title_length.saturating_sub(*left_side_len + 1); // 1 for a middle separator - self.render_title_right_side(space_left) - }); + let anchored = self.anchored_right_side_len(); + let separator = if anchored > 0 { 1 } else { 0 }; + let left_side = + self.render_title_left_side(total_title_length.saturating_sub(anchored + separator)); + let right_side = match left_side.as_ref() { + Some((_left_side, left_side_len)) => { + let space_left = total_title_length.saturating_sub(*left_side_len + 1); // 1 for a middle separator + self.render_title_right_side(space_left) + }, + // sl0p fork: with no title the right side gets the whole line. + // Upstream only ever computed it INSIDE the left side's `and_then`, + // so an unnamed pane (`hide_default_pane_names true`, the live + // default) silently had no scroll indication and no split button. + None => self.render_title_right_side(total_title_length), + }; match (left_side, right_side) { (Some((left_side, left_side_len)), Some((right_side, right_side_len))) => { self.two_part_title_line(left_side, &left_side_len, right_side, &right_side_len) @@ -989,16 +1042,40 @@ impl PaneFrame { (Some((left_side, left_side_len)), None) => { self.left_only_title_line(left_side, &left_side_len) }, - _ => self.empty_title_line(), + // an empty left side lays out exactly like a two-part line whose + // left part is zero-width: padding, then the right side flush + // against the corner. + (None, Some((right_side, right_side_len))) => { + self.two_part_title_line(vec![], &0, right_side, &right_side_len) + }, + (None, None) => self.empty_title_line(), } } + /// Width of the part of the title line that is ANCHORED to the top-right + /// corner and may therefore never be squeezed out by layout: the split + /// button, whose hit-test is a pure function of the pane's geometry + /// (`split_button_columns`) and cannot ask the renderer what it drew. + /// + /// Everything else on the right side (scroll, pin) is positional only in + /// relation to it, so it keeps its historical leftover budget. + fn anchored_right_side_len(&self) -> usize { + self.render_split_button(self.geom.cols.saturating_sub(2)) + .map(|(_, len)| len) + .unwrap_or(0) + } fn render_title(&self) -> Result<Vec<TerminalCharacter>> { + // The middle is CENTRED, so room for the anchored button has to come + // off BOTH ends -- otherwise a middle that is merely centred-and-wide + // reaches into the button's columns, and the layout loop, which only + // recognises a segment's exact start column, walks straight past the + // button and emits a title line narrower than the pane. let total_title_length = self.geom.cols.saturating_sub(2); // 2 for the left and right corners + let middle_budget = total_title_length.saturating_sub(2 * self.anchored_right_side_len()); - self.render_title_middle(total_title_length) - .map(|(middle, middle_length)| self.title_line_with_middle(middle, &middle_length)) - .or_else(|| Some(self.title_line_without_middle())) - .with_context(|| format!("failed to render title '{}'", self.title)) + Ok(match self.render_title_middle(middle_budget) { + Some((middle, middle_length)) => self.title_line_with_middle(middle, &middle_length), + None => self.title_line_without_middle(), + }) } fn render_stack_list_entry(&self, entry: &StackListEntry) -> Vec<TerminalCharacter> { let usable_cols = self.geom.cols; @@ -1119,12 +1196,9 @@ impl PaneFrame { } fn render_one_line_title(&self) -> Result<Vec<TerminalCharacter>> { if self.should_draw_pane_frames { - let total_title_length = self.geom.cols.saturating_sub(2); - return self - .render_title_middle(total_title_length) - .map(|(middle, middle_length)| self.title_line_with_middle(middle, &middle_length)) - .or_else(|| Some(self.title_line_without_middle())) - .with_context(|| format!("failed to render title '{}'", self.title)); + // a framed pane one row tall is all title line, laid out exactly as + // the top row of a taller one + return self.render_title(); } let width = self.geom.cols; @@ -2097,4 +2171,123 @@ mod tests { assert!(!text.contains("MouseScroll")); assert!(text.contains("<drag borders>")); } + + /// Every combination the LIVE config puts a frame in, which the earlier + /// snapshots never did: they all used zellij's defaults (left-aligned, + /// square corners, a pane title, one client). + /// + /// `title`: `hide_default_pane_names true` renders unnamed panes with an + /// EMPTY title. `other_focused_clients` / `other_cursors_exist_in_session`: + /// any second client in the session -- including the phantom one a headless + /// `sl0ppi up` leaks -- puts a "MY FOCUS" middle in the title line. + fn live_frames(cols: usize) -> Vec<(String, PaneFrame)> { + let mut frames = vec![]; + for alignment in [ + PaneTitleAlignment::Left, + PaneTitleAlignment::Center, + PaneTitleAlignment::Right, + ] { + for rounded_corners in [false, true] { + for title in ["", "mytitle"] { + for (other_clients, other_cursors) in + [(vec![], false), (vec![], true), (vec![2], true)] + { + let mut frame = pane_frame_with(false, false, cols); + frame.title = title.to_string(); + frame.style.pane_title_alignment = alignment; + frame.style.rounded_corners = rounded_corners; + frame.other_focused_clients = other_clients.clone(); + frame.other_cursors_exist_in_session = other_cursors; + frames.push(( + format!( + "cols {} {:?} rounded {} title {:?} other_clients {:?} other_cursors {}", + cols, alignment, rounded_corners, title, other_clients, other_cursors + ), + frame, + )); + } + } + } + } + frames + } + + /// The columns of a rendered title line that the mouse handler will treat + /// as the split button. + fn drawn_split_button(frame: &PaneFrame) -> String { + let rendered: Vec<char> = characters_to_string(&frame.render_title().unwrap()) + .chars() + .collect(); + assert_eq!( + rendered.len(), + frame.geom.cols, + "title line is not the width of the pane" + ); + let columns = split_button_columns(frame.geom.x, frame.geom.cols) + .expect("no split button geometry for this pane"); + rendered[columns.start - frame.geom.x..columns.end - frame.geom.x] + .iter() + .collect() + } + + #[test] + fn split_button_is_drawn_where_it_is_hit_tested() { + // REGRESSION: the button rendered in every snapshot and was never once + // visible on a live session. The hit-test is geometric and never + // consults the renderer, so "clicking it works" proved nothing: the + // title builder that runs when a pane has no title AND a "MY FOCUS" + // middle dropped the whole right side of the title line on the floor. + // + // Asserting on the exact COLUMNS rather than "contains a +" is the + // point: the short "MY FOCUS AND: +" indication contains one too. + for cols in [20, 41, 80, 120] { + for (what, frame) in live_frames(cols) { + assert_eq!( + drawn_split_button(&frame), + SPLIT_BUTTON_TEXT, + "split button not drawn in its clickable columns: {}", + what + ); + } + } + } + + #[test] + fn split_button_is_not_drawn_when_the_pane_is_too_narrow() { + // the other half of the same invariant: where the hit-test declines to + // find a button, nothing may be drawn that looks like one. + // + // From 2: a 1-column pane renders 2 characters (both corners) on the + // left-aligned path, which is upstream behaviour and not this feature's + // to fix. + for cols in 2..SPLIT_BUTTON_MIN_PANE_COLS { + for (what, frame) in live_frames(cols) { + assert!( + split_button_columns(frame.geom.x, frame.geom.cols).is_none(), + "{}: expected no button geometry below {} cols", + what, + SPLIT_BUTTON_MIN_PANE_COLS + ); + let rendered: Vec<char> = characters_to_string(&frame.render_title().unwrap()) + .chars() + .collect(); + assert_eq!(rendered.len(), cols, "{}", what); + // positional, not `contains`: the short multi-client + // indication is itself a "+" between separators, so a + // substring search here reports a button that is not one. + if cols > SPLIT_BUTTON_WIDTH + 1 { + let would_be: String = rendered[cols - 1 - SPLIT_BUTTON_WIDTH..cols - 1] + .iter() + .collect(); + assert_ne!( + would_be, + SPLIT_BUTTON_TEXT, + "{}: drew a split button that cannot be clicked: {:?}", + what, + rendered.iter().collect::<String>() + ); + } + } + } + } } |
