fix(ui): clamp the font metrics to the config's own range, and stop buckets mislabeling a hand-set value (#550) (#572)

* fix(ui): clamp the font metrics to the config's own range, and stop buckets mislabeling a hand-set value (#550)

The settings steppers and the Ctrl+=/Ctrl+- keys clamped font size to
6-48 and line height to 1.0-2.0, while `sanitize` allows 4-256 and
0.5-4.0. A value inside the config range but outside the GUI's got
pushed the wrong way by a single step — `font_size: 50` shrank to 48 on
"+" — and `set_font_size` writes the result back to the file, so one
misclick permanently changed a value it only meant to nudge. The bounds
move into tty7-core beside `sanitize` (the `ui_font_size` precedent),
one shared range for validation, the steppers, and the keyboard path.

The scrollback and notify-threshold preset rows had the matching
display bug: the highlight matched a *range*, so a hand-set 5000 lit up
"10,000" and 20s lit up "30s", and clicking that cell silently
overwrote the real value with the bucket's. The segmented control now
highlights a bucket only on an exact match and otherwise shows a
"Custom (N)" cell that names the live value and is not a button.

* fix(ui): name a custom preset the way the cells beside it are written

Review follow-up on #550. The "Custom (N)" cell rendered the raw integer, so
a documented `scrollback_limit: 50000` read "Custom (50000)" between cells
reading "10,000" and "100,000" — the one number on the row not written like a
count. It is grouped now, and the presets and their labels are one pair of
lists each, checked against each other, so a cell cannot come to show one
number and write another.

The bucket match moves out of the render bodies into `preset_choice`, which
is what makes the exact-match rule the issue asked for testable: the presets
the default lands on, the 50,000 the example config in
`docs/reference/configuration.mdx` carries, and 20s on the notify row.

The core test claimed to pin "the GUI steps within the range sanitize
allows", but only asserted that sanitize agrees with the constants it is
written in terms of — true by construction, and its line-height case took the
reset path rather than the clamp, so it passed without touching
LINE_HEIGHT_MIN at all. It now pins the published numbers themselves, the
clamp in both directions, and the two values the issue was reported with.

---------

Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com>
This commit is contained in:
Hongwei Qin
2026-08-13 10:08:45 +08:00
committed by GitHub
co-authored by l0ng-ai
parent 5dd60a6660
commit 8071eddb5b
8 changed files with 304 additions and 44 deletions
+11
View File
@@ -105,6 +105,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
a retired copy of the whole table behind for every keystroke to walk. A
config.json that does not parse keeps the keys it was already dispatching,
since the reload that fails never reaches the rebuild (#548).
- **The font-size and line-height steppers no longer push a hand-set value
the wrong way** — the settings steppers and the `Ctrl+=`/`Ctrl+-` keys
clamped to a narrower range (font 648, line height 1.02.0) than the
config file allows (4256, 0.54.0), so `font_size: 50` shrank to 48 on
"+" and wrote that back, permanently changing a value it only meant to
nudge. The steppers and `sanitize` now share one range, defined next to the
validation. The scrollback and notify-threshold preset rows got the matching
fix: a value between two buckets no longer lights up the nearest one
(`scrollback_limit: 5000` highlighted "10,000", and clicking that cell
silently overwrote it) — it shows a "Custom (5,000)" cell that names the
real value and is not a button (#550).
## [26.8.3] - 2026-08-12
+68 -2
View File
@@ -685,11 +685,11 @@ impl Config {
if !self.font_size.is_finite() || self.font_size <= 0.0 {
self.font_size = Config::default().font_size;
}
self.font_size = self.font_size.clamp(4.0, 256.0);
self.font_size = self.font_size.clamp(FONT_SIZE_MIN, FONT_SIZE_MAX);
if !self.line_height.is_finite() || self.line_height <= 0.0 {
self.line_height = Config::default().line_height;
}
self.line_height = self.line_height.clamp(0.5, 4.0);
self.line_height = self.line_height.clamp(LINE_HEIGHT_MIN, LINE_HEIGHT_MAX);
if !self.ui_font_size.is_finite() || self.ui_font_size <= 0.0 {
self.ui_font_size = default_ui_font_size();
}
@@ -1036,6 +1036,17 @@ pub const UI_FONT_SIZE_DEFAULT: f32 = 16.0;
pub const UI_FONT_SIZE_MIN: f32 = 12.0;
pub const UI_FONT_SIZE_MAX: f32 = 24.0;
/// The terminal's font-size and line-height bounds, shared by `sanitize` and
/// the GUI's steppers. The GUI used to keep its own, narrower pair (648,
/// 1.02.0), so a value inside the config range but outside the GUI's got
/// pushed the *wrong way* by a single step — `font_size: 50` shrank to 48 on
/// "+" — and written back to the file, permanently (#550). One range, defined
/// where the value is validated, is the `ui_font_size` precedent.
pub const FONT_SIZE_MIN: f32 = 4.0;
pub const FONT_SIZE_MAX: f32 = 256.0;
pub const LINE_HEIGHT_MIN: f32 = 0.5;
pub const LINE_HEIGHT_MAX: f32 = 4.0;
fn default_ui_font_size() -> f32 {
UI_FONT_SIZE_DEFAULT
}
@@ -1311,6 +1322,61 @@ mod tests {
assert_eq!(sanitized(15.0, 1.4), (15.0, 1.4));
}
#[test]
fn sanitize_clamps_to_the_same_bounds_the_gui_steps_within() {
// The GUI used to clamp to its own, narrower pair, so a config-legal
// value landed outside the stepper's range and one click pushed it the
// wrong way. The bounds are one shared set now: `Tty7App::set_font_size`
// and `set_line_height` clamp to these very constants, which is what
// makes the steppers and the Ctrl+=/Ctrl+- keys agree with the file
// (#550).
//
// The numbers themselves are the support surface
// `docs/reference/configuration.mdx` publishes, so pin them: narrowing
// either side is a documented behaviour change, not a refactor.
assert_eq!((FONT_SIZE_MIN, FONT_SIZE_MAX), (4.0, 256.0));
assert_eq!((LINE_HEIGHT_MIN, LINE_HEIGHT_MAX), (0.5, 4.0));
// A value at either edge survives sanitize unchanged, so the stepper
// has somewhere to stop rather than a value that keeps being rewritten.
let mut cfg = Config {
font_size: FONT_SIZE_MIN,
line_height: LINE_HEIGHT_MAX,
..Config::default()
};
cfg.sanitize();
assert_eq!(cfg.font_size, FONT_SIZE_MIN);
assert_eq!(cfg.line_height, LINE_HEIGHT_MAX);
// And a value past an edge lands *on* that edge — the direction the
// step was going — rather than anywhere else.
let mut cfg = Config {
font_size: 1_000.0,
line_height: 0.1,
..Config::default()
};
cfg.sanitize();
assert_eq!(cfg.font_size, FONT_SIZE_MAX, "over the top clamps down");
assert_eq!(
cfg.line_height, LINE_HEIGHT_MIN,
"under the floor clamps up"
);
// The values the issue was reported with: both are legal, so sanitize
// leaves them alone, and the steppers now step from where they are.
let mut cfg = Config {
font_size: 50.0,
line_height: 3.0,
..Config::default()
};
cfg.sanitize();
assert_eq!((cfg.font_size, cfg.line_height), (50.0, 3.0));
// Both are inside the range the steppers clamp to, which is the whole
// point: `50 + 1` and `3.0 - 0.05` are legal, so neither click can be
// turned around by a clamp.
assert!(50.0 + 1.0 <= FONT_SIZE_MAX && 3.0 - 0.05 >= LINE_HEIGHT_MIN);
}
#[test]
fn a_config_written_before_ui_font_size_existed_keeps_the_chrome_it_had() {
// The whole interface is laid out against this, so a missing field
+8 -4
View File
@@ -92,14 +92,18 @@ fn hsla_to_u32(color: gpui::Hsla) -> u32 {
(to(rgba.r) << 16) | (to(rgba.g) << 8) | to(rgba.b)
}
const FONT_SIZE_MIN: f32 = 6.0;
const FONT_SIZE_MAX: f32 = 48.0;
// The steppers clamp to the same range `sanitize` allows, defined in
// tty7-core next to the validation: a local, narrower pair used to push a
// config-legal value the wrong way — `font_size: 50` shrank to 48 on "+",
// and the result was written back to the file (#550).
pub(crate) use crate::core::config::{
FONT_SIZE_MAX, FONT_SIZE_MIN, LINE_HEIGHT_MAX, LINE_HEIGHT_MIN,
};
pub(crate) const FONT_SIZE_STEP: f32 = 1.0;
pub(crate) const UI_FONT_SIZE_STEP: f32 = 1.0;
const LINE_HEIGHT_MIN: f32 = 1.0;
const LINE_HEIGHT_MAX: f32 = 2.0;
pub(crate) const LINE_HEIGHT_STEP: f32 = 0.05;
const MAX_CLOSED_TABS: usize = 20;
+1
View File
@@ -484,6 +484,7 @@ pub fn translate_en(key: L10nKey) -> &'static str {
L10nKey::SettingsThemePanelLight => "Choose the theme for light mode.",
L10nKey::SettingsThemePanelDark => "Choose the theme for dark mode.",
L10nKey::SettingsCustom => "Custom",
L10nKey::SettingsCustomValue => "Custom ({value})",
L10nKey::SettingsBuiltIn => "Built-in",
L10nKey::SettingsDark => "Dark",
L10nKey::SettingsLight => "Light",
+1
View File
@@ -489,6 +489,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> {
L10nKey::SettingsThemePanelLight => "ライトモード用のテーマを選択",
L10nKey::SettingsThemePanelDark => "ダークモード用のテーマを選択",
L10nKey::SettingsCustom => "カスタム",
L10nKey::SettingsCustomValue => "カスタム ({value})",
L10nKey::SettingsBuiltIn => "組み込み",
L10nKey::SettingsDark => "ダーク",
L10nKey::SettingsLight => "ライト",
+1
View File
@@ -422,6 +422,7 @@ l10n_keys! {
SettingsThemePanelLight,
SettingsThemePanelDark,
SettingsCustom,
SettingsCustomValue,
SettingsBuiltIn,
SettingsDark,
SettingsLight,
+1
View File
@@ -423,6 +423,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> {
L10nKey::SettingsThemePanelLight => "选择浅色模式的主题。",
L10nKey::SettingsThemePanelDark => "选择深色模式的主题。",
L10nKey::SettingsCustom => "自定义",
L10nKey::SettingsCustomValue => "自定义 ({value})",
L10nKey::SettingsBuiltIn => "内置",
L10nKey::SettingsDark => "深色",
L10nKey::SettingsLight => "浅色",
+213 -38
View File
@@ -226,6 +226,59 @@ const SPLIT_FORWARD_ROW_BELOW: f32 = 620.;
/// window the report came from, so the two ends take a line each.
const STACK_FORWARD_ENDS_BELOW: f32 = 340.;
/// The scrollback presets, and the labels their cells carry, in draw order.
/// One list each so the number a cell writes is the number it shows —
/// `preset_row_labels_name_the_value_they_write` holds the two together.
const SCROLLBACK_BUCKETS: [usize; 3] = [1_000, 10_000, 100_000];
const SCROLLBACK_LABELS: [&str; 3] = ["1,000", "10,000", "100,000"];
/// The notify-threshold presets. The last one is drawn in minutes, which is
/// why these labels are written out rather than derived.
const NOTIFY_THRESHOLD_BUCKETS: [u64; 4] = [5, 10, 30, 60];
const NOTIFY_THRESHOLD_LABELS: [&str; 4] = ["5s", "10s", "30s", "1m"];
/// Which preset a live value *is*, and — when it is none of them — the label
/// for the trailing cell that names it.
///
/// The match is exact on purpose. Matching a *range* is what made
/// `scrollback_limit: 5000` light up "10,000" and `notify_threshold_secs: 20`
/// light up "30s", with no digits anywhere on the row to correct the
/// impression, and clicking the cell that was wrongly lit overwrote the real
/// value with the bucket's (#550).
fn preset_choice<T: Copy + PartialEq>(
buckets: &[T],
value: T,
name: impl FnOnce(T) -> String,
) -> (Option<usize>, Option<String>) {
match buckets.iter().position(|&b| b == value) {
Some(ix) => (Some(ix), None),
None => (
None,
Some(t_fmt(
L10nKey::SettingsCustomValue,
&[("value", &name(value))],
)),
),
}
}
/// `50000` beside cells reading `10,000` and `100,000` looks like a different
/// kind of number, so the custom cell groups its digits the way the presets
/// next to it are written. Every locale tty7 ships writes these counts the
/// same way — the preset labels themselves are one set of literals for all
/// three.
fn group_thousands(n: usize) -> String {
let digits = n.to_string();
let mut out = String::with_capacity(digits.len() + digits.len() / 3);
for (i, c) in digits.char_indices() {
if i > 0 && (digits.len() - i) % 3 == 0 {
out.push(',');
}
out.push(c);
}
out
}
fn settings_row_id(label: &str, _desc: &str) -> SharedString {
SharedString::from(format!("settings-row-{label}"))
}
@@ -1816,11 +1869,58 @@ impl Tty7App {
selected: usize,
cx: &mut Context<Self>,
on_pick: impl Fn(&mut Self, usize, &mut Window, &mut Context<Self>) + 'static,
) -> AnyElement {
self.segmented_full(sf, id, options, Some(selected), None, cx, on_pick)
}
/// A segmented control over a fixed set of values, used where the config
/// accepts anything in a range. When the live value matches a bucket
/// exactly that bucket is highlighted; when it does not, a trailing
/// "Custom (N)" cell carries the highlight instead of the nearest bucket
/// getting a label it does not have — `scrollback_limit: 5000` used to
/// light up "10,000", and clicking that cell silently overwrote the real
/// value with the bucket's (#550).
///
/// `selected` and `custom_label` come as a pair out of [`preset_choice`]:
/// exactly one of them is `Some`, so exactly one cell is highlighted. The
/// custom cell is not a button — there is no bucket value behind it to
/// write — so it takes neither a click handler nor a pointer cursor, and
/// the buckets beside it stay clickable to move off the custom value.
pub(crate) fn segmented_valued(
&self,
id: impl Into<SharedString>,
options: &[&str],
selected: Option<usize>,
custom_label: Option<String>,
cx: &mut Context<Self>,
on_pick: impl Fn(&mut Self, usize, &mut Window, &mut Context<Self>) + 'static,
) -> AnyElement {
let sf = cx.global::<presets::Surfaces>().window;
self.segmented_full(sf, id, options, selected, custom_label, cx, on_pick)
}
fn segmented_full(
&self,
sf: presets::Surface,
id: impl Into<SharedString>,
options: &[&str],
selected: Option<usize>,
custom_label: Option<String>,
cx: &mut Context<Self>,
on_pick: impl Fn(&mut Self, usize, &mut Window, &mut Context<Self>) + 'static,
) -> AnyElement {
let border = cx.theme().border;
let id: SharedString = id.into();
let on_pick = std::rc::Rc::new(on_pick);
let count = options.len();
let count = options.len() + usize::from(custom_label.is_some());
// The display cells: the fixed buckets, then the custom cell if the
// live value matched none of them.
let cells: Vec<(String, Option<usize>)> = options
.iter()
.enumerate()
.map(|(i, l)| (l.to_string(), Some(i)))
.chain(custom_label.map(|l| (l, None)))
.collect();
h_flex()
.id(gpui::ElementId::Name(id.clone()))
.h(px(24.))
@@ -1829,19 +1929,20 @@ impl Tty7App {
.border_color(border)
.bg(gpui::rgb(sf.base))
.overflow_hidden()
.children(options.iter().enumerate().map(|(i, label)| {
let active = i == selected;
.children(cells.into_iter().enumerate().map(|(i, (label, bucket))| {
// A bucket is highlighted only on an exact match, and the
// custom cell (`bucket == None`) exactly when no bucket was.
let active = bucket == selected;
let on_pick = on_pick.clone();
let corners =
rounding::segment_corners(i, count, rounding::TRACK_RADIUS, rounding::HAIRLINE);
h_flex()
let cell = h_flex()
.id(gpui::ElementId::NamedInteger(id.clone(), i as u64))
.items_center()
.justify_center()
.h_full()
.px_2p5()
.text_sm()
.cursor_pointer()
.rounded_corners(corners)
.when(i > 0, |s| s.border_l_1().border_color(border))
.when(active, |s| {
@@ -1853,11 +1954,18 @@ impl Tty7App {
s.text_color(gpui::rgb(sf.text_resting))
.hover(|h| h.bg(gpui::rgb(sf.hover)))
})
.active(|s| s.bg(gpui::rgb(sf.pressed)))
.child(label.to_string())
.on_click(cx.listener(move |this, _, window, cx| {
on_pick(this, i, window, cx);
}))
.child(label);
match bucket {
Some(ix) => cell
.cursor_pointer()
.active(|s| s.bg(gpui::rgb(sf.pressed)))
.on_click(cx.listener(move |this, _, window, cx| {
on_pick(this, ix, window, cx);
})),
// The custom cell names the current value; it is not a
// button, because there is no bucket value to write.
None => cell,
}
}))
.into_any_element()
}
@@ -4779,11 +4887,13 @@ impl Tty7App {
let smooth_scroll = cfg.smooth_scroll;
let mouse_reporting = cfg.mouse_reporting;
let bell = cfg.bell;
let scrollback_idx = match cfg.scrollback_limit {
n if n <= 1_000 => 0,
n if n <= 10_000 => 1,
_ => 2,
};
// A bucket highlights only on an exact match; any other value gets a
// "Custom (N)" cell so the highlight never claims a number the config
// does not have, and clicking that cell cannot overwrite it (#550).
// Read off `cfg` here, with the rest of the copies: the control itself
// is built further down, past calls that borrow `cx` mutably.
let (scrollback_sel, scrollback_custom) =
preset_choice(&SCROLLBACK_BUCKETS, cfg.scrollback_limit, group_thousands);
let scroll_slider = match self.active_settings() {
Some(s) => s.scroll_slider.clone(),
None => return div().into_any_element(),
@@ -4834,17 +4944,17 @@ impl Tty7App {
.child(Input::new(&link_file_command_input).small())
.into_any_element()
});
let scrollback_radio = self.segmented(
let scrollback_radio = self.segmented_valued(
"term-scrollback",
&["1,000", "10,000", "100,000"],
scrollback_idx,
&SCROLLBACK_LABELS,
scrollback_sel,
scrollback_custom,
cx,
|this, ix, _w, cx| {
let lines = match ix {
0 => 1_000,
1 => 10_000,
_ => 100_000,
};
let lines = SCROLLBACK_BUCKETS
.get(ix)
.copied()
.unwrap_or(Config::default().scrollback_limit);
this.set_scrollback_limit(lines, cx);
},
);
@@ -5333,12 +5443,13 @@ impl Tty7App {
NotifyMode::Unfocused => 1,
NotifyMode::Always => 2,
};
let threshold_idx = match cfg.notify_threshold_secs {
n if n <= 5 => 0,
n if n <= 10 => 1,
n if n <= 30 => 2,
_ => 3,
};
// Exact-match highlight with a "Custom (Ns)" fallback, same as the
// scrollback row: a hand-set 20s used to light up "30s" (#550).
let (threshold_sel, threshold_custom) = preset_choice(
&NOTIFY_THRESHOLD_BUCKETS,
cfg.notify_threshold_secs,
|secs| format!("{secs}s"),
);
let notify_radio = self.segmented(
"wt-notify",
&[
@@ -5357,18 +5468,17 @@ impl Tty7App {
this.set_notify_mode(mode, cx);
},
);
let threshold_radio = self.segmented(
let threshold_radio = self.segmented_valued(
"wt-notify-threshold",
&["5s", "10s", "30s", "1m"],
threshold_idx,
&NOTIFY_THRESHOLD_LABELS,
threshold_sel,
threshold_custom,
cx,
|this, ix, _w, cx| {
let secs = match ix {
0 => 5,
1 => 10,
2 => 30,
_ => 60,
};
let secs = NOTIFY_THRESHOLD_BUCKETS
.get(ix)
.copied()
.unwrap_or(Config::default().notify_threshold_secs);
this.set_notify_threshold(secs, cx);
},
);
@@ -6807,6 +6917,71 @@ mod tests {
);
}
/// A preset row lights up the bucket the value *is*, and nothing when it
/// is none of them — the range match it used to do labelled a hand-set
/// value with a number the config did not hold, and the row carried no
/// digits anywhere to correct it (#550).
#[test]
fn a_preset_row_highlights_only_the_bucket_the_value_actually_is() {
// The default lands on a bucket, so the common case still reads as a
// plain radio row.
let (sel, custom) = preset_choice(
&SCROLLBACK_BUCKETS,
Config::default().scrollback_limit,
group_thousands,
);
assert_eq!((sel, custom), (Some(1), None));
// 50,000 is the value `docs/reference/configuration.mdx` puts in its
// example config, so this is what following the documentation shows.
let (sel, custom) = preset_choice(&SCROLLBACK_BUCKETS, 50_000, group_thousands);
assert_eq!(sel, None, "50,000 is not one of the presets");
let custom = custom.expect("a value off the presets names itself");
assert!(
custom.contains("50,000"),
"the custom cell has to carry the real value, got {custom:?}"
);
// Boundaries: the old range match lit "10,000" for everything from
// 1,001 up, and "100,000" for everything above that.
assert_eq!(
preset_choice(&SCROLLBACK_BUCKETS, 1_001, group_thousands).0,
None
);
assert_eq!(
preset_choice(&SCROLLBACK_BUCKETS, 100_000, group_thousands).0,
Some(2)
);
// Same rule on the notify row, where 20s used to light up "30s".
let (sel, custom) = preset_choice(&NOTIFY_THRESHOLD_BUCKETS, 20, |secs| format!("{secs}s"));
assert_eq!(sel, None);
assert!(custom.is_some_and(|c| c.contains("20s")));
assert_eq!(
preset_choice(&NOTIFY_THRESHOLD_BUCKETS, 60, |secs| format!("{secs}s")).0,
Some(3),
"60s is the '1m' cell, not a custom value"
);
}
/// Each preset cell has to name the number clicking it writes, and the
/// custom cell has to be written the same way as the cells beside it.
#[test]
fn preset_row_labels_name_the_value_they_write() {
assert_eq!(SCROLLBACK_BUCKETS.len(), SCROLLBACK_LABELS.len());
for (bucket, label) in SCROLLBACK_BUCKETS.iter().zip(SCROLLBACK_LABELS) {
assert_eq!(group_thousands(*bucket), label);
}
assert_eq!(
NOTIFY_THRESHOLD_BUCKETS.len(),
NOTIFY_THRESHOLD_LABELS.len()
);
// Grouping starts at four digits and repeats every three.
assert_eq!(group_thousands(0), "0");
assert_eq!(group_thousands(999), "999");
assert_eq!(group_thousands(1_000_000), "1,000,000");
}
/// The thresholds are widths a *label* needs, and a reader who scaled the
/// interface up scaled every label with it while the slider beside it kept
/// the px width it was built at. A window that reads fine at the default