From 2057ab0008fdc7bda800c59c2df5310719ea4e51 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 27 Sep 2026 13:41:59 +0200 Subject: [PATCH] fix(ui): draw every confirmation as the app's own dialog card window.prompt fell through to NSAlert on macOS and TaskDialog on Windows, so closing a busy tab threw up a system alert with a centred app icon and stacked grey buttons while every other question the app asks is a ui::dialog card. The Linux fallback card had drifted too (accent buttons, its own padding). TextPrompt is now built from ui::dialog on all three platforms: the 12px card at the switcher's top offset, a wrapping title row with an esc cap, a hairline footer and ink-filled primary. Return and Escape keep the native meanings; the scrim occludes the window and backs out; a lone OK answers Escape; an answer listed after Cancel in a prompt of three stands apart at the footer's left, and an action behind a Cancel-first default is painted red. SCM's discard and destructive-op prompts passed Cancel as a bare label, which gpui only recognises as a cancel when it is the English word; they now mark it explicitly. --- src/ui/prompt.rs | 252 +++++++++++++++++++++++++++++++++--------- src/ui/scm/actions.rs | 18 ++- 2 files changed, 216 insertions(+), 54 deletions(-) diff --git a/src/ui/prompt.rs b/src/ui/prompt.rs index 80c6459e..1612a976 100644 --- a/src/ui/prompt.rs +++ b/src/ui/prompt.rs @@ -1,27 +1,32 @@ -//! The confirmation dialog on platforms with no native one. +//! Every confirmation the app asks — `window.prompt` — drawn as one of the +//! app's own cards. //! -//! macOS and Windows answer `window.prompt` with NSAlert and TaskDialog. Linux -//! has neither, so gpui falls back to a renderer of its own — and that one sets -//! the message and the detail as single unbreakable lines inside a fixed-width -//! box that clips its overflow. Any sentence longer than the box is cut off -//! mid-word, which for the quit-and-stop warning is exactly the part that says -//! what will be lost (#920). This is the same dialog laid out as a card that -//! wraps its text, in the app's own surfaces, with Return and Escape answering -//! the way they do on the native dialogs. +//! gpui answers `window.prompt` with the platform's dialog where there is one: +//! NSAlert on macOS, TaskDialog on Windows, and on Linux a fallback of its own +//! that clipped any sentence longer than its box (#920). The native two were +//! correct and still wrong for this app. Every other question tty7 asks — the +//! SSH sheet, the worktree prompt, New Workspace — is a card built from +//! [`crate::ui::dialog`]: the switcher's 12px corner, a title row with an +//! `esc` cap, a hairline footer, an ink-filled primary. Closing a busy tab +//! instead threw up a system alert with a centred app icon and grey stacked +//! buttons, so the same app had two ideas of what a dialog looks like, and +//! which one you got depended on which question it was. +//! +//! So all three platforms use this. It is laid out like the other cards, sits +//! where they sit, wraps its text, and answers Return and Escape the way the +//! native dialogs did — which the call sites were written against. use gpui::{ App, Context, EventEmitter, FocusHandle, Focusable, PromptButton, PromptHandle, PromptLevel, - PromptResponse, RenderablePromptHandle, Window, div, prelude::*, px, relative, + PromptResponse, RenderablePromptHandle, Window, div, prelude::*, px, rems, }; -use gpui_component::button::{Button, ButtonVariants as _}; -use gpui_component::{ActiveTheme as _, Sizable as _, h_flex, v_flex}; +use gpui_component::{ActiveTheme as _, h_flex}; -/// Route every `window.prompt` through [`TextPrompt`] where the platform has -/// no dialog of its own to show. +use crate::ui::dialog::{self, Tone}; +use crate::ui::right_panel::{TAB_TEXT, TEXT}; + +/// Route every `window.prompt` through [`TextPrompt`]. pub(crate) fn install(cx: &mut App) { - if cfg!(any(target_os = "macos", target_os = "windows")) { - return; - } cx.set_prompt_builder(build); } @@ -38,6 +43,10 @@ fn build( handle.with_view(prompt, window, cx) } +/// A confirmation card is a sentence and two buttons; the worktree form, with +/// three fields, is 440. +const WIDTH: f32 = 420.; + pub(crate) struct TextPrompt { message: String, detail: Option, @@ -54,22 +63,63 @@ impl TextPrompt { ) -> Self { Self { message: message.to_string(), - detail: detail.map(str::to_string), + detail: detail.map(str::to_string).filter(|d| !d.trim().is_empty()), answers: answers.to_vec(), focus: cx.focus_handle(), } } /// The answer a key picks. Return is answer 0 — `confirm_answers` puts the - /// action there for exactly this — and Escape is the one marked cancel; - /// a prompt with no cancel answer has nothing Escape can safely mean. + /// action there for exactly this — and Escape is [`Self::cancel_answer`]; + /// a prompt with no way out has nothing Escape can safely mean. fn answer_for_key(&self, key: &str) -> Option { match key { "enter" if !self.answers.is_empty() => Some(0), - "escape" => self.answers.iter().position(PromptButton::is_cancel), + "escape" => self.cancel_answer(), _ => None, } } + + /// The answer that backs out: the one marked cancel, or a lone + /// acknowledgement — an `OK` with nothing else to pick is also the way + /// out, and a card Escape cannot close is a trap. + fn cancel_answer(&self) -> Option { + match self.answers.as_slice() { + [_] => Some(0), + answers => answers.iter().position(PromptButton::is_cancel), + } + } + + /// Whether answer `ix` sits apart, at the footer's far left. Answers + /// listed after the cancel in a prompt of three or more are the ones a + /// caller wants away from Return — Discard beside Save / Cancel — and + /// packed in with the rest it would sit one button from the default. + fn stands_apart(&self, ix: usize) -> bool { + self.answers.len() >= 3 + && self + .answers + .iter() + .position(PromptButton::is_cancel) + .is_some_and(|cancel| ix > cancel) + } + + /// How answer `ix` is painted. + /// + /// Answer 0 is the one Return picks, so it is the one filled in — unless + /// it is the cancel. A caller only puts Cancel first when the action is + /// one Return must never reach by accident (discarding changes, a hard + /// reset); that action is the one the card warns about, so it goes red, + /// and the safe answer keeps the quiet paint. + fn tone(&self, ix: usize) -> Tone { + let answer = &self.answers[ix]; + let cancel_first = self.answers.first().is_some_and(PromptButton::is_cancel); + match (ix, answer.is_cancel()) { + (_, true) => Tone::Secondary, + (0, false) => Tone::Primary, + (_, false) if cancel_first => Tone::Danger, + _ => Tone::Secondary, + } + } } impl EventEmitter for TextPrompt {} @@ -82,51 +132,86 @@ impl Focusable for TextPrompt { impl Render for TextPrompt { fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { - let muted = cx.theme().muted_foreground; + let theme = cx.theme(); + let muted = theme.muted_foreground; + let border = theme.border; + let rungs = dialog::popover_rungs(cx); + let escapable = self.cancel_answer().is_some(); + let has_detail = self.detail.is_some(); + // Answer 0 goes rightmost, where the native dialogs put it and where // `confirm_answers` expects it to land. - let buttons = self.answers.iter().enumerate().rev().map(|(ix, answer)| { - Button::new(("prompt-answer", ix)) - .label(answer.label().clone()) - .small() - .when(ix == 0 && !answer.is_cancel(), |b| b.primary()) - .on_click(cx.listener(move |_, _, _, cx| { + let mut apart = Vec::new(); + let mut packed = Vec::new(); + for ix in (0..self.answers.len()).rev() { + let button = dialog::button( + ("prompt-answer", ix), + self.answers[ix].label().clone(), + self.tone(ix), + true, + rungs, + cx, + cx.listener(move |_, _, _, cx| { cx.emit(PromptResponse(ix)); cx.stop_propagation(); - })) - }); + }), + ); + match self.stands_apart(ix) { + true => apart.push(button), + false => packed.push(button), + } + } - let card = v_flex() - .occlude() - .w(px(420.)) - .max_w(relative(0.9)) - .gap_3() - .p_5() - .map(|card| crate::ui::theme::floating_surface(card, cx)) + // The other cards' title row, except that the title wraps: theirs are + // names, which truncate cleanly, and this one is a question, which + // cut short no longer asks anything. One line still sits in the + // 48px row exactly where theirs does. + let title = h_flex() + .flex_none() + .items_center() + .gap(px(10.)) + .min_h(px(dialog::HEADER_H)) + .py(px(12.)) + .pl(px(dialog::INSET)) + .pr(px(14.)) + .when(has_detail, |row| row.border_b_1().border_color(border)) .child( div() - .text_sm() - .font_weight(gpui::FontWeight::SEMIBOLD) + .flex_1() + .min_w_0() + .text_size(rems(TEXT)) + .font_weight(gpui::FontWeight::MEDIUM) .child(self.message.clone()), ) - .children( - self.detail - .clone() - .map(|detail| div().text_sm().text_color(muted).child(detail)), - ) + .when(escapable, |row| row.child(dialog::keycap("esc", cx))); + + let card = dialog::card(WIDTH, cx) + .max_w(gpui::relative(0.9)) + .child(title) + .children(self.detail.clone().map(|detail| { + dialog::body().child( + div() + .text_size(rems(TAB_TEXT)) + .line_height(rems(TAB_TEXT * 1.5)) + .text_color(muted) + .child(detail), + ) + })) .child( - h_flex() - .pt_2() - .justify_end() - .flex_wrap() - .gap_2() - .children(buttons), + dialog::footer(cx) + .children(apart) + .when(self.answers.len() >= 3, |row| row.child(div().flex_1())) + .children(packed), ); div() .id("text-prompt") .track_focus(&self.focus) .size_full() + // Nothing under the scrim answers the pointer while the question + // is up: the prompt is painted over the window, not into it, so + // without this a click on the dimmed tab strip still switched tabs. + .occlude() .cursor_default() .bg(crate::ui::presets::scrim_fill(cx)) .on_key_down(cx.listener(|this, ev: &gpui::KeyDownEvent, _, cx| { @@ -135,9 +220,21 @@ impl Render for TextPrompt { cx.stop_propagation(); } })) + // Clicking the scrim backs out, as it does around the other + // cards — when there is a safe answer to back out to. + .on_mouse_down( + gpui::MouseButton::Left, + cx.listener(|this, _: &gpui::MouseDownEvent, _, cx| { + if let Some(ix) = this.cancel_answer() { + cx.emit(PromptResponse(ix)); + } + }), + ) .flex() + .flex_col() .items_center() - .justify_center() + .justify_start() + .pt(px(crate::ui::switcher::CARD_TOP)) .child(card) } } @@ -170,6 +267,59 @@ mod tests { }); } + #[gpui::test] + fn the_action_is_filled_and_the_safe_answer_stays_quiet(cx: &mut TestAppContext) { + use crate::ui::dialog::Tone; + let p = prompt(cx, &crate::ui::confirm_answers("Close", "Keep")); + p.read_with(cx, |p, _| { + assert_eq!(p.tone(0), Tone::Primary); + assert_eq!(p.tone(1), Tone::Secondary); + }); + } + + #[gpui::test] + fn cancel_first_marks_the_other_answer_as_the_dangerous_one(cx: &mut TestAppContext) { + // Discard puts Cancel at 0 so Return cannot throw work away; the card + // has to say which button does. + use crate::ui::dialog::Tone; + let p = prompt( + cx, + &[PromptButton::cancel("Cancel"), PromptButton::new("Discard")], + ); + p.read_with(cx, |p, _| { + assert_eq!(p.answer_for_key("enter"), Some(0)); + assert_eq!(p.answer_for_key("escape"), Some(0)); + assert_eq!(p.tone(0), Tone::Secondary); + assert_eq!(p.tone(1), Tone::Danger); + }); + } + + #[gpui::test] + fn discard_stands_apart_from_save_and_cancel(cx: &mut TestAppContext) { + let p = prompt( + cx, + &[ + PromptButton::ok("Save"), + PromptButton::cancel("Cancel"), + PromptButton::ok("Discard"), + ], + ); + p.read_with(cx, |p, _| { + assert!(!p.stands_apart(0)); + assert!(!p.stands_apart(1)); + assert!(p.stands_apart(2)); + assert_eq!(p.answer_for_key("escape"), Some(1)); + }); + } + + #[gpui::test] + fn a_lone_ok_answers_escape_too(cx: &mut TestAppContext) { + let p = prompt(cx, &[PromptButton::new("OK")]); + p.read_with(cx, |p, _| { + assert_eq!(p.answer_for_key("escape"), Some(0)); + }); + } + #[gpui::test] fn escape_does_nothing_without_a_cancel_answer(cx: &mut TestAppContext) { let p = prompt(cx, &[PromptButton::ok("OK"), PromptButton::new("Retry")]); diff --git a/src/ui/scm/actions.rs b/src/ui/scm/actions.rs index bdede8b6..27c8916f 100644 --- a/src/ui/scm/actions.rs +++ b/src/ui/scm/actions.rs @@ -4,7 +4,7 @@ //! the key binding, the palette entry and the button on the row — cannot drift //! into meaning different things. -use gpui::{Context, PromptLevel, Window}; +use gpui::{Context, PromptButton, PromptLevel, Window}; use tty7_core::core::git::ops::{Destructive, GitOp, PullMode}; @@ -133,7 +133,7 @@ impl Tty7App { PromptLevel::Warning, &confirm_question(&op, loss), None, - &[t(L10nKey::Cancel), confirm_verb(&op, loss)], + &guarded_answers(confirm_verb(&op, loss)), cx, ); cx.spawn_in(window, async move |app, cx| { @@ -320,7 +320,7 @@ impl Tty7App { PromptLevel::Warning, &t(L10nKey::ScmDiscardAllConfirm).to_string(), None, - &[t(L10nKey::Cancel), t(L10nKey::ScmDiscard)], + &guarded_answers(t(L10nKey::ScmDiscard)), cx, ); cx.spawn_in(window, async move |app, cx| { @@ -513,6 +513,18 @@ fn confirm_verb(op: &GitOp, loss: Destructive) -> &'static str { } } +/// Cancel first, so Return lands on it and not on the loss, then the action. +/// +/// Cancel is marked as one rather than passed as a bare label: gpui only +/// recognises a cancel by its label when that label is the English word, so +/// in any other language Escape had nothing to answer and the dialog could +/// not tell which button was the safe one. +fn guarded_answers(action: &str) -> [PromptButton; 2] { + [ + PromptButton::cancel(t(L10nKey::Cancel)), + PromptButton::new(action), + ] +} #[cfg(test)] mod tests { use super::*;