Fix rendering of control whitespace in completion items (#1622)

Author: kmvanbrunt

cmd2/argparse_completer.py

@@ -33,7 +33,6 @@
 from .completion import (
     CompletionItem,
     Completions,
-    all_display_numeric,
 )
 from .constants import INFINITY
 from .exceptions import CompletionError
@@ -647,12 +646,15 @@ def _build_completion_table(self, arg_state: _ArgumentState, completions: Comple
             # the 3rd or more argument here.
             destination = destination[min(len(destination) - 1, arg_state.count)]
 
-        # Determine if all display values are numeric so we can right-align them
-        all_nums = all_display_numeric(completions.items)
-
         # Build header row
         rich_columns: list[Column] = []
-        rich_columns.append(Column(destination.upper(), justify="right" if all_nums else "left", no_wrap=True))
+        rich_columns.append(
+            Column(
+                destination.upper(),
+                justify="right" if completions.numeric_display else "left",
+                no_wrap=True,
+            )
+        )
         rich_columns.extend(
             column if isinstance(column, Column) else Column(column, overflow="fold") for column in table_columns
         )

cmd2/completion.py

@@ -3,7 +3,6 @@
 import re
 import sys
 from collections.abc import (
-    Collection,
     Iterable,
     Iterator,
     Sequence,
@@ -31,26 +30,15 @@
 
 from . import rich_utils as ru
 
-# Regular expression to identify strings which we should sort numerically
-NUMERIC_RE = re.compile(
-    r"""
-    ^              # Start of string
-    [-+]?          # Optional sign
-    (?:            # Start of non-capturing group
-        \d+\.?\d*  # Matches 123 or 123. or 123.45
-        |          # OR
-        \.\d+      # Matches .45
-    )              # End of group
-    $              # End of string
-""",
-    re.VERBOSE,
-)
-
 
 @dataclass(frozen=True, slots=True, kw_only=True)
 class CompletionItem:
     """A single completion result."""
 
+    # Regular expression to identify whitespace characters that are rendered as
+    # control sequences (like ^J or ^I) in the completion menu.
+    _CONTROL_WHITESPACE_RE = re.compile(r'\r\n|[\n\r\t\f\v]')
+
     # The underlying object this completion represents (e.g., str, int, Path).
     # This is used to support argparse choices validation.
     value: Any = field(kw_only=False)
@@ -76,6 +64,18 @@ class CompletionItem:
     display_plain: str = field(init=False)
     display_meta_plain: str = field(init=False)
 
+    @classmethod
+    def _clean_display(cls, val: str) -> str:
+        """Clean a string for display in the completion menu.
+
+        This replaces whitespace characters that are rendered as
+        control sequences (like ^J or ^I) with spaces.
+
+        :param val: string to be cleaned
+        :return: the cleaned string
+        """
+        return cls._CONTROL_WHITESPACE_RE.sub(' ', val)
+
     def __post_init__(self) -> None:
         """Finalize the object after initialization."""
         # Derive text from value if it wasn't explicitly provided
@@ -86,7 +86,11 @@ def __post_init__(self) -> None:
         if not self.display:
             object.__setattr__(self, "display", self.text)
 
-        # Pre-calculate plain text versions by stripping ANSI sequences.
+        # Clean display and display_meta
+        object.__setattr__(self, "display", self._clean_display(self.display))
+        object.__setattr__(self, "display_meta", self._clean_display(self.display_meta))
+
+        # Create plain text versions by stripping ANSI sequences.
         # These are stored as attributes for fast access during sorting/filtering.
         object.__setattr__(self, "display_plain", su.strip_style(self.display))
         object.__setattr__(self, "display_meta_plain", su.strip_style(self.display_meta))
@@ -135,20 +139,44 @@ def __hash__(self) -> int:
 class CompletionResultsBase:
     """Base class for results containing a collection of CompletionItems."""
 
+    # Regular expression to identify strings that we should sort numerically
+    _NUMERIC_RE = re.compile(
+        r"""
+        ^              # Start of string
+        [-+]?          # Optional sign
+        (?:            # Start of non-capturing group
+            \d+\.?\d*  # Matches 123 or 123. or 123.45
+            |          # OR
+            \.\d+      # Matches .45
+        )              # End of group
+        $              # End of string
+    """,
+        re.VERBOSE,
+    )
+
     # The collection of CompletionItems. This is stored internally as a tuple.
     items: Sequence[CompletionItem] = field(default_factory=tuple, kw_only=False)
 
     # If True, indicates the items are already provided in the desired display order.
     # If False, items will be sorted by their display value during initialization.
     is_sorted: bool = False
 
+    # True if every item in this collection has a numeric display string.
+    # Used for sorting and alignment.
+    numeric_display: bool = field(init=False)
+
     def __post_init__(self) -> None:
         """Finalize the object after initialization."""
         from . import utils
 
         unique_items = utils.remove_duplicates(self.items)
+
+        # Determine if all items have numeric display strings
+        numeric_display = bool(unique_items) and all(self._NUMERIC_RE.match(i.display_plain) for i in unique_items)
+        object.__setattr__(self, "numeric_display", numeric_display)
+
         if not self.is_sorted:
-            if all_display_numeric(unique_items):
+            if self.numeric_display:
                 # Sort numerically
                 unique_items.sort(key=lambda item: float(item.display_plain))
             else:
@@ -249,8 +277,3 @@ class Completions(CompletionResultsBase):
 
     # The quote character to use if adding an opening or closing quote to the matches.
     _quote_char: str = ""
-
-
-def all_display_numeric(items: Collection[CompletionItem]) -> bool:
-    """Return True if items is non-empty and every item.display_plain value is a numeric string."""
-    return bool(items) and all(NUMERIC_RE.match(item.display_plain) for item in items)

tests/test_completion.py

@@ -19,7 +19,6 @@
     Completions,
     utils,
 )
-from cmd2.completion import all_display_numeric
 
 from .conftest import (
     normalize,
@@ -877,7 +876,7 @@ def test_is_sorted() -> None:
 
 
 @pytest.mark.parametrize(
-    ('values', 'all_nums'),
+    ('values', 'numeric_display'),
     [
         ([2, 3], True),
         ([2, 3.7], True),
@@ -889,11 +888,10 @@ def test_is_sorted() -> None:
         (["\x1b[31mNOT_STRING\x1b[0m", "\x1b[32m9.2\x1b[0m"], False),
     ],
 )
-def test_all_display_numeric(values: list[int | float | str], all_nums: bool) -> None:
-    """Test that all_display_numeric() evaluates the display_plain field."""
-
-    items = [CompletionItem(v) for v in values]
-    assert all_display_numeric(items) == all_nums
+def test_numeric_display(values: list[int | float | str], numeric_display: bool) -> None:
+    """Test setting of the Completions.numeric_display field."""
+    completions = Completions.from_values(values)
+    assert completions.numeric_display == numeric_display
 
 
 def test_remove_duplicates() -> None:
@@ -932,6 +930,25 @@ def test_plain_fields() -> None:
     assert completion_item.display_meta_plain == "A tasty apple"
 
 
+def test_clean_display() -> None:
+    """Test display string cleaning in CompletionItem."""
+    # Test all problematic characters being replaced by a single space.
+    # Also verify that \r\n is replaced by a single space.
+    display = "str1\r\nstr2\nstr3\rstr4\tstr5\fstr6\vstr7"
+    expected = "str1 str2 str3 str4 str5 str6 str7"
+
+    # Since display defaults to text if not provided, we test both text and display fields
+    completion_item = CompletionItem("item", display=display, display_meta=display)
+    assert completion_item.display == expected
+    assert completion_item.display_meta == expected
+
+    # Verify that text derived display is also sanitized
+    text = "item\nwith\nnewlines"
+    expected_text_display = "item with newlines"
+    completion_item = CompletionItem(text)
+    assert completion_item.display == expected_text_display
+
+
 def test_styled_completion_sort() -> None:
     """Test that sorting is done with the display_plain field."""