-
Notifications
You must be signed in to change notification settings - Fork 0
fix: clamp ColumnsPrinter table width to terminal and truncate overflowing cells #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| from functools import wraps | ||
| from typing import Any, Callable, Dict, Iterable, List, Optional, Sequence, Tuple, Union | ||
|
|
||
| from .drawing import iter_display_atoms as _iter_display_atoms | ||
| from .drawing import strip_ansi as _strip_ansi | ||
| from .drawing import visible_length as _visible_length | ||
| from .terminal import terminal_size | ||
|
|
@@ -24,6 +25,42 @@ def _pad_visible(text: str, target: int) -> str: | |
| return f"{text}{' ' * (target - current)}" | ||
|
|
||
|
|
||
| def _fit_visible(text: str, target: int, placeholder: str = "...") -> str: | ||
| """Pad or truncate `text` so its rendered width is exactly `target`.""" | ||
|
|
||
| if target <= 0: | ||
| return "" | ||
|
|
||
| current = _visible_length(text) | ||
| if current < target: | ||
| return f"{text}{' ' * (target - current)}" | ||
| if current == target: | ||
| return text | ||
|
|
||
| result: List[str] = [] | ||
| width = 0 | ||
| ph_width = _visible_length(placeholder) | ||
| budget = target - ph_width if target >= ph_width else target | ||
|
|
||
| for is_ansi, token, token_width in _iter_display_atoms(text): | ||
| if is_ansi: | ||
| result.append(token) | ||
| continue | ||
| if width + token_width > budget: | ||
| break | ||
| result.append(token) | ||
| width += token_width | ||
|
|
||
| if target >= ph_width: | ||
| result.append(placeholder) | ||
|
|
||
| # Stop any active ANSI style from bleeding into trailing padding. | ||
| if "\x1b" in text: | ||
| result.append("\033[0m") | ||
|
|
||
| return "".join(result) | ||
|
|
||
|
|
||
| def _columns_locked(method): | ||
| """Serialize one `ColumnsPrinter` API call through its instance lock.""" | ||
|
|
||
|
|
@@ -109,6 +146,8 @@ def __init__( | |
| self._terminal_size = terminal_size_provider or terminal_size | ||
| self._level_proxies: Dict[Any, ColumnsPrinter._LevelProxy] = {} | ||
| self._lock = threading.RLock() | ||
| self._slot_min_widths: List[int] = [] | ||
| self._fixed_slots: set = set() | ||
|
|
||
| if headers: | ||
| self._set_columns(headers) | ||
|
|
@@ -321,6 +360,13 @@ def _recompute_layout(self) -> None: | |
| elif len(self._slot_padding) > slot_count: | ||
| self._slot_padding = self._slot_padding[:slot_count] | ||
|
|
||
| if len(self._slot_min_widths) < slot_count: | ||
| self._slot_min_widths.extend([1] * (slot_count - len(self._slot_min_widths))) | ||
| elif len(self._slot_min_widths) > slot_count: | ||
| self._slot_min_widths = self._slot_min_widths[:slot_count] | ||
|
|
||
| self._fixed_slots = set() | ||
|
|
||
| def _parse_width_hint(self, value: Optional[Union[str, int, float]]) -> Optional[Tuple[str, float]]: | ||
| """Normalize width hints such as `fit`, percentages, or fixed chars.""" | ||
|
|
||
|
|
@@ -406,7 +452,9 @@ def _get_target_width(self) -> int: | |
| minimal_cells = minimal_total | ||
| target_cells = target_total | ||
|
|
||
| return max(target_cells, minimal_cells) | ||
| desired = max(target_cells, minimal_cells) | ||
| content_budget = max(0, available_total - separator_count) | ||
| return min(desired, content_budget) | ||
|
Comment on lines
+459
to
+461
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 _get_target_width now clamps below minimal header width The new Was this helpful? React with 👍 or 👎 to provide feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed as intentional. The whole point of this PR is to prevent headers from forcing the table wider than the terminal, so headers are now truncated with |
||
|
|
||
| def _apply_initial_widths(self) -> None: | ||
| """Seed slot widths from hints, then distribute any remaining space.""" | ||
|
|
@@ -418,26 +466,33 @@ def _apply_initial_widths(self) -> None: | |
| self._slot_widths = [1] * slot_count | ||
| self._slot_padding = [self._padding] * slot_count | ||
|
|
||
| total_width = self._get_target_width() | ||
| max_content = self._get_target_width() | ||
| column_count = len(self._columns) | ||
|
|
||
| for col_idx, spec in enumerate(self._columns): | ||
| if spec.width is None: | ||
| continue | ||
| if spec.width[0] == "fit": | ||
| continue | ||
| target = self._resolve_width_hint(spec.width, total_width) | ||
| target = self._resolve_width_hint(spec.width, max_content) | ||
| self._configure_column_width(col_idx, target) | ||
|
|
||
| current_total = sum(self._column_total_width(idx) for idx in range(column_count)) | ||
| has_percent = any(spec.width and spec.width[0] == "percent" for spec in self._columns) | ||
| if current_total > total_width: | ||
| total_width = current_total | ||
|
|
||
| if not self._target_width_hint and not has_percent: | ||
| total_width = current_total | ||
| if self._target_width_hint or has_percent: | ||
| desired_content = max_content | ||
| else: | ||
| desired_content = min(current_total, max_content) | ||
|
|
||
| if current_total > desired_content: | ||
| full_width = desired_content + (slot_count + 1) | ||
| self._clamp_total_width(full_width) | ||
| current_total = sum(self._column_total_width(idx) for idx in range(column_count)) | ||
| remaining = 0 | ||
| else: | ||
| remaining = max(0, desired_content - current_total) | ||
|
|
||
| remaining = max(0, total_width - current_total) | ||
| expandable = [idx for idx, spec in enumerate(self._columns) if spec.width is None] | ||
| if not expandable: | ||
| expandable = [ | ||
|
|
@@ -446,25 +501,23 @@ def _apply_initial_widths(self) -> None: | |
| if spec.width is None or (spec.width and spec.width[0] != "fit") | ||
| ] | ||
|
|
||
| if remaining > 0 and expandable: | ||
| while remaining > 0: | ||
| progressed = False | ||
| for col_idx in expandable: | ||
| if remaining <= 0: | ||
| break | ||
| spec = self._columns[col_idx] | ||
| if spec.width and spec.width[0] == "fit": | ||
| continue | ||
| self._grow_column(col_idx, 1) | ||
| remaining -= 1 | ||
| progressed = True | ||
| if not progressed: | ||
| while remaining > 0: | ||
| progressed = False | ||
| for col_idx in expandable: | ||
| if remaining <= 0: | ||
| break | ||
| spec = self._columns[col_idx] | ||
| if spec.width and spec.width[0] == "fit": | ||
| continue | ||
| self._grow_column(col_idx, 1) | ||
| remaining -= 1 | ||
| progressed = True | ||
| if not progressed: | ||
| break | ||
|
|
||
| slot_count = self._slot_count() | ||
| separator_count = slot_count + 1 if slot_count else 0 | ||
| current_total = sum(self._column_total_width(idx) for idx in range(column_count)) | ||
| self._current_total_width = current_total + separator_count | ||
| full_width = max_content + (slot_count + 1) | ||
| self._clamp_total_width(full_width) | ||
| self._update_current_total_width() | ||
|
|
||
| def _resolve_width_hint(self, hint: Optional[Tuple[str, float]], total_width: int) -> Optional[int]: | ||
| """Convert a parsed width hint into an absolute character width.""" | ||
|
|
@@ -503,6 +556,10 @@ def _configure_column_width(self, col_idx: int, target: Optional[int]) -> None: | |
| if extra <= 0: | ||
| break | ||
|
|
||
| for idx in slot_indices: | ||
| self._slot_min_widths[idx] = self._slot_widths[idx] | ||
| self._fixed_slots.add(idx) | ||
|
|
||
| def _grow_column(self, col_idx: int, amount: int) -> None: | ||
| """Expand a column span by distributing width across its slots.""" | ||
|
|
||
|
|
@@ -534,6 +591,112 @@ def _column_total_width(self, col_idx: int) -> int: | |
| total += max(0, span - 1) | ||
| return total | ||
|
|
||
| def _row_width_budget(self) -> int: | ||
| """Return the maximum bordered row width that fits in the terminal.""" | ||
|
|
||
| term_cols, _ = self._terminal_size() | ||
| if term_cols <= 0: | ||
| term_cols = 80 | ||
| return max(0, term_cols - (self._level_max_length + 2)) | ||
|
|
||
| def _clamp_total_width(self, max_total: Optional[int] = None) -> None: | ||
| """Reduce slot widths so the full bordered row does not exceed `max_total`.""" | ||
|
|
||
| slot_count = self._slot_count() | ||
| if slot_count == 0: | ||
| return | ||
|
|
||
| if max_total is None: | ||
| max_total = self._row_width_budget() | ||
| if max_total <= 0: | ||
| return | ||
|
|
||
| separators = slot_count + 1 | ||
| padding_total = sum((self._slot_padding[idx] * 2) for idx in range(slot_count)) | ||
| max_content = max(0, max_total - separators - padding_total) | ||
| total = sum(self._slot_widths) | ||
| if total <= max_content: | ||
| self._update_current_total_width() | ||
| return | ||
|
|
||
| min_widths = self._slot_min_widths | ||
|
|
||
| while total > max_content: | ||
| reducible = [i for i in range(slot_count) if self._slot_widths[i] > min_widths[i]] | ||
| if not reducible: | ||
| break | ||
|
|
||
| max_w = max(self._slot_widths[i] for i in reducible) | ||
| count_max = sum(1 for i in reducible if self._slot_widths[i] == max_w) | ||
| next_candidates = [self._slot_widths[i] for i in reducible if self._slot_widths[i] < max_w] | ||
| min_at_max = max(min_widths[i] for i in reducible if self._slot_widths[i] == max_w) | ||
| next_candidates.append(min_at_max) | ||
| next_w = max(next_candidates) | ||
|
|
||
| available = (max_w - next_w) * count_max | ||
| need = total - max_content | ||
| reduce_total = min(available, need) | ||
| if reduce_total <= 0: | ||
| break | ||
|
|
||
| per_slot = reduce_total // count_max | ||
| extra = reduce_total % count_max | ||
| for idx in reducible: | ||
| if self._slot_widths[idx] == max_w: | ||
| self._slot_widths[idx] -= per_slot | ||
| if extra > 0: | ||
| self._slot_widths[idx] -= 1 | ||
| extra -= 1 | ||
| if self._slot_widths[idx] < min_widths[idx]: | ||
| self._slot_widths[idx] = min_widths[idx] | ||
| total = sum(self._slot_widths) | ||
|
|
||
| if total > max_content: | ||
| # Fallback: reduce padding and scale proportionally when minimums are infeasible. | ||
| room = max_total - separators - slot_count | ||
| new_pad = 0 | ||
| if room > 0: | ||
| new_pad = min(self._padding, room // (2 * slot_count)) | ||
| for idx in range(slot_count): | ||
| self._slot_padding[idx] = new_pad | ||
|
|
||
| padding_total = 2 * new_pad * slot_count | ||
| max_content = max(0, max_total - separators - padding_total) | ||
|
|
||
| if max_content < slot_count: | ||
| for idx in range(slot_count): | ||
| self._slot_widths[idx] = 1 | ||
| total = slot_count | ||
| else: | ||
| factor = max_content / total | ||
| scaled = [max(1, int(self._slot_widths[idx] * factor)) for idx in range(slot_count)] | ||
| leftover = max_content - sum(scaled) | ||
| if leftover > 0: | ||
| fracs = sorted( | ||
| ( | ||
| (self._slot_widths[idx] * factor) - int(self._slot_widths[idx] * factor), | ||
| idx, | ||
| ) | ||
| for idx in range(slot_count) | ||
| ) | ||
| for _, idx in fracs[:leftover]: | ||
| scaled[idx] += 1 | ||
| self._slot_widths = scaled | ||
| total = sum(self._slot_widths) | ||
|
|
||
| self._update_current_total_width() | ||
|
|
||
| def _update_current_total_width(self) -> None: | ||
| """Store the full bordered width implied by the current slot widths.""" | ||
|
|
||
| slot_count = self._slot_count() | ||
| if slot_count == 0: | ||
| self._current_total_width = 0 | ||
| return | ||
|
|
||
| current_total = sum(self._column_total_width(idx) for idx in range(len(self._columns))) | ||
| self._current_total_width = current_total + (slot_count + 1) | ||
|
|
||
| def _slot_count(self) -> int: | ||
| """Return the number of physical slots in the current layout.""" | ||
|
|
||
|
|
@@ -568,6 +731,8 @@ def _apply_header_widths(self) -> None: | |
| continue | ||
| if start >= len(self._slot_widths): | ||
| continue | ||
| if start in self._fixed_slots: | ||
| continue | ||
|
|
||
| total_slot_width = 0 | ||
| for offset in range(span): | ||
|
|
@@ -588,6 +753,9 @@ def _apply_header_widths(self) -> None: | |
| if inner_width < label_len: | ||
| deficit = label_len - inner_width | ||
| self._slot_widths[start] += deficit | ||
| self._slot_min_widths[start] = max(self._slot_min_widths[start], label_len) | ||
|
|
||
| self._clamp_total_width() | ||
|
|
||
| def _prepare_values(self, values: Iterable) -> List[str]: | ||
| """Normalize row values to strings and match the active slot count.""" | ||
|
|
@@ -607,10 +775,14 @@ def _update_slot_widths(self, values: Iterable[str]) -> None: | |
| for idx, value in enumerate(values): | ||
| if idx >= len(self._slot_widths): | ||
| break | ||
| if idx in self._fixed_slots: | ||
| continue | ||
| current = _visible_length(value) | ||
| if current > self._slot_widths[idx]: | ||
| self._slot_widths[idx] = current | ||
|
|
||
| self._clamp_total_width() | ||
|
|
||
| def _render_header(self) -> str: | ||
| """Build the visible header row with current widths and padding.""" | ||
|
|
||
|
|
@@ -628,7 +800,7 @@ def _render_header(self) -> str: | |
| inner_width = max(0, total_width - left_pad_val - right_pad_val) | ||
| pad_left = " " * left_pad_val | ||
| pad_right = " " * right_pad_val | ||
| content = _pad_visible(spec.label, inner_width) | ||
| content = _fit_visible(spec.label, inner_width) | ||
| cells.append(f"{pad_left}{content}{pad_right}") | ||
|
|
||
| return "|" + "|".join(cells) + "|" | ||
|
|
@@ -648,7 +820,7 @@ def _render_row(self, values: Iterable[str]) -> str: | |
| width = _visible_length(text) | ||
| pad_width = self._slot_padding[idx] if idx < len(self._slot_padding) else self._padding | ||
| pad = " " * pad_width | ||
| padded_text = _pad_visible(text, width) | ||
| padded_text = _fit_visible(text, width) | ||
| cell = f"{pad}{padded_text}{pad}" | ||
| cells.append(cell) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Truncated cells containing wide characters come out one column too narrow, misaligning the table
When a cell value is shortened to fit its column (
_fit_visibleatlogbar/columns.py:40-61) and the text contains a full-width character (e.g. CJK or emoji) right at the truncation boundary, the truncated text plus the...marker ends up one column short of the intended width, so that row is drawn narrower than its border and the table stops lining up.Impact: Rows that hold wide (double-width) characters render slightly misaligned against the surrounding borders, breaking the clean grid the fix is meant to guarantee.
Why the width falls short during truncation
In the truncation path,
budget = target - ph_widthand the loop stops adding clusters once the next cluster would exceedbudget(logbar/columns.py:49). A double-width cluster (width 2) can leave the accumulatedwidthatbudget - 1because adding it would overflow. The code then appends the placeholder (ph_width) giving a total visible width oftarget - 1, but it never pads the shortfall. Compare with_pad_visible/thecurrent < targetbranch which always reaches exactlytarget. The border segment in_emit_border(logbar/columns.py:844-850) is sized fromself._slot_widths[idx](the fulltarget), so the content cell is one column narrower than its border. Single-width text always fillsbudgetexactly, so this only manifests with wide clusters.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed —
_fit_visiblenow pads after the placeholder so the returned string always matches the target visible width, even when a double-width character falls on the truncation boundary. Addedtest_columns_fit_visible_exact_width_for_wide_charsto cover CJK cells.