Skip to content

Commit 1052f82

Browse files
authored
refactor(client): one delete modal, and its decoration leaves the locale data (UI/UX v3 N-4d) (#107)
ssh_tab.rs と keybindings_tab.rs はそれぞれ `draw_delete_dialog` を持ち、 片方はもう片方のコピーだった。数か月のあいだに 4 点が食い違っていた: 1. 同じボタンに 2 つの翻訳キー(`-plain` と `-bracketed`) 2. keybindings はフォーカスが外れるとキャンセルの文字色を落とすが、ssh は 常に primary。ssh 側は背景だけでフォーカスを示していた 3. 同じ幅のボタン内で、ラベルの x オフセットが 0.5 セルと 1 セル 4. ssh だけヒント行を描く いずれも誰かが決めた差ではない。それぞれ「既に正しかった側」へ寄せる: キーは装飾なしの 1 つ、色は keybindings の挙動、ラベルは中央揃え(オフセット 自体が消える)、ヒントは Option。 装飾は Rust ではなく翻訳データ側にあった。`"[ Cancel (Esc) ]"` の角括弧は `add_px_rect` が実際に描く枠の代用で、`" Cancel (Esc)"` の先頭 2 スペースは セルしか位置指定の単位が無かった頃のインデント。中央揃えにすると先頭スペース は中心を 1 セルずらすため、8 locale から装飾を外した。 ボタン幅は P4c と同じ `measure_run(label) + padding`。14 セル固定では `[ Abbrechen (Esc) ]` が収まらなかった。 重複が実在したことはコンパイラが裏付けた: ローカル定義を消すと、両タブから add_px_rect / danger_button_colors / SCRIM_ALPHA_FLOOR / scrim_color が 未使用になった。モーダル以外にこれらを使う箇所が無かったということ。 ssh_tab.rs 386→233 行、keybindings_tab.rs 396→255 行。 Spec: docs/plans/2026-08-30-n4-menus-and-dialogs.md §2 D5, §5.5
1 parent 633b2c4 commit 1052f82

13 files changed

Lines changed: 388 additions & 345 deletions

File tree

docs/plans/2026-08-30-n4-menus-and-dialogs.md

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,36 @@ Three notes:
570570
scoping was duller: the test scans its own file, so the literals naming what
571571
must not return matched themselves.)
572572

573+
### 5.5 As built (N-4d)
574+
575+
`settings/delete_dialog.rs` owns the modal; each tab contributes a
576+
`DeleteDialogView` and nothing else. The four drifts resolved as D5 specified,
577+
and the de-duplication is visible in the line counts: `ssh_tab.rs` 386 → 233,
578+
`keybindings_tab.rs` 396 → 255, with 315 lines of shared modal replacing two
579+
copies totalling 326.
580+
581+
- **The compiler confirmed the duplication was real.** Removing the two local
582+
definitions left `add_px_rect`, `danger_button_colors`, `SCRIM_ALPHA_FLOOR`
583+
and `scrim_color` unused in *both* tabs. Those imports existed only to draw
584+
the modal — nothing else in either file needed a rectangle or a danger
585+
colour.
586+
- **`fl!` resolves at the call site, not in the shared module.** `DeleteDialogView`
587+
carries resolved `String`s rather than keys, so the shared file holds no
588+
table of which key belongs to which tab. The one exception is
589+
`settings-delete-confirm-message`, which was already shared and stays inside
590+
the module that interpolates it.
591+
- **The decoration gate reads the locale files directly** rather than the Rust.
592+
That is deliberate: the failure mode §1.4 describes is a *translation* PR
593+
reintroducing `[ … ]`, and no amount of scanning `.rs` would catch it. It
594+
asserts the two old keys are gone and that the surviving one is neither
595+
padded nor bracketed, across all eight files.
596+
597+
One thing this phase did not settle, and should not have: **whether the
598+
Keybindings modal ought to have a hint line.** SSH has one, Keybindings does
599+
not, and the shared modal takes an `Option` so both keep today's appearance.
600+
Making them agree is a UX decision; N-4d only removed the reason they *couldn't*
601+
agree. §8.
602+
573603
---
574604

575605
## 6. Verification
Lines changed: 314 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,314 @@
1+
//! The delete-confirmation modal, shared by the list-shaped tabs (UI/UX v3 N-4d).
2+
//!
3+
//! `ssh_tab.rs` and `keybindings_tab.rs` each had their own
4+
//! `draw_delete_dialog`, one a copy of the other. Four differences had
5+
//! accumulated between the copies, and none of them was a decision anyone made:
6+
//!
7+
//! 1. Two translation keys for the same button —
8+
//! `settings-dialog-cancel-plain` in SSH, `-bracketed` in Keybindings.
9+
//! 2. Keybindings moved the cancel label `primary → secondary` when focus left
10+
//! it; SSH held `primary` in both states, so its cancel button signalled
11+
//! focus by background alone.
12+
//! 3. The label sat at `cell_w * 0.5` in one and `cell_w` in the other, inside
13+
//! boxes that were the same width.
14+
//! 4. SSH drew a hint line under the buttons; Keybindings drew nothing.
15+
//!
16+
//! Each resolves toward whichever copy was already right: one undecorated key,
17+
//! Keybindings' focus colours, labels centred (so the offset ceases to exist),
18+
//! and the hint optional.
19+
//!
20+
//! **The decoration was in the locale data, not just in the Rust.**
21+
//! `"[ Cancel (Esc) ]"` and `" Cancel (Esc)"` carried a button border and an
22+
//! indent inside the translated string — stand-ins from when a cell was the
23+
//! only unit of positioning available. `add_px_rect` draws the real border now,
24+
//! and a two-space prefix moves a centred label off-centre by a cell, so the
25+
//! decoration had to come out of all eight locales rather than out of this
26+
//! file. It is the N-3 finding ("the label's decorative spaces are gone") one
27+
//! layer further out.
28+
//!
29+
//! Button widths follow P4c: `measure_run(label) + padding`, with `n - 1` gaps
30+
//! between `n` buttons. Fourteen cells fit ` Cancel (Esc)` and not
31+
//! `[ Abbrechen (Esc) ]`.
32+
//!
33+
//! This stays hand-written rather than moving into `widgets/`: `CLAUDE.md`
34+
//! records the decision that a modal over the panel is not a settings row.
35+
//! N-4d de-duplicates it; it does not migrate it.
36+
37+
use crate::font::FontManager;
38+
use crate::glyph_atlas::{BgVertex, GlyphAtlas, TextVertex};
39+
use crate::vertex_util::{add_px_rect, add_run_verts, measure_run};
40+
41+
use super::super::util::{SCRIM_ALPHA_FLOOR, scrim_color};
42+
use super::row::danger_button_colors;
43+
use nexterm_config::SurfaceLevel;
44+
45+
/// What differs between the two tabs' modals — everything else is shared.
46+
///
47+
/// Strings rather than keys: `fl!` resolves at the call site, so a tab names
48+
/// its own message without this module holding a table of which key belongs to
49+
/// whom.
50+
pub(super) struct DeleteDialogView {
51+
/// Localised title, e.g. "Delete this SSH host?".
52+
pub title: String,
53+
/// What is being deleted, interpolated into the shared confirm message.
54+
pub target: String,
55+
/// Localised label for the destructive button.
56+
pub confirm_label: String,
57+
/// Optional keyboard hint under the buttons. SSH has one; Keybindings does
58+
/// not, and whether it should is a UX question rather than a geometry one.
59+
pub hint: Option<String>,
60+
/// Whether the destructive button holds focus (`false` = Cancel does).
61+
pub confirm_focused: bool,
62+
}
63+
64+
/// Horizontal padding inside a button, in cells. P4c's value for the consent
65+
/// dialog, reused so the two look like each other.
66+
const BTN_PAD_CELLS: f32 = 1.5;
67+
68+
/// Gap between the two buttons, in cells.
69+
const BTN_GAP_CELLS: f32 = 2.0;
70+
71+
/// Draw the modal centred over the settings panel.
72+
#[allow(clippy::too_many_arguments)]
73+
pub(super) fn draw_delete_dialog(
74+
view: &DeleteDialogView,
75+
tokens: &nexterm_config::DesignTokens,
76+
px: f32,
77+
py: f32,
78+
panel_w: f32,
79+
panel_h: f32,
80+
sw: f32,
81+
sh: f32,
82+
cell_w: f32,
83+
cell_h: f32,
84+
font: &mut FontManager,
85+
atlas: &mut GlyphAtlas,
86+
queue: &wgpu::Queue,
87+
bg_verts: &mut Vec<BgVertex>,
88+
bg_idx: &mut Vec<u16>,
89+
text_verts: &mut Vec<TextVertex>,
90+
text_idx: &mut Vec<u16>,
91+
) {
92+
let metrics = nexterm_config::MetricTokens::default();
93+
let title_style = &metrics.type_ramp.title;
94+
let body_style = &metrics.type_ramp.body;
95+
let btn_style = &metrics.type_ramp.body_strong;
96+
let hint_style = &metrics.type_ramp.caption;
97+
98+
add_px_rect(
99+
px,
100+
py,
101+
panel_w,
102+
panel_h,
103+
scrim_color(tokens, SCRIM_ALPHA_FLOOR),
104+
sw,
105+
sh,
106+
bg_verts,
107+
bg_idx,
108+
);
109+
110+
let dialog_w = panel_w * 0.55;
111+
let dialog_h = cell_h * 8.5;
112+
let dialog_x = px + (panel_w - dialog_w) / 2.0;
113+
let dialog_y = py + (panel_h - dialog_h) / 2.0;
114+
115+
// Danger ring, then the panel face.
116+
add_px_rect(
117+
dialog_x - 2.0,
118+
dialog_y - 2.0,
119+
dialog_w + 4.0,
120+
dialog_h + 4.0,
121+
{
122+
let [r, g, b, _] = tokens.semantic_error;
123+
[r, g, b, 0.80]
124+
},
125+
sw,
126+
sh,
127+
bg_verts,
128+
bg_idx,
129+
);
130+
add_px_rect(
131+
dialog_x,
132+
dialog_y,
133+
dialog_w,
134+
dialog_h,
135+
tokens.surface_0,
136+
sw,
137+
sh,
138+
bg_verts,
139+
bg_idx,
140+
);
141+
142+
add_run_verts(
143+
&view.title,
144+
title_style,
145+
dialog_x + cell_w,
146+
dialog_y + cell_h * 0.6,
147+
tokens.text_on(SurfaceLevel::S0).error,
148+
sw,
149+
sh,
150+
font,
151+
atlas,
152+
queue,
153+
text_verts,
154+
text_idx,
155+
);
156+
157+
let msg = nexterm_i18n::fl!("settings-delete-confirm-message", target = &*view.target);
158+
add_run_verts(
159+
&msg,
160+
body_style,
161+
dialog_x + cell_w,
162+
dialog_y + cell_h * 2.2,
163+
tokens.text_on(SurfaceLevel::S0).secondary,
164+
sw,
165+
sh,
166+
font,
167+
atlas,
168+
queue,
169+
text_verts,
170+
text_idx,
171+
);
172+
173+
// Buttons. Widths come from the labels rather than a cell count, so a
174+
// translation cannot overflow its box (P4c).
175+
let cancel_label = nexterm_i18n::fl!("settings-dialog-cancel");
176+
let labels = [&cancel_label, &view.confirm_label];
177+
let pad = cell_w * BTN_PAD_CELLS;
178+
let gap = cell_w * BTN_GAP_CELLS;
179+
let widths: Vec<f32> = labels
180+
.iter()
181+
.map(|l| measure_run(l, btn_style, font) + pad * 2.0)
182+
.collect();
183+
let total_w: f32 = widths.iter().sum::<f32>() + gap * (labels.len() - 1) as f32;
184+
185+
let btn_h = cell_h * 1.4;
186+
let mut bx = dialog_x + (dialog_w - total_w) / 2.0;
187+
let by = dialog_y + dialog_h - cell_h * 2.5;
188+
let (_, btn_line_h, _) = font.chrome_metrics(btn_style);
189+
190+
for (i, (label, &bw)) in labels.iter().zip(widths.iter()).enumerate() {
191+
let is_confirm = i == 1;
192+
let focused = view.confirm_focused == is_confirm;
193+
194+
let (bg, fg) = if is_confirm {
195+
danger_button_colors(tokens, view.confirm_focused)
196+
} else {
197+
let bg = if focused {
198+
tokens.surface_3
199+
} else {
200+
tokens.surface_1
201+
};
202+
// Keybindings' behaviour: a button that shows focus only by its
203+
// background is the weaker of the two copies.
204+
let fg = if focused {
205+
tokens.text_on(SurfaceLevel::S3).primary
206+
} else {
207+
tokens.text_on(SurfaceLevel::S3).secondary
208+
};
209+
(bg, fg)
210+
};
211+
212+
add_px_rect(bx, by, bw, btn_h, bg, sw, sh, bg_verts, bg_idx);
213+
// Centred, which is what removes the two copies' disagreement about
214+
// the label's x offset — and what the locale decoration would have
215+
// broken.
216+
let label_w = measure_run(label, btn_style, font);
217+
add_run_verts(
218+
label,
219+
btn_style,
220+
bx + (bw - label_w) * 0.5,
221+
by + (btn_h - btn_line_h) * 0.5,
222+
fg,
223+
sw,
224+
sh,
225+
font,
226+
atlas,
227+
queue,
228+
text_verts,
229+
text_idx,
230+
);
231+
bx += bw + gap;
232+
}
233+
234+
if let Some(hint) = &view.hint {
235+
add_run_verts(
236+
hint,
237+
hint_style,
238+
dialog_x + cell_w,
239+
dialog_y + dialog_h - cell_h * 0.9,
240+
tokens.text_on(SurfaceLevel::S0).muted,
241+
sw,
242+
sh,
243+
font,
244+
atlas,
245+
queue,
246+
text_verts,
247+
text_idx,
248+
);
249+
}
250+
}
251+
252+
#[cfg(test)]
253+
mod tests {
254+
/// The two tabs describe the modal; neither draws one.
255+
///
256+
/// The copies drifted in four places over the months they existed side by
257+
/// side (see this module's header). A second `draw_delete_dialog` is how
258+
/// that starts again.
259+
#[test]
260+
fn neither_tab_draws_its_own_delete_dialog() {
261+
for (name, src) in [
262+
("ssh_tab.rs", include_str!("ssh_tab.rs")),
263+
("keybindings_tab.rs", include_str!("keybindings_tab.rs")),
264+
] {
265+
assert!(
266+
!src.contains("fn draw_delete_dialog"),
267+
"{name} defines its own delete modal again; delete_dialog owns it"
268+
);
269+
assert!(
270+
!src.contains("cell_w * 14.0"),
271+
"{name} sizes a dialog button by a cell count again; button \
272+
widths come from measure_run (P4c)"
273+
);
274+
}
275+
}
276+
277+
/// G-decoration: a button's label is text, not a drawn border.
278+
///
279+
/// `"[ Cancel (Esc) ]"` and `" Cancel (Esc)"` put a border and an indent
280+
/// inside the translation. `add_px_rect` draws the border, and a centred
281+
/// label cannot carry a leading indent without going off-centre — so this
282+
/// is the gate that stops the decoration returning through a translation
283+
/// PR, which is the only door left open to it.
284+
#[test]
285+
fn no_button_label_carries_its_own_decoration() {
286+
for locale in ["en", "ja", "de", "fr", "es", "it", "ko", "zh-CN"] {
287+
let raw = std::fs::read_to_string(format!(
288+
"{}/../nexterm-i18n/locales/{locale}.json",
289+
env!("CARGO_MANIFEST_DIR")
290+
))
291+
.expect("locale file is readable");
292+
let map: serde_json::Value =
293+
serde_json::from_str(&raw).expect("locale file is valid JSON");
294+
let obj = map.as_object().expect("locale file is an object");
295+
296+
assert!(
297+
!obj.contains_key("settings-dialog-cancel-plain")
298+
&& !obj.contains_key("settings-dialog-cancel-bracketed"),
299+
"{locale}: the two decorated cancel keys are replaced by one"
300+
);
301+
302+
const KEY: &str = "settings-dialog-cancel";
303+
let v = obj
304+
.get(KEY)
305+
.and_then(|v| v.as_str())
306+
.unwrap_or_else(|| panic!("{locale}: {KEY} is missing"));
307+
assert_eq!(v.trim(), v, "{locale}: {KEY} is padded: {v:?}");
308+
assert!(
309+
!v.starts_with('[') && !v.ends_with(']'),
310+
"{locale}: {KEY} draws its own button border: {v:?}"
311+
);
312+
}
313+
}
314+
}

0 commit comments

Comments
 (0)