diff --git a/src/core/actions.rs b/src/core/actions.rs index 09604411..7286eb1f 100644 --- a/src/core/actions.rs +++ b/src/core/actions.rs @@ -18,6 +18,7 @@ actions!( SelectWorkspace7, SelectWorkspace8, SelectWorkspace9, + NewWindow, CloseActiveTab, RenameTab, NewWorktreeTab, diff --git a/src/ui/app.rs b/src/ui/app.rs index dea55890..737cedf6 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1468,6 +1468,22 @@ impl Tty7App { crate::ui::windows::refresh_menu(cx); } + /// Opens a second window, on a workspace of its own. + /// + /// A window on *this* workspace is not the other reading of "new window"; + /// it is a thing the app cannot hold. `WindowRegistry` is keyed by + /// workspace — `window_for`, `app_for`, `unregister` and `rebind` all + /// address a window by the workspace it shows — and `windows::open` + /// answers a workspace that already has a window by activating it. Asking + /// for the current one here would raise the window you are already in. + /// + /// So this is the same call the switcher makes for "Open in New Window", + /// with no workspace named: a fresh one, which is also what a new window + /// holds everywhere else it is offered. + pub(crate) fn new_window(&self, cx: &mut App) { + crate::ui::windows::open(cx, None); + } + pub(crate) fn teardown_workspace_forwards(&self, cx: &gpui::App) { let Some(route) = self .tabs @@ -4892,6 +4908,7 @@ impl Tty7App { match kind { NewTab => self.new_tab(window, cx), NewWorkspace => self.open_workspace_form(window, cx), + NewWindow => self.new_window(cx), OpenWorkspacePicker => self.open_switcher(window, cx), StopWorkspace => self.stop_workspace(self.workspace, window, cx), DeleteWorkspace => self.delete_workspace(self.workspace, window, cx), @@ -7391,6 +7408,9 @@ impl Render for Tty7App { .on_action(cx.listener(|this, _: &NewWorkspace, window, cx| { this.open_workspace_form(window, cx); })) + .on_action(cx.listener(|this, _: &NewWindow, _window, cx| { + this.new_window(cx); + })) .on_action(cx.listener(|this, _: &CloseActiveTab, window, cx| { if !this.editor_close_active_if_focused(window, cx) { this.close_pane(window, cx) @@ -10244,3 +10264,85 @@ mod managed_forward_gpui_tests { }); } } + +#[cfg(test)] +mod new_window_action_tests { + use crate::core::actions::NewWindow; + use crate::core::config::Config; + use crate::core::session::Session; + use crate::ui::app::Tty7App; + use crate::ui::windows::WindowRegistry; + use gpui::{AppContext as _, TestAppContext, VisualTestContext}; + + /// `NewWindow` has to open a window, not merely exist. + /// + /// Everything else about the action is a table entry — the `actions!` + /// row, the keymap slot, the palette command — and every one of those can + /// be there while the action reaches nothing. This drives the real + /// dispatch path and then asks the registry, so the assertion is "a second + /// window is open, on a workspace of its own, and the first one is still + /// here": the same `windows::open` the switcher calls for "Open in New + /// Window", with no workspace named. + #[gpui::test] + fn dispatching_new_window_opens_a_second_window_beside_the_first(cx: &mut TestAppContext) { + crate::core::config::pin_test_config_dir(); + cx.executor().allow_parking(); + cx.update(|cx| { + gpui_component::init(cx); + cx.set_global(Config::default()); + crate::ui::keymap::init(cx); + WindowRegistry::init(cx); + }); + let window = cx.add_window(|window, cx| { + let app = + cx.new(|cx| Tty7App::with_session(None, Some(Session::default()), window, cx)); + gpui_component::Root::new(app, window, cx) + }); + let app = window + .update(cx, |root, _, _| { + root.view() + .clone() + .downcast::() + .ok() + .expect("window root wraps a Tty7App") + }) + .unwrap(); + // Registered the way an opened window registers itself; without it the + // registry cannot tell the two windows apart afterwards. + let handle = window.into(); + let weak = app.downgrade(); + app.update(cx, |app, cx| { + WindowRegistry::register(cx, app.workspace, handle, weak); + }); + + let mut vcx = VisualTestContext::from_window(handle, cx); + vcx.background_executor.run_until_parked(); + let first = app.update(&mut vcx, |app, _| app.workspace); + assert_eq!( + vcx.update(|_, cx| WindowRegistry::count(cx)), + 1, + "the harness starts with exactly the one window" + ); + + vcx.dispatch_action(NewWindow); + vcx.background_executor.run_until_parked(); + + let open = vcx.update(|_, cx| WindowRegistry::open_windows(cx)); + assert_eq!( + open.len(), + 2, + "NewWindow has to reach windows::open; it opened {} window(s)", + open.len() + ); + assert!( + open.iter().any(|(id, _)| *id == first), + "the window the action was fired from must survive it" + ); + // The registry is keyed by workspace, so a second window on the + // current one is not a thing it could tell apart from the first. + assert!( + open.iter().any(|(id, _)| *id != first), + "the new window belongs on a workspace of its own" + ); + } +} diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index eb62d36f..494a0faf 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1403,6 +1403,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::CmdGroupAgents => "Agents", L10nKey::CmdGroupApplication => "Application", L10nKey::CmdNewTab => "New Tab", + L10nKey::CmdNewWindow => "New Window", L10nKey::CmdNewWorktreeTab => "New Worktree Tab…", L10nKey::CmdNewWorktreeTabSubtitle => "isolated checkout on a fresh branch", L10nKey::CmdRenameTab => "Rename Tab…", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 367c4d95..896cafb4 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1460,6 +1460,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::CmdGroupAgents => "エージェント", L10nKey::CmdGroupApplication => "アプリケーション", L10nKey::CmdNewTab => "新しいタブ", + L10nKey::CmdNewWindow => "新しいウィンドウ", L10nKey::CmdNewWorktreeTab => "新しいワークツリータブ…", L10nKey::CmdNewWorktreeTabSubtitle => "新しいブランチでの独立したチェックアウト", L10nKey::CmdRenameTab => "タブの名前を変更…", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 1b54fbca..f8b27fef 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -1123,6 +1123,7 @@ l10n_keys! { CmdGroupAgents, CmdGroupApplication, CmdNewTab, + CmdNewWindow, CmdNewWorktreeTab, CmdNewWorktreeTabSubtitle, CmdRenameTab, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index f802207a..fabd615f 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1321,6 +1321,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::CmdGroupAgents => "Agents", L10nKey::CmdGroupApplication => "应用", L10nKey::CmdNewTab => "新标签页", + L10nKey::CmdNewWindow => "新建窗口", L10nKey::CmdNewWorktreeTab => "新建 worktree 标签页…", L10nKey::CmdNewWorktreeTabSubtitle => "在全新分支上独立检出", L10nKey::CmdRenameTab => "重命名标签页…", diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 67fd2c97..f644c0db 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -265,6 +265,16 @@ pub(crate) fn default_bindings() -> Vec<(&'static str, &'static str)> { vec![ ("NewTab", per_platform("secondary-t", "secondary-shift-t")), ("NewWorkspace", "secondary-shift-n"), + // Cmd+N is what "New Window" means on macOS, and nothing else here + // claims it. Off macOS both chords the convention offers are gone: + // Ctrl+N is a C0 byte the shell is owed, which + // `no_default_binding_sits_on_a_terminal_control_code` fails a build + // over, and Ctrl+Shift+N — where that same test says window actions + // belong — has been `NewWorkspace` for far longer than this action has + // existed. Minting an unguessable third chord would be worse than + // shipping unbound: the palette and the Keybindings page both carry + // this, so a key is one line of config away. + ("NewWindow", per_platform("secondary-n", "")), ( "CloseActiveTab", per_platform("secondary-w", "secondary-shift-w"), @@ -733,6 +743,10 @@ fn authored_entry(action: &str) -> Option<(CommandGroup, String)> { CommandGroup::Application, t(L10nKey::AppMenuCommandPalette).to_string(), ), + "NewWindow" => ( + CommandGroup::Application, + t(L10nKey::CmdNewWindow).to_string(), + ), "OpenSettings" => ( CommandGroup::Application, t(L10nKey::CmdSettings).to_string(), @@ -1036,6 +1050,7 @@ fn make_binding(action: &str, keystroke: &str) -> Option { "DeleteWorkspace" => KeyBinding::new(keystroke, DeleteWorkspace, None), "RenameWorkspace" => KeyBinding::new(keystroke, RenameWorkspace, None), "ToggleSwitcher" => KeyBinding::new(keystroke, ToggleSwitcher, None), + "NewWindow" => KeyBinding::new(keystroke, NewWindow, None), "CloseActiveTab" => KeyBinding::new(keystroke, CloseActiveTab, None), "RenameTab" => KeyBinding::new(keystroke, RenameTab, None), "NewWorktreeTab" => KeyBinding::new(keystroke, NewWorktreeTab, None), @@ -1217,6 +1232,7 @@ mod tests { // palette and the docs all say Zoom Pane. assert_eq!(action_entry("ToggleMaximizePane").1, "Zoom Pane"); assert_eq!(action_entry("CloseActiveTab").1, "Close Pane / Tab"); + assert_eq!(action_entry("NewWindow").1, "New Window"); assert_eq!(action_entry("ClearScrollback").1, "Clear Scrollback"); assert_eq!(action_entry("TogglePalette").1, "Command Palette…"); assert_eq!(action_entry("ToggleSwitcher").1, "Switch Workspace…"); @@ -1230,6 +1246,57 @@ mod tests { assert_eq!(action_entry("ForkAgentSessionUp").0, CommandGroup::Agents); } + #[test] + fn new_window_ships_a_chord_only_where_one_is_free() { + let mut effective: Vec<(String, String)> = default_bindings() + .into_iter() + .map(|(a, k)| (a.to_string(), k.to_string())) + .collect(); + let default = effective + .iter() + .find(|(action, _)| action == "NewWindow") + .map(|(_, key)| key.clone()) + .expect("NewWindow has to be listed here or it cannot be bound at all"); + assert_eq!( + default, + if cfg!(target_os = "macos") { + "secondary-n" + } else { + "" + }, + "macOS gets Cmd+N; off macOS this ships unbound on purpose" + ); + + // Ask the keymap rather than the table. A default can look bound and + // dispatch nothing — gpui folds shift into the punctuation glyph, so + // `secondary-shift-]` reached no action at all off macOS (#750). An + // exact match is also the conflict check: any other action holding + // this chord would show up in the list. + if !default.is_empty() { + assert_eq!( + dispatched(&effective, &default, "Terminal"), + vec![NewWindow::name_for_type()], + "{default} must reach NewWindow, and nothing else may answer it" + ); + } + + // Unbound still has to mean bindable. `set_binding` only writes into + // slots this table already has, and `make_binding` is what turns the + // name back into a dispatchable binding; miss either and the action + // sits on the Keybindings page, takes a key, and does nothing — which + // is the whole complaint in #710, not just the missing default. + effective + .iter_mut() + .find(|(action, _)| action == "NewWindow") + .expect("found once already") + .1 = "ctrl-alt-shift-n".to_string(); + assert_eq!( + dispatched(&effective, "ctrl-alt-shift-n", "Terminal"), + vec![NewWindow::name_for_type()], + "a chord the user assigns to NewWindow has to reach it" + ); + } + #[cfg(target_os = "macos")] const SECONDARY: &str = "⌘"; #[cfg(not(target_os = "macos"))] diff --git a/src/ui/palette.rs b/src/ui/palette.rs index 28272380..09c03a7c 100644 --- a/src/ui/palette.rs +++ b/src/ui/palette.rs @@ -22,6 +22,7 @@ pub enum CommandKind { RenameWorkspace, StopWorkspace, DeleteWorkspace, + NewWindow, SplitRight, SplitDown, ClosePane, @@ -124,6 +125,7 @@ impl CommandKind { RenameWorkspace => "rename-workspace", StopWorkspace => "stop-workspace", DeleteWorkspace => "delete-workspace", + NewWindow => "new-window", SplitRight => "split-right", SplitDown => "split-down", ClosePane => "close-pane", @@ -230,6 +232,7 @@ impl CommandKind { RenameWorkspace => "RenameWorkspace", StopWorkspace => "StopWorkspace", DeleteWorkspace => "DeleteWorkspace", + NewWindow => "NewWindow", SplitRight => "SplitRight", SplitDown => "SplitDown", ClosePane => "CloseActiveTab", @@ -565,6 +568,7 @@ impl Command { ]; let application = [ + Command::localized(L10nKey::CmdNewWindow, NewWindow), Command::localized(L10nKey::CmdSettings, OpenSettings), Command::localized(L10nKey::CmdKeyboardShortcuts, ShowKeyboardShortcuts), Command::localized(L10nKey::CmdAboutTty7, About),