feat(ui): make opening a new window a bindable action (#710)

Opening a window was reachable one way only: the switcher, with the
platform modifier held or through its row menu. Both land in
`windows::open`, neither is an action, so the thing could not be
keybound, palette-searched, or put on the Keybindings page.

`NewWindow` calls `windows::open(cx, None)` — a window on a workspace of
its own. The other reading, a second window on the current workspace, is
not a thing the app can hold: `WindowRegistry` is keyed by workspace, and
`windows::open` answers a workspace that already has a window by
activating it, so asking for the current one would raise the window you
are already in.

macOS takes Cmd+N, which is free and is what New Window means there. Off
macOS it ships unbound on purpose: Ctrl+N is a control code the shell is
owed, and Ctrl+Shift+N — where the control-code rule says window actions
belong — has been NewWorkspace for far longer than this action has
existed. The palette and the Keybindings page carry it either way.

src/core/actions.rs

@@ -18,6 +18,7 @@ actions!(
         SelectWorkspace7,
         SelectWorkspace8,
         SelectWorkspace9,
+        NewWindow,
         CloseActiveTab,
         RenameTab,
         NewWorktreeTab,

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::<Tty7App>()
+                    .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"
+        );
+    }
+}

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…",

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 => "タブの名前を変更…",

src/ui/i18n/mod.rs

@@ -1123,6 +1123,7 @@ l10n_keys! {
     CmdGroupAgents,
     CmdGroupApplication,
     CmdNewTab,
+    CmdNewWindow,
     CmdNewWorktreeTab,
     CmdNewWorktreeTabSubtitle,
     CmdRenameTab,

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 => "重命名标签页…",

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<KeyBinding> {
         "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"))]

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),