Skip to content

Commit d60ab45

Browse files
sinelawclaude
andcommitted
refactor(editor): address indentation-guide review findings
Follow-up cleanup on the indentation-guides feature from code review: - Reuse indent-based folding (find_fold_range_at_byte) for active-mode block detection instead of hand-coded {([:/} delimiter heuristics, so a guide spans exactly the rows the matching fold would hide. Probe with the cursor's line-start byte so a cursor past a block's last-line indent still resolves its enclosing block. - Compute all-mode guide columns with a monotonic indentation-staircase scanner walked once over the viewport: O(n) instead of O(n^2), and two depth-bounded reused buffers instead of a per-frame Vec<Vec<usize>>. The scan is skipped entirely unless mode is `all`. - Draw guides continuously through whitespace-only lines (matching active mode) rather than leaving a one-row gap. - Reuse indent_folding::slice_indent for indent measurement, deleting the two bespoke indent helpers. - Drop the two hardcoded settings-ordering functions; settings sort alphabetically like the rest. - Drop the unrelated humanizeSchemaName refactor from theme_editor.ts. - Rename the config key indentation_guides -> indentation_guide and regenerate config-schema.json. Give CellPassInput/CellPass a second lifetime so the per-row guide-column slice can borrow the reused buffer independently of the render-wide lifetime. Active-mode tests rewritten for the new pure-indent model; adds unit + e2e coverage for guides through blank lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 841110d commit d60ab45

17 files changed

Lines changed: 370 additions & 699 deletions

File tree

crates/fresh-editor/plugins/config-schema.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@
7979
"terminal_auto_title": true,
8080
"cursor_style": "default",
8181
"rulers": [],
82-
"indentation_guides": "none",
82+
"indentation_guide": "none",
8383
"indentation_guide_glyph": "",
8484
"whitespace_show": true,
8585
"whitespace_spaces_leading": false,
@@ -581,7 +581,7 @@
581581
"default": [],
582582
"x-section": "Display"
583583
},
584-
"indentation_guides": {
584+
"indentation_guide": {
585585
"description": "Vertical indentation guide rendering mode.\nGuides are drawn at indentation levels derived from the active buffer's\ntab size. They replace existing leading whitespace cells visually only;\nbuffer text, cursor positions, and mouse mappings are unchanged.\nModes:\n- `none`: disable indentation guides.\n- `all`: draw every indentation level in leading whitespace.\n- `active`: draw only the innermost guide for the cursor's current\n indentation block.\nDefault: none",
586586
"$ref": "#/$defs/IndentationGuideMode",
587587
"default": "none",

crates/fresh-editor/plugins/theme_editor.ts

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -249,25 +249,6 @@ function fieldRefersToColorDef(fieldObj: Record<string, unknown>): boolean {
249249
return false;
250250
}
251251

252-
function humanizeSchemaName(name: string): string {
253-
return name
254-
.split("_")
255-
.filter(part => part.length > 0)
256-
.map(part => {
257-
if (part === "fg") return "Foreground";
258-
if (part === "bg") return "Background";
259-
if (part === "ui") return "UI";
260-
if (part === "lsp") return "LSP";
261-
return part.charAt(0).toUpperCase() + part.slice(1);
262-
})
263-
.join(" ");
264-
}
265-
266-
function translateOrFallback(key: string, fallback: string): string {
267-
const translated = editor.t(key);
268-
return translated && translated !== key ? translated : fallback;
269-
}
270-
271252
/**
272253
* Load theme sections from the Rust API.
273254
* Parses the raw JSON Schema and resolves $ref references.
@@ -338,8 +319,8 @@ function loadThemeSections(): ThemeSection[] {
338319

339320
fields.push({
340321
key: fieldName,
341-
displayName: translateOrFallback(i18nName, humanizeSchemaName(fieldName)),
342-
description: translateOrFallback(i18nDesc, fieldDesc),
322+
displayName: editor.t(i18nName) || fieldDesc || fieldName,
323+
description: editor.t(i18nDesc) || fieldDesc,
343324
section: sectionName,
344325
});
345326
}
@@ -353,8 +334,8 @@ function loadThemeSections(): ThemeSection[] {
353334

354335
sections.push({
355336
name: sectionName,
356-
displayName: translateOrFallback(sectionI18nName, humanizeSchemaName(sectionName)),
357-
description: translateOrFallback(sectionI18nDesc, sectionDesc),
337+
displayName: editor.t(sectionI18nName) || sectionDesc || sectionName,
338+
description: editor.t(sectionI18nDesc) || sectionDesc,
358339
fields,
359340
});
360341
}

crates/fresh-editor/src/app/render.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ impl Editor {
719719
self.config.editor.diagnostics_inline_text,
720720
self.config.editor.show_tilde,
721721
self.config.editor.highlight_current_column,
722-
self.config.editor.indentation_guides,
722+
self.config.editor.indentation_guide,
723723
&self.config.editor.indentation_guide_glyph,
724724
self.config.editor.hide_current_line_on_selection,
725725
__cell_theme_map_mut,
@@ -2356,7 +2356,7 @@ impl Editor {
23562356
self.config.editor.diagnostics_inline_text,
23572357
false, // hide tilde markers in the preview
23582358
self.config.editor.highlight_current_column,
2359-
self.config.editor.indentation_guides,
2359+
self.config.editor.indentation_guide,
23602360
&self.config.editor.indentation_guide_glyph,
23612361
self.config.editor.hide_current_line_on_selection,
23622362
&mut scratch_cell_theme_map,
@@ -3382,7 +3382,7 @@ impl Editor {
33823382
let diagnostics_inline_text = self.config.editor.diagnostics_inline_text;
33833383
let show_tilde = false; // preview hides tilde markers
33843384
let highlight_current_column = self.config.editor.highlight_current_column;
3385-
let indentation_guides = self.config.editor.indentation_guides;
3385+
let indentation_guide = self.config.editor.indentation_guide;
33863386
let indentation_guide_glyph = self.config.editor.indentation_guide_glyph.clone();
33873387
let screen_width = frame.area().width;
33883388

@@ -3454,7 +3454,7 @@ impl Editor {
34543454
diagnostics_inline_text,
34553455
show_tilde,
34563456
highlight_current_column,
3457-
indentation_guides,
3457+
indentation_guide,
34583458
&indentation_guide_glyph,
34593459
cell_theme_map,
34603460
screen_width,

crates/fresh-editor/src/config.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1208,7 +1208,7 @@ pub struct EditorConfig {
12081208
/// Default: none
12091209
#[serde(default)]
12101210
#[schemars(extend("x-section" = "Display"))]
1211-
pub indentation_guides: IndentationGuideMode,
1211+
pub indentation_guide: IndentationGuideMode,
12121212

12131213
/// Glyph used to draw indentation guides. The default is a left-aligned
12141214
/// vertical guide character. Leading/trailing whitespace is ignored, and
@@ -1799,7 +1799,7 @@ impl Default for EditorConfig {
17991799
set_window_title: true,
18001800
terminal_auto_title: true,
18011801
rulers: Vec::new(),
1802-
indentation_guides: IndentationGuideMode::None,
1802+
indentation_guide: IndentationGuideMode::None,
18031803
indentation_guide_glyph: default_indentation_guide_glyph(),
18041804
whitespace_show: true,
18051805
whitespace_spaces_leading: false,
@@ -7727,10 +7727,10 @@ mod tests {
77277727
#[test]
77287728
fn test_indentation_guide_mode_rejects_booleans() {
77297729
assert!(
7730-
serde_json::from_str::<Config>(r#"{"editor":{"indentation_guides":false}}"#).is_err()
7730+
serde_json::from_str::<Config>(r#"{"editor":{"indentation_guide":false}}"#).is_err()
77317731
);
77327732
assert!(
7733-
serde_json::from_str::<Config>(r#"{"editor":{"indentation_guides":true}}"#).is_err()
7733+
serde_json::from_str::<Config>(r#"{"editor":{"indentation_guide":true}}"#).is_err()
77347734
);
77357735
}
77367736

@@ -7741,9 +7741,9 @@ mod tests {
77417741
("all", IndentationGuideMode::All),
77427742
("active", IndentationGuideMode::Active),
77437743
] {
7744-
let json = format!(r#"{{"editor":{{"indentation_guides":"{}"}}}}"#, value);
7744+
let json = format!(r#"{{"editor":{{"indentation_guide":"{}"}}}}"#, value);
77457745
let cfg: Config = serde_json::from_str(&json).unwrap();
7746-
assert_eq!(cfg.editor.indentation_guides, expected);
7746+
assert_eq!(cfg.editor.indentation_guide, expected);
77477747
}
77487748
}
77497749

@@ -7752,10 +7752,10 @@ mod tests {
77527752
assert_eq!(Config::default().editor.indentation_guide_glyph, "▏");
77537753

77547754
let cfg: Config = serde_json::from_str(
7755-
r#"{"editor":{"indentation_guides":"all","indentation_guide_glyph":"┊"}}"#,
7755+
r#"{"editor":{"indentation_guide":"all","indentation_guide_glyph":"┊"}}"#,
77567756
)
77577757
.unwrap();
7758-
assert_eq!(cfg.editor.indentation_guides, IndentationGuideMode::All);
7758+
assert_eq!(cfg.editor.indentation_guide, IndentationGuideMode::All);
77597759
assert_eq!(cfg.editor.indentation_guide_glyph, "┊");
77607760
}
77617761

crates/fresh-editor/src/partial_config.rs

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ pub struct PartialEditorConfig {
218218
pub set_window_title: Option<bool>,
219219
pub terminal_auto_title: Option<bool>,
220220
pub rulers: Option<Vec<usize>>,
221-
pub indentation_guides: Option<IndentationGuideMode>,
221+
pub indentation_guide: Option<IndentationGuideMode>,
222222
pub indentation_guide_glyph: Option<String>,
223223
pub whitespace_show: Option<bool>,
224224
pub whitespace_spaces_leading: Option<bool>,
@@ -338,8 +338,7 @@ impl Merge for PartialEditorConfig {
338338
self.terminal_auto_title
339339
.merge_from(&other.terminal_auto_title);
340340
self.rulers.merge_from(&other.rulers);
341-
self.indentation_guides
342-
.merge_from(&other.indentation_guides);
341+
self.indentation_guide.merge_from(&other.indentation_guide);
343342
self.indentation_guide_glyph
344343
.merge_from(&other.indentation_guide_glyph);
345344
self.whitespace_show.merge_from(&other.whitespace_show);
@@ -656,7 +655,7 @@ impl From<&crate::config::EditorConfig> for PartialEditorConfig {
656655
set_window_title: Some(cfg.set_window_title),
657656
terminal_auto_title: Some(cfg.terminal_auto_title),
658657
rulers: Some(cfg.rulers.clone()),
659-
indentation_guides: Some(cfg.indentation_guides),
658+
indentation_guide: Some(cfg.indentation_guide),
660659
indentation_guide_glyph: Some(cfg.indentation_guide_glyph.clone()),
661660
whitespace_show: Some(cfg.whitespace_show),
662661
whitespace_spaces_leading: Some(cfg.whitespace_spaces_leading),
@@ -829,9 +828,7 @@ impl PartialEditorConfig {
829828
.terminal_auto_title
830829
.unwrap_or(defaults.terminal_auto_title),
831830
rulers: self.rulers.unwrap_or_else(|| defaults.rulers.clone()),
832-
indentation_guides: self
833-
.indentation_guides
834-
.unwrap_or(defaults.indentation_guides),
831+
indentation_guide: self.indentation_guide.unwrap_or(defaults.indentation_guide),
835832
indentation_guide_glyph: self
836833
.indentation_guide_glyph
837834
.map(|glyph| crate::config::normalize_indentation_guide_glyph(&glyph))

crates/fresh-editor/src/view/folding.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,9 @@ pub mod indent_folding {
436436
}
437437

438438
/// Measure leading indent of a line given as a byte slice (no trailing `\n`).
439-
fn slice_indent(line: &[u8], tab_size: usize) -> (usize, bool) {
439+
/// Returns `(indent_width, all_blank)` where `all_blank` is true when the
440+
/// line has no non-whitespace character. Tabs expand against `tab_size`.
441+
pub fn slice_indent(line: &[u8], tab_size: usize) -> (usize, bool) {
440442
let mut indent = 0;
441443
let mut all_blank = true;
442444
for &b in line {

crates/fresh-editor/src/view/settings/items.rs

Lines changed: 5 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -896,13 +896,11 @@ fn build_page(category: &SettingCategory, ctx: &BuildContext) -> SettingsPage {
896896
.collect();
897897

898898
// Sort items: by section first (None comes last), then alphabetically by name
899-
items.sort_by(|a, b| {
900-
indentation_guide_items_order(a, b).unwrap_or_else(|| match (&a.section, &b.section) {
901-
(Some(sec_a), Some(sec_b)) => sec_a.cmp(sec_b).then_with(|| a.name.cmp(&b.name)),
902-
(Some(_), None) => std::cmp::Ordering::Less,
903-
(None, Some(_)) => std::cmp::Ordering::Greater,
904-
(None, None) => a.name.cmp(&b.name),
905-
})
899+
items.sort_by(|a, b| match (&a.section, &b.section) {
900+
(Some(sec_a), Some(sec_b)) => sec_a.cmp(sec_b).then_with(|| a.name.cmp(&b.name)),
901+
(Some(_), None) => std::cmp::Ordering::Less,
902+
(None, Some(_)) => std::cmp::Ordering::Greater,
903+
(None, None) => a.name.cmp(&b.name),
906904
});
907905

908906
// Mark items that start a new section, and capture the section list
@@ -945,17 +943,6 @@ fn build_page(category: &SettingCategory, ctx: &BuildContext) -> SettingsPage {
945943
}
946944
}
947945

948-
fn indentation_guide_items_order(a: &SettingItem, b: &SettingItem) -> Option<std::cmp::Ordering> {
949-
const MODE_PATH: &str = "/editor/indentation_guides";
950-
const GLYPH_PATH: &str = "/editor/indentation_guide_glyph";
951-
952-
match (a.path.as_str(), b.path.as_str()) {
953-
(MODE_PATH, GLYPH_PATH) => Some(std::cmp::Ordering::Less),
954-
(GLYPH_PATH, MODE_PATH) => Some(std::cmp::Ordering::Greater),
955-
_ => None,
956-
}
957-
}
958-
959946
/// Expand an Object schema into its children when every child has a native
960947
/// (non-JSON) control, otherwise build it as a single item. This lets compound
961948
/// config structs like `StatusBarConfig` surface their children as individual
@@ -1720,44 +1707,6 @@ mod tests {
17201707
}
17211708
}
17221709

1723-
#[test]
1724-
fn test_indentation_guide_glyph_follows_mode_in_built_page() {
1725-
let categories = crate::view::settings::schema::parse_schema(include_str!(
1726-
"../../../plugins/config-schema.json"
1727-
))
1728-
.unwrap();
1729-
let config = serde_json::json!({
1730-
"editor": {
1731-
"indentation_guides": "none",
1732-
"indentation_guide_glyph": "▏"
1733-
}
1734-
});
1735-
let pages = build_pages(
1736-
&categories,
1737-
&config,
1738-
&HashMap::new(),
1739-
ConfigLayer::User,
1740-
&HashMap::new(),
1741-
);
1742-
let editor = pages
1743-
.iter()
1744-
.find(|page| page.path == "/editor")
1745-
.expect("editor page should exist");
1746-
1747-
let guide_mode_idx = editor
1748-
.items
1749-
.iter()
1750-
.position(|item| item.path == "/editor/indentation_guides")
1751-
.expect("indentation_guides item should exist");
1752-
let guide_glyph_idx = editor
1753-
.items
1754-
.iter()
1755-
.position(|item| item.path == "/editor/indentation_guide_glyph")
1756-
.expect("indentation_guide_glyph item should exist");
1757-
1758-
assert_eq!(guide_glyph_idx, guide_mode_idx + 1);
1759-
}
1760-
17611710
#[test]
17621711
fn test_build_text_item() {
17631712
let schema = SettingSchema {

crates/fresh-editor/src/view/settings/schema.rs

Lines changed: 11 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -498,32 +498,16 @@ fn parse_properties(
498498

499499
// Sort settings: by x-order (if set) first, then alphabetically by name.
500500
// Settings with x-order come before those without.
501-
settings.sort_by(|a, b| {
502-
indentation_guide_settings_order(a, b).unwrap_or_else(|| match (a.order, b.order) {
503-
(Some(a_ord), Some(b_ord)) => a_ord.cmp(&b_ord).then_with(|| a.name.cmp(&b.name)),
504-
(Some(_), None) => std::cmp::Ordering::Less,
505-
(None, Some(_)) => std::cmp::Ordering::Greater,
506-
(None, None) => a.name.cmp(&b.name),
507-
})
501+
settings.sort_by(|a, b| match (a.order, b.order) {
502+
(Some(a_ord), Some(b_ord)) => a_ord.cmp(&b_ord).then_with(|| a.name.cmp(&b.name)),
503+
(Some(_), None) => std::cmp::Ordering::Less,
504+
(None, Some(_)) => std::cmp::Ordering::Greater,
505+
(None, None) => a.name.cmp(&b.name),
508506
});
509507

510508
settings
511509
}
512510

513-
fn indentation_guide_settings_order(
514-
a: &SettingSchema,
515-
b: &SettingSchema,
516-
) -> Option<std::cmp::Ordering> {
517-
const MODE_PATH: &str = "/editor/indentation_guides";
518-
const GLYPH_PATH: &str = "/editor/indentation_guide_glyph";
519-
520-
match (a.path.as_str(), b.path.as_str()) {
521-
(MODE_PATH, GLYPH_PATH) => Some(std::cmp::Ordering::Less),
522-
(GLYPH_PATH, MODE_PATH) => Some(std::cmp::Ordering::Greater),
523-
_ => None,
524-
}
525-
}
526-
527511
/// Parse a single setting from its schema
528512
fn parse_setting(
529513
name: &str,
@@ -979,53 +963,31 @@ mod tests {
979963
}
980964

981965
#[test]
982-
fn test_indentation_guides_generated_schema_renders_as_enum() {
966+
fn test_indentation_guide_generated_schema_renders_as_enum() {
983967
let categories = parse_schema(include_str!("../../../plugins/config-schema.json")).unwrap();
984968
let editor = categories
985969
.iter()
986970
.find(|c| c.path == "/editor")
987971
.expect("editor category should exist");
988-
let indentation_guides = editor
972+
let indentation_guide = editor
989973
.settings
990974
.iter()
991-
.find(|s| s.path == "/editor/indentation_guides")
992-
.expect("indentation_guides setting should exist");
975+
.find(|s| s.path == "/editor/indentation_guide")
976+
.expect("indentation_guide setting should exist");
993977

994-
match &indentation_guides.setting_type {
978+
match &indentation_guide.setting_type {
995979
SettingType::Enum { options } => {
996980
let values: Vec<&str> =
997981
options.iter().map(|option| option.value.as_str()).collect();
998982
assert_eq!(values, vec!["none", "all", "active"]);
999983
}
1000984
other => panic!(
1001-
"expected indentation_guides to render as enum, got {:?}",
985+
"expected indentation_guide to render as enum, got {:?}",
1002986
other
1003987
),
1004988
}
1005989
}
1006990

1007-
#[test]
1008-
fn test_indentation_guide_glyph_follows_mode_setting() {
1009-
let categories = parse_schema(include_str!("../../../plugins/config-schema.json")).unwrap();
1010-
let editor = categories
1011-
.iter()
1012-
.find(|c| c.path == "/editor")
1013-
.expect("editor category should exist");
1014-
1015-
let guide_mode_idx = editor
1016-
.settings
1017-
.iter()
1018-
.position(|s| s.path == "/editor/indentation_guides")
1019-
.expect("indentation_guides setting should exist");
1020-
let guide_glyph_idx = editor
1021-
.settings
1022-
.iter()
1023-
.position(|s| s.path == "/editor/indentation_guide_glyph")
1024-
.expect("indentation_guide_glyph setting should exist");
1025-
1026-
assert_eq!(guide_glyph_idx, guide_mode_idx + 1);
1027-
}
1028-
1029991
#[test]
1030992
fn test_humanize_name() {
1031993
assert_eq!(humanize_name("tab_size"), "Tab Size");

0 commit comments

Comments
 (0)