feat: add configurable line_height setting#278
Merged
lassejlv merged 5 commits intotermy-org:mainfrom Mar 23, 2026
Merged
Conversation
Replace the hardcoded 1.4 line-height multiplier with a user-facing config option (range 0.8–2.5, default 1.4). Adds parsing, validation, settings UI stepper, and live config reload support.
📝 WalkthroughWalkthroughThis PR introduces a new configurable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a user-configurable line_height multiplier to replace the previously hardcoded terminal row-height scaling, wiring it through config parsing/validation, settings UI, and live reload so spacing can be adjusted without restart.
Changes:
- Introduces
line_heightconfig (defaults/range/constants) and parser + schema support. - Updates settings UI/state to edit and persist
line_heightwith bounds and stepper behavior. - Applies
line_heightinTerminalViewinitialization and config reload; updates docs/default config.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/terminal_view/mod.rs |
Uses config.line_height (clamped) instead of a hardcoded multiplier; updates on config reload. |
src/settings_view/state_apply.rs |
Adds validation and persistence for the new LineHeight editable field. |
src/settings_view/state.rs |
Adds EditableField::LineHeight, numeric stepper config, formatting helper, and updates tests. |
src/settings_view/sections.rs |
Renders the new line-height row in the FONT settings group. |
src/main.rs |
Removes debug-build dock icon setup (unrelated to line-height feature). |
docs/configuration.md |
Documents the new line_height setting. |
crates/terminal_ui/src/grid.rs |
Minor refactor/cleanup in test-only draw-op collection path. |
crates/config_core/src/types.rs |
Adds line_height to AppConfig with default. |
crates/config_core/src/schema.rs |
Adds LineHeight root setting + default value test. |
crates/config_core/src/parser_tests.rs |
Adds parsing/validation coverage for line_height (bounds + NaN/inf). |
crates/config_core/src/parser.rs |
Parses line_height and validates range/finite. |
crates/config_core/src/lib.rs |
Re-exports line-height constants. |
crates/config_core/src/default_config.txt |
Adds default line_height = 1.4. |
crates/config_core/src/constants.rs |
Defines DEFAULT_LINE_HEIGHT, MIN_LINE_HEIGHT, MAX_LINE_HEIGHT. |
Cargo.lock |
Reflects workspace version bump / lockfile updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/terminal_view/mod.rs (1)
2966-2966: Add a regression test for startup vs reload clamp parity.Given clamping exists in two places (Line 2966 and Line 3278), a focused test would guard against drift between init and runtime-reload behavior.
Also applies to: 3278-3278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/terminal_view/mod.rs` at line 2966, Add a regression test that verifies clamping parity for config.line_height between the init path and the runtime-reload path: create a test that constructs a config with line_height below MIN_LINE_HEIGHT and another above MAX_LINE_HEIGHT, then run the init code path that produces the initial clamped value and the runtime reload/apply path (the code that also calls .clamp on line_height) and assert both results equal the expected clamped value (using MIN_LINE_HEIGHT and MAX_LINE_HEIGHT). Locate the code via the symbols line_height, MIN_LINE_HEIGHT, MAX_LINE_HEIGHT and the two code paths (the initializer that sets line_height and the reload/apply method that reassigns it) and ensure the test covers both negative and oversized inputs to guard against drift.crates/config_core/src/schema.rs (1)
482-482: Consider normalizing line-height serialization format across layers.Line 482 uses
to_string(), whilesrc/settings_view/state.rspersists with two decimals. Aligning both would avoid cosmetic1.4vs1.40config diffs.♻️ Optional consistency tweak
- RootSettingId::LineHeight => Some(config.line_height.to_string()), + RootSettingId::LineHeight => Some(format!("{:.2}", config.line_height)),- Some(defaults.line_height.to_string()) + Some(format!("{:.2}", defaults.line_height))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config_core/src/schema.rs` at line 482, The RootSettingId::LineHeight branch currently serializes with config.line_height.to_string(), causing inconsistencies with the two-decimal persistence in src/settings_view/state.rs; change the serialization in the RootSettingId::LineHeight arm to format the value the same way as settings_view/state.rs (e.g. two decimals) so both layers produce identical string output—locate the RootSettingId::LineHeight case in schema.rs and replace the to_string() usage with the same formatting approach used in settings_view/state.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/settings_view/state_apply.rs`:
- Around line 112-116: The code currently assigns self.config.line_height =
parsed before calling config::set_root_setting, which can leave in-memory state
changed if persistence fails; change the order so you call
config::set_root_setting(termy_config_core::RootSettingId::LineHeight,
&parsed.to_string()) first and only assign self.config.line_height = parsed
after that call succeeds (propagate or return the error from set_root_setting on
failure). Locate the update to self.config.line_height and the subsequent call
to config::set_root_setting in state_apply.rs and swap them, ensuring you
preserve the same parsed value and error handling.
---
Nitpick comments:
In `@crates/config_core/src/schema.rs`:
- Line 482: The RootSettingId::LineHeight branch currently serializes with
config.line_height.to_string(), causing inconsistencies with the two-decimal
persistence in src/settings_view/state.rs; change the serialization in the
RootSettingId::LineHeight arm to format the value the same way as
settings_view/state.rs (e.g. two decimals) so both layers produce identical
string output—locate the RootSettingId::LineHeight case in schema.rs and replace
the to_string() usage with the same formatting approach used in
settings_view/state.rs.
In `@src/terminal_view/mod.rs`:
- Line 2966: Add a regression test that verifies clamping parity for
config.line_height between the init path and the runtime-reload path: create a
test that constructs a config with line_height below MIN_LINE_HEIGHT and another
above MAX_LINE_HEIGHT, then run the init code path that produces the initial
clamped value and the runtime reload/apply path (the code that also calls .clamp
on line_height) and assert both results equal the expected clamped value (using
MIN_LINE_HEIGHT and MAX_LINE_HEIGHT). Locate the code via the symbols
line_height, MIN_LINE_HEIGHT, MAX_LINE_HEIGHT and the two code paths (the
initializer that sets line_height and the reload/apply method that reassigns it)
and ensure the test covers both negative and oversized inputs to guard against
drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0dfec68-4a47-4ad3-89e8-589fd7527de1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/config_core/src/constants.rscrates/config_core/src/default_config.txtcrates/config_core/src/lib.rscrates/config_core/src/parser.rscrates/config_core/src/parser_tests.rscrates/config_core/src/schema.rscrates/config_core/src/types.rscrates/terminal_ui/src/grid.rsdocs/configuration.mdsrc/main.rssrc/settings_view/sections.rssrc/settings_view/state.rssrc/settings_view/state_apply.rssrc/terminal_view/mod.rs
💤 Files with no reviewable changes (1)
- src/main.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/settings_view/state.rs (1)
1260-1280: Consider asserting line-height step semantics explicitly.Current test checks presence of
numeric_step, but not that Line Height keepsdelta=0.05andmin/maxbound totermy_config_core::{MIN_LINE_HEIGHT, MAX_LINE_HEIGHT}.🧪 Suggested test hardening
fn numeric_step_specs_are_defined_for_numeric_fields() { let numeric_fields = [ EditableField::BackgroundOpacity, EditableField::FontSize, EditableField::LineHeight, EditableField::PaddingX, EditableField::PaddingY, EditableField::ScrollbackHistory, EditableField::InactiveTabScrollback, EditableField::ScrollMultiplier, EditableField::PaneFocusStrength, EditableField::WindowWidth, EditableField::WindowHeight, EditableField::VerticalTabsWidth, ]; for field in numeric_fields { let spec = SettingsWindow::field_spec(field); assert_eq!(spec.codec, FieldCodec::Numeric); assert!(spec.numeric_step.is_some()); } + + let line_height_spec = SettingsWindow::field_spec(EditableField::LineHeight); + let step = line_height_spec.numeric_step.expect("missing line-height step"); + assert!((step.delta - 0.05).abs() < f32::EPSILON); + assert!((step.min - termy_config_core::MIN_LINE_HEIGHT).abs() < f32::EPSILON); + assert!((step.max - termy_config_core::MAX_LINE_HEIGHT).abs() < f32::EPSILON); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings_view/state.rs` around lines 1260 - 1280, The test numeric_step_specs_are_defined_for_numeric_fields currently only checks that spec.numeric_step is present; add explicit assertions for EditableField::LineHeight by retrieving let spec = SettingsWindow::field_spec(EditableField::LineHeight) and assert that spec.numeric_step.as_ref().unwrap().delta == 0.05 and that spec.numeric_step.as_ref().unwrap().min == termy_config_core::MIN_LINE_HEIGHT and spec.numeric_step.as_ref().unwrap().max == termy_config_core::MAX_LINE_HEIGHT so the test enforces the expected step semantics and bounds for LineHeight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/settings_view/state.rs`:
- Around line 991-999: EditableField::LineHeight handler updates
self.config.line_height before persisting which can leave UI state inconsistent
if config::set_root_setting fails; change the flow in the
EditableField::LineHeight branch to compute next = (self.config.line_height +
(delta as f32 * step.delta)).clamp(...), call
config::set_root_setting(termy_config_core::RootSettingId::LineHeight,
&format_line_height(next)) first, check the Result and only assign
self.config.line_height = next on success (handle/log/propagate the error on
failure instead of mutating in-memory state).
---
Nitpick comments:
In `@src/settings_view/state.rs`:
- Around line 1260-1280: The test
numeric_step_specs_are_defined_for_numeric_fields currently only checks that
spec.numeric_step is present; add explicit assertions for
EditableField::LineHeight by retrieving let spec =
SettingsWindow::field_spec(EditableField::LineHeight) and assert that
spec.numeric_step.as_ref().unwrap().delta == 0.05 and that
spec.numeric_step.as_ref().unwrap().min == termy_config_core::MIN_LINE_HEIGHT
and spec.numeric_step.as_ref().unwrap().max ==
termy_config_core::MAX_LINE_HEIGHT so the test enforces the expected step
semantics and bounds for LineHeight.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b4e8dfc-b5ff-46ce-9fad-3cf2f68f7c47
📒 Files selected for processing (5)
crates/config_core/src/lib.rscrates/config_core/src/schema.rssrc/settings_view/mod.rssrc/settings_view/state.rssrc/settings_view/state_apply.rs
✅ Files skipped from review due to trivial changes (1)
- src/settings_view/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/settings_view/state_apply.rs
- crates/config_core/src/lib.rs
Member
|
W pr, let me know when you're done and i will review it :D |
Contributor
Author
|
@lassejlv thanks! Just finished it, you can go ahead and review it :-) |
Member
|
Looks perfect, gonna merge it from here! Thanks for your contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
line_heightconfig option (range 0.8–2.5, default 1.4)Test plan
line_height = 1.0produces tightly packed rowsline_height = 2.0doubles vertical spacing0.5,3.0) fall back to defaultcargo test -p termy_config_core— all passSummary by CodeRabbit
New Features
Documentation