diff --git a/CHANGELOG.md b/CHANGELOG.md index b0a122ed..340a27e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 6–48, line height 1.0–2.0) than the + config file allows (4–256, 0.5–4.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 diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index 4a51f0f4..95092e8b 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -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 (6–48, +/// 1.0–2.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 diff --git a/src/ui/app.rs b/src/ui/app.rs index 753c3946..04875996 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -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; diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index fb0c77e4..9c53a128 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -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", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 3186269a..0a2abed0 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -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 => "ライト", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index ddadd080..d3d40b78 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -422,6 +422,7 @@ l10n_keys! { SettingsThemePanelLight, SettingsThemePanelDark, SettingsCustom, + SettingsCustomValue, SettingsBuiltIn, SettingsDark, SettingsLight, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index a2899c6c..9eebd15f 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -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 => "浅色", diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 5907b4e9..b0d283bb 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -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( + buckets: &[T], + value: T, + name: impl FnOnce(T) -> String, +) -> (Option, Option) { + 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, on_pick: impl Fn(&mut Self, usize, &mut Window, &mut Context) + '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, + options: &[&str], + selected: Option, + custom_label: Option, + cx: &mut Context, + on_pick: impl Fn(&mut Self, usize, &mut Window, &mut Context) + 'static, + ) -> AnyElement { + let sf = cx.global::().window; + self.segmented_full(sf, id, options, selected, custom_label, cx, on_pick) + } + + fn segmented_full( + &self, + sf: presets::Surface, + id: impl Into, + options: &[&str], + selected: Option, + custom_label: Option, + cx: &mut Context, + on_pick: impl Fn(&mut Self, usize, &mut Window, &mut Context) + '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)> = 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