From 026e78078e4ee31cefa49129f02dc957674f173b Mon Sep 17 00:00:00 2001 From: Jean-Pierre Martineau Date: Tue, 1 Sep 2026 02:19:19 -0400 Subject: [PATCH] The icon column is measured, not assumed to be two cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit πŸ—‘ is drawn in one cell where πŸ’Ύ, 🌍, ⏳ and πŸ“± are drawn in two, and a command row that wrote `icon + " "` started its label wherever its own icon ended. So Delete contact… sat a column left of Archive contact above it (JP). Ten of the app's lexicon icons are like this β€” πŸ—‘ ✎ βš™ β–Ά β˜… ↻ ↕ ⇄ ⌨ # β€” so any list mixing them with the other twenty-seven was out of line; the Contacts tail's πŸ—‘ Purge against πŸ“‚ View archived was the same fault one screen over. Not an emoji-width entry. The classifier is clear that both authorities and the terminal agree πŸ—‘ is one cell: it sits outside Emoji_Presentation and carries no VS16, so the font draws a one-cell text glyph and everyone is right. Forcing it into the wide set would reserve a cell nothing draws and pull the border in β€” the mistake πŸ›£ already cost the Trophy case. The measurement was never wrong; the layout assumed it away. The trace and TX sweep screens had solved this properly and privately, each with its own copy: declare the marks a list can draw, measure the widest, pad every mark out to it. That is now menus.icon_lane/icon_mark, THE icon column, and those two delegate rather than duplicate. marked_label takes the lane so a mixed list can pass it, defaulting to measuring the single icon β€” which stays right for a list whose rows all lead with the same mark, and is precisely what misaligns one that doesn't. The node page measures its column once over every action it can offer, so the group ends aligned however many of archive/restore/delete a given contact earns. PicoCalc is unaffected and stays that way by construction: it draws no icon lane at all, so the column measures zero, the separator goes with it, and the tint lands on the label as before. The tests assert in display cells rather than characters. That distinction is the bug β€” πŸ—‘ plus two spaces and πŸ“‚ plus one are three cells and different string lengths, and an index-based assertion would have called the fix broken and the break fixed. Co-Authored-By: Claude Opus 5 (1M context) --- meshterm/ui/contacts_screen.py | 17 +++++-- meshterm/ui/menus.py | 60 +++++++++++++++++++++-- meshterm/ui/node_detail_screen.py | 20 ++++++-- meshterm/ui/trace_screen.py | 25 ++-------- meshterm/ui/tx_screen.py | 14 ++---- tests/test_menus.py | 80 +++++++++++++++++++++++++++++++ 6 files changed, 175 insertions(+), 41 deletions(-) diff --git a/meshterm/ui/contacts_screen.py b/meshterm/ui/contacts_screen.py index 10afc1e..fb2016b 100644 --- a/meshterm/ui/contacts_screen.py +++ b/meshterm/ui/contacts_screen.py @@ -40,7 +40,7 @@ from typing import TYPE_CHECKING from .archived_screen import open_archived from .contactlist import ContactListScreen, ContactRow -from .menus import marked_label +from .menus import icon_lane, marked_label from .purge_screen import purge_contacts from .tui import Choice, Separator from .tui.screen import CANCEL @@ -59,6 +59,11 @@ YOU = ("you",) _PURGE = ("purge",) _ARCHIVED = ("archived",) +#: Every mark the tail's maintenance rows lead with β€” the set the icon column is measured +#: over (see :func:`~meshterm.ui.menus.icon_lane`). Declared rather than inferred so the +#: column is one width for the whole tail whichever of the two rows a given visit draws. +_TAIL_ICONS = ("πŸ—‘", "πŸ“‚") + class ContactsScreen(ContactListScreen): """A full-screen, Ctrl+arrow-sortable list of this node and its known contacts. @@ -114,17 +119,23 @@ class ContactsScreen(ContactListScreen): # the way in to what the sweep has already taken. The two sit together because they # are the two halves of one idea, and the archived row is *under* the purge for the # same reason: it is where the purge's output went. + # One icon column for both rows: πŸ—‘ draws a single cell where πŸ“‚ draws two, so an + # unpadded mark would start the purge row's label a column left of the other's. + lane = icon_lane(_TAIL_ICONS) tail: list = [] if contacts: tail = [ Separator(" "), - Choice(title=marked_label("πŸ—‘", "Purge contacts…", "err"), value=_PURGE), + Choice( + title=marked_label("πŸ—‘", "Purge contacts…", "err", lane=lane), + value=_PURGE, + ), ] if archived: tail = tail or [Separator(" ")] # The tally rides the row muted, as a status atom rather than as part of the # verb: it is what the row leads to, not what the row does. - label = marked_label("πŸ“‚", "View archived contacts", "") + label = marked_label("πŸ“‚", "View archived contacts", "", lane=lane) label.append(f" Β· {archived}", style="muted") tail.append(Choice(title=label, value=_ARCHIVED)) super().__init__( diff --git a/meshterm/ui/menus.py b/meshterm/ui/menus.py index 945341f..cd0f307 100644 --- a/meshterm/ui/menus.py +++ b/meshterm/ui/menus.py @@ -85,7 +85,55 @@ def command_icon(icon: str) -> str: return glyph(icon) if get_platform().menu_icons else "" -def marked_label(icon: str, label: str, style: str) -> Text: +def icon_lane(icons: Iterable[str]) -> int: + """THE icon column's width in cells: the widest mark among ``icons`` on this platform. + + A list's icons are **not all the same width**, and the terminal is the authority on + which are which: of the app's lexicon, ten (``πŸ—‘ ✎ βš™ β–Ά β˜… ↻ ↕ ⇄ ⌨ #``) draw one cell + and the rest two. A row that simply wrote ``icon + " "`` therefore started its label a + column left of its two-cell siblings β€” which is what put ``πŸ—‘ Delete contact…`` out of + line under ``πŸ’Ύ Archive contact`` on the node page (JP, 2026-09-01). So a list declares + the icons it uses, measures the column once, and pads every mark out to it. + + Measured rather than hard-coded because the marks themselves change with the platform: + the PicoCalc draws no icon lane on a command row at all (see :func:`command_icon`), so + the column measures zero and the labels take back the cells β€” the same alignment rule + wherever the icons land. + + Args: + icons: Every mark the list's rows can lead with, as written on the regular platform. + + Returns: + The column width in cells, or ``0`` where this platform draws no icons (and for an + empty set, which is the same thing to a caller). + """ + return max((cell_len(command_icon(icon)) for icon in icons if icon), default=0) + + +def icon_mark(icon: str, style: str, lane: int) -> Text: + """One row's mark, tinted, padded to ``lane`` cells, its trailing space included. + + The single place a command row's icon column is written, so a mark one cell narrower + than its neighbours cannot shift the label after it. An emptied lane (``lane`` of + ``0``) appends nothing at all, separator included β€” the cells belong to the label. + + Args: + icon: The row's icon, as written on the regular platform. + style: The theme style the mark is drawn in. + lane: The column width from :func:`icon_lane`. + + Returns: + The padded mark, or empty :class:`~rich.text.Text` where there is no lane. + """ + mark = command_icon(icon) if icon else "" + if not lane or not mark: + return Text() + text = Text(mark, style=style) + text.append(" " * (lane - cell_len(mark) + 1)) + return text + + +def marked_label(icon: str, label: str, style: str, *, lane: Optional[int] = None) -> Text: """A command row whose icon carries a tint β€” the tint moving to the label if it goes. The app marks a destructive command by tinting its icon, not its words (``πŸ—‘`` in @@ -98,12 +146,18 @@ def marked_label(icon: str, label: str, style: str) -> Text: icon: The row's icon, as written on the regular platform. label: The row's words, with no icon and no leading space. style: The theme style the mark (or, iconless, the label) is drawn in. + lane: The icon column's width in cells, from :func:`icon_lane` over every icon the + surrounding list uses. **Pass it whenever the list mixes icon widths**, or the + narrow ones start their labels a column early. ``None`` (the default) measures + this icon alone, which is right for a list whose rows all lead with the same + mark, and for the lone action row. Returns: The composed row label. """ - mark = command_icon(icon) - return Text.assemble((f"{mark} ", style), label) if mark else Text(label, style=style) + width = icon_lane((icon,)) if lane is None else lane + mark = icon_mark(icon, style, width) + return Text.assemble(mark, label) if mark.plain else Text(label, style=style) def command_label(label: LabelT) -> LabelT: diff --git a/meshterm/ui/node_detail_screen.py b/meshterm/ui/node_detail_screen.py index c14647c..352cf61 100644 --- a/meshterm/ui/node_detail_screen.py +++ b/meshterm/ui/node_detail_screen.py @@ -88,7 +88,7 @@ from ..core.geo import EARTH_RADIUS_KM, usable_fix from ..core.models import NODE_TYPE_LABELS, Contact, utcnow from ..platforms import Platform, on_platform from .mapcanvas import RGB -from .menus import command_icon +from . import menus from .minimap import MiniMap from .pathgraph import ( DST_NODE, @@ -447,6 +447,13 @@ class NodeDetailScreen(Screen): self._routes = routes self._info_actions = info_actions or [] self._trace_action = trace_action + #: The action rows' icon column, in cells β€” measured once over every mark this page + #: can draw, so a one-cell ``πŸ—‘`` pads out to its two-cell siblings and every label + #: starts in the same column. Zero on a platform that draws no icon lane at all. + self._icon_lane = menus.icon_lane( + action.glyph + for action in (*self._info_actions, *( [trace_action] if trace_action else [] )) + ) self._tab_index = 0 self._row_index = 0 #: The highlighted route on the Routes tab (drives the graph); tracks the cursor as @@ -1043,10 +1050,13 @@ class NodeDetailScreen(Screen): # destructive row announces itself β€” so where the lane goes, the tint lands on the # label instead. Same move as :func:`~meshterm.ui.menus.marked_label` makes for a # menu row, for the same reason: a delete must not read like any other action. - mark = command_icon(action.glyph) if action.glyph else "" - if mark: - text.append(f"{mark} ", style=action.glyph_style) - text.append(action.label, style="" if mark else action.glyph_style) + # + # Padded to the page's own lane rather than written as `icon + " "`: this page mixes + # a one-cell πŸ—‘ with two-cell siblings, and the unpadded form started its label a + # column early (JP, 2026-09-01). + mark = menus.icon_mark(action.glyph, action.glyph_style, self._icon_lane) + text.append_text(mark) + text.append(action.label, style="" if mark.plain else action.glyph_style) if selected: text.style = "cursor" text.no_wrap = True diff --git a/meshterm/ui/trace_screen.py b/meshterm/ui/trace_screen.py index f3d66bb..b577c97 100644 --- a/meshterm/ui/trace_screen.py +++ b/meshterm/ui/trace_screen.py @@ -96,7 +96,7 @@ from ..services import trace_runner from ..services.records import first_repeated_edge from ..services.topology import render_forced_spec from .braillechart import meter -from .menus import command_icon, marked_label, section_heading +from .menus import icon_lane, icon_mark, marked_label, section_heading from .theme import snr_style from .tui.render import render_lines, render_to_ansi from .tui.screen import ListWindow, Screen @@ -172,28 +172,13 @@ _ACTION_ICONS = ("✎", "⇄", "⚑", "βš™", "#", "β–Ά") def _icon_lane() -> int: - """The action rows' icon column, in cells: the widest mark on this platform. - - Measured rather than hard-coded because the marks themselves change with the - platform: PicoCalc draws no icon lane on a command row at all - (:func:`~meshterm.ui.menus.command_icon`), so the column measures zero and the labels - take the cells β€” the same alignment rule wherever the icons land. - """ - return max(cell_len(command_icon(icon)) for icon in _ACTION_ICONS) + """The action rows' icon column, in cells β€” this screen's own marks, measured once.""" + return icon_lane(_ACTION_ICONS) def _icon(text: Text, icon: str, style: str) -> None: - """Append an action row's mark in the icon column, its trailing space included. - - A one-cell mark pads out to :func:`_icon_lane` so every label starts in the same - column; an emptied lane (see there) appends nothing, separator included. - """ - lane = _icon_lane() - if not lane: - return - mark = command_icon(icon) - text.append(mark, style=style) - text.append(" " * (lane - cell_len(mark) + 1)) + """Append an action row's mark in the icon column, its trailing space included.""" + text.append_text(icon_mark(icon, style, _icon_lane())) def _lane_lines(label: str, value: list[Text], width: int) -> list[str]: diff --git a/meshterm/ui/tx_screen.py b/meshterm/ui/tx_screen.py index 77d2547..8d57520 100644 --- a/meshterm/ui/tx_screen.py +++ b/meshterm/ui/tx_screen.py @@ -33,7 +33,6 @@ import asyncio import re from typing import TYPE_CHECKING, Any, Callable, Optional -from rich.cells import cell_len from rich.console import Group, RenderableType from rich.table import Table from rich.text import Text @@ -42,7 +41,7 @@ from ..core.connection import REMOTE_TX_MAX, REMOTE_TX_MIN from ..core.models import Contact, LoginResult, TraceResult, TxLevelResult, TxOptResult from ..services import trace_runner, tx_optimizer from ..services.topology import build_topology, render_custom_spec -from .menus import command_icon +from .menus import icon_lane, icon_mark from .pathline import SELF_GLYPH, PathHop, PathLine, cut_to, path_line from .theme import name_style, snr_style from .trace_screen import TracingDialog, _collapse_trace_width, snr_bar @@ -76,18 +75,13 @@ _ACTION_ICONS = ("✎", "βš™", "#", "β–Ά", "β˜…") def _icon_lane() -> int: - """The action rows' icon column, in cells: the widest mark on this platform.""" - return max(cell_len(command_icon(icon)) for icon in _ACTION_ICONS) + """The action rows' icon column, in cells β€” this screen's own marks, measured once.""" + return icon_lane(_ACTION_ICONS) def _icon(text: Text, icon: str, style: str) -> None: """Append an action row's mark in the icon column, its trailing space included.""" - lane = _icon_lane() - if not lane: - return - mark = command_icon(icon) - text.append(mark, style=style) - text.append(" " * (lane - cell_len(mark) + 1)) + text.append_text(icon_mark(icon, style, _icon_lane())) class TxSweepScreen(Screen): diff --git a/tests/test_menus.py b/tests/test_menus.py index 4410a2a..65b4db3 100644 --- a/tests/test_menus.py +++ b/tests/test_menus.py @@ -7,6 +7,7 @@ regression here fails once rather than on every screen. from __future__ import annotations +import pytest from rich.text import Text from meshterm.ui.menus import ( @@ -15,7 +16,10 @@ from meshterm.ui.menus import ( column_header, exit_rows, fit_cells, + icon_lane, + icon_mark, lane_header, + marked_label, menu_rows, section_heading, ) @@ -168,3 +172,79 @@ def test_main_menu_sections_answer_the_menus_own_question() -> None: # No section is so big it stops being a grouping (the old Mesh bucket held seven of # nineteen rows), and none is a bucket of one. assert all(2 <= len(names) <= 5 for names in sections.values()) + + +#: Two lexicon icons the terminal genuinely draws in different widths β€” the whole reason +#: the icon column has to be measured. Of the app's icons, ten (``πŸ—‘ ✎ βš™ β–Ά β˜… ↻ ↕ ⇄ ⌨ #``) +#: draw one cell and the rest two. +_NARROW, _WIDE = "πŸ—‘", "πŸ“‚" + + +def test_the_icon_column_is_the_widest_mark_a_list_can_draw() -> None: + """A list declares its icons and the column measures them; an empty set has no column.""" + from rich.cells import cell_len + + assert cell_len(_NARROW) == 1 and cell_len(_WIDE) == 2, "the premise of this whole lane" + assert icon_lane((_NARROW,)) == 1 # a list of only narrow marks keeps a narrow column + assert icon_lane((_WIDE,)) == 2 + assert icon_lane((_NARROW, _WIDE)) == 2 # a mixed list pads up to its widest + assert icon_lane(()) == 0 + assert icon_lane(("",)) == 0 # a row with no icon contributes no column + + +def test_a_narrow_mark_pads_out_to_its_wider_siblings() -> None: + """THE fix for a row whose label started a column early (JP, 2026-09-01). + + ``πŸ—‘ Delete contact…`` sat one cell left of ``πŸ’Ύ Archive contact`` because the row wrote + ``icon + " "`` and the terminal draws the two icons in different widths. Both marks now + occupy the same number of *cells*, which is the only measure the terminal cares about β€” + the character counts still differ, and asserting on those is what hid this. + """ + from rich.cells import cell_len + + lane = icon_lane((_NARROW, _WIDE)) + narrow = icon_mark(_NARROW, "err", lane) + wide = icon_mark(_WIDE, "", lane) + assert cell_len(narrow.plain) == cell_len(wide.plain) == lane + 1 + assert len(narrow.plain) != len(wide.plain), "cells, not characters, are the measure" + + +def test_marked_label_lines_up_a_mixed_list_when_told_its_lane() -> None: + """Labels start in the same cell once the caller passes the list's measured column.""" + from rich.cells import cell_len + + lane = icon_lane((_NARROW, _WIDE)) + rows = [ + marked_label(_NARROW, "Purge contacts…", "err", lane=lane), + marked_label(_WIDE, "View archived contacts", "", lane=lane), + ] + starts = {cell_len(row.plain[: row.plain.index(word)]) + for row, word in zip(rows, ("Purge", "View"))} + assert starts == {lane + 1} + + # Without the lane each mark measures itself, which is right for a list whose rows all + # lead with the same icon β€” and is exactly what misaligns a mixed one. + solo = [marked_label(_NARROW, "A", "err"), marked_label(_WIDE, "B", "")] + assert len({cell_len(row.plain[: row.plain.index(letter)]) + for row, letter in zip(solo, ("A", "B"))}) == 2 + + +def test_an_iconless_platform_collapses_the_column_and_keeps_the_tint( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """No icon lane means no cells spent and no separator β€” and the tint moves to the words. + + A destructive row announces itself by its red mark; drop the mark and the claim has to + land somewhere, or a delete reads like any other action. + """ + from meshterm.platforms import PICOCALC, REGULAR, set_platform + + set_platform(PICOCALC) + try: + assert icon_lane((_NARROW, _WIDE)) == 0 + assert icon_mark(_NARROW, "err", 0).plain == "" + row = marked_label(_NARROW, "Delete contact…", "err", lane=0) + assert row.plain == "Delete contact…" + assert any(span.style == "err" for span in row.spans) or row.style == "err" + finally: + set_platform(REGULAR)