From 465d60def86c86593eb28d9d4b39aa4a06452326 Mon Sep 17 00:00:00 2001 From: axdank Date: Thu, 24 Oct 2024 00:01:54 -0300 Subject: [PATCH 01/18] gui: add move_current_tab action --- src/Surface.zig | 6 ++++++ src/apprt/action.zig | 10 ++++++++++ src/apprt/glfw.zig | 1 + src/apprt/gtk/App.zig | 18 ++++++++++++++++++ src/apprt/gtk/Window.zig | 9 +++++++++ src/apprt/gtk/notebook.zig | 27 +++++++++++++++++++++++++++ src/input/Binding.zig | 4 ++++ 7 files changed, 75 insertions(+) diff --git a/src/Surface.zig b/src/Surface.zig index bd5073e3a..dca9c6316 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -3913,6 +3913,12 @@ pub fn performBindingAction(self: *Surface, action: input.Binding.Action) !bool }, ), + .move_current_tab => |position| try self.rt_app.performAction( + .{ .surface = self }, + .move_current_tab, + position, + ), + .new_split => |direction| try self.rt_app.performAction( .{ .surface = self }, .new_split, diff --git a/src/apprt/action.zig b/src/apprt/action.zig index 9fce8502f..e9c1f1005 100644 --- a/src/apprt/action.zig +++ b/src/apprt/action.zig @@ -100,6 +100,9 @@ pub const Action = union(Key) { /// Toggle the visibility of all Ghostty terminal windows. toggle_visibility, + /// Move current tab given a position + move_current_tab: isize, + /// Jump to a specific tab. Must handle the scenario that the tab /// value is invalid. goto_tab: GotoTab, @@ -190,6 +193,7 @@ pub const Action = union(Key) { toggle_window_decorations, toggle_quick_terminal, toggle_visibility, + move_current_tab, goto_tab, goto_split, resize_split, @@ -318,6 +322,12 @@ pub const GotoTab = enum(c_int) { _, }; +/// Move current tab . +pub const MoveCurrentTabDirection = enum(c_int) { + right, + left, +}; + /// The fullscreen mode to toggle to if we're moving to fullscreen. pub const Fullscreen = enum(c_int) { native, diff --git a/src/apprt/glfw.zig b/src/apprt/glfw.zig index 668dd9143..49eecab32 100644 --- a/src/apprt/glfw.zig +++ b/src/apprt/glfw.zig @@ -213,6 +213,7 @@ pub const App = struct { .toggle_quick_terminal, .toggle_visibility, .goto_tab, + .move_current_tab, .inspector, .render_inspector, .quit_timer, diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 10a2bdc02..595e8e5b6 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -456,6 +456,7 @@ pub fn performAction( .new_tab => try self.newTab(target), .goto_tab => self.gotoTab(target, value), + .move_current_tab => self.moveCurrentTab(target, value), .new_split => try self.newSplit(target, value), .resize_split => self.resizeSplit(target, value), .equalize_splits => self.equalizeSplits(target), @@ -527,6 +528,23 @@ fn gotoTab(_: *App, target: apprt.Target, tab: apprt.action.GotoTab) void { } } +fn moveCurrentTab(_: *App, target: apprt.Target, position: isize) void { + switch (target) { + .app => {}, + .surface => |v| { + const window = v.rt_surface.container.window() orelse { + log.info( + "moveCurrentTab invalid for container={s}", + .{@tagName(v.rt_surface.container)}, + ); + return; + }; + + window.moveCurrentTab(v.rt_surface, @intCast(position)); + }, + } +} + fn newSplit( self: *App, target: apprt.Target, diff --git a/src/apprt/gtk/Window.zig b/src/apprt/gtk/Window.zig index 65041d600..8fb6cac77 100644 --- a/src/apprt/gtk/Window.zig +++ b/src/apprt/gtk/Window.zig @@ -456,6 +456,15 @@ pub fn gotoNextTab(self: *Window, surface: *Surface) void { self.focusCurrentTab(); } +/// Move the current tab for a surface. +pub fn moveCurrentTab(self: *Window, surface: *Surface, position: c_int) void { + const tab = surface.container.tab() orelse { + log.info("surface is not attached to a tab bar, cannot navigate", .{}); + return; + }; + self.notebook.moveTab(tab, position); +} + /// Go to the next tab for a surface. pub fn gotoLastTab(self: *Window) void { const max = self.notebook.nPages() -| 1; diff --git a/src/apprt/gtk/notebook.zig b/src/apprt/gtk/notebook.zig index 53cadc2d2..dce729989 100644 --- a/src/apprt/gtk/notebook.zig +++ b/src/apprt/gtk/notebook.zig @@ -183,6 +183,33 @@ pub const Notebook = union(enum) { self.gotoNthTab(next_idx); } + pub fn moveTab(self: Notebook, tab: *Tab, position: c_int) void { + switch (self) { + .gtk_notebook => |notebook| { + c.gtk_notebook_reorder_child(notebook, @ptrCast(tab.box), position); + }, + .adw_tab_view => |tab_view| { + if (comptime !adwaita.versionAtLeast(0, 0, 0)) unreachable; + const page = c.adw_tab_view_get_page(tab_view, @ptrCast(tab.box)); + + const page_idx = self.getTabPosition(tab) orelse return; + + const max = self.nPages() -| 1; + var new_position: c_int = page_idx + position; + + if (new_position < 0) { + new_position = max; + } else if (new_position > max) { + new_position = 0; + } + + if (new_position == page_idx) return; + + _ = c.adw_tab_view_reorder_page(tab_view, page, new_position); + }, + } + } + pub fn setTabLabel(self: Notebook, tab: *Tab, title: [:0]const u8) void { switch (self) { .adw_tab_view => |tab_view| { diff --git a/src/input/Binding.zig b/src/input/Binding.zig index c9e90d946..943a704c4 100644 --- a/src/input/Binding.zig +++ b/src/input/Binding.zig @@ -300,6 +300,9 @@ pub const Action = union(enum) { /// Go to the tab with the specific number, 1-indexed. goto_tab: usize, + /// Move current tab to a position + move_current_tab: isize, + /// Toggle the tab overview. /// This only works with libadwaita enabled currently. toggle_tab_overview: void, @@ -646,6 +649,7 @@ pub const Action = union(enum) { .next_tab, .last_tab, .goto_tab, + .move_current_tab, .toggle_tab_overview, .new_split, .goto_split, From 23927d1fda61ec05c66fd760f00165f1cef89e07 Mon Sep 17 00:00:00 2001 From: axdank Date: Thu, 24 Oct 2024 00:11:04 -0300 Subject: [PATCH 02/18] removing unnecessary enum --- src/apprt/action.zig | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/apprt/action.zig b/src/apprt/action.zig index e9c1f1005..703becbd8 100644 --- a/src/apprt/action.zig +++ b/src/apprt/action.zig @@ -322,12 +322,6 @@ pub const GotoTab = enum(c_int) { _, }; -/// Move current tab . -pub const MoveCurrentTabDirection = enum(c_int) { - right, - left, -}; - /// The fullscreen mode to toggle to if we're moving to fullscreen. pub const Fullscreen = enum(c_int) { native, From 3c8d9ae5a3bb10a05bba96ec2c10938297604980 Mon Sep 17 00:00:00 2001 From: Mohammad Al-Ahdal Date: Thu, 24 Oct 2024 03:00:17 -0600 Subject: [PATCH 03/18] Fix: aerospace no longer resizes quick terminal and instead treats it as float --- .../Sources/Features/QuickTerminal/QuickTerminalWindow.swift | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift b/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift index 2d9d1df7c..37f80260c 100644 --- a/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift +++ b/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift @@ -12,6 +12,11 @@ class QuickTerminalWindow: NSWindow { // Note: almost all of this stuff can be done in the nib/xib directly // but I prefer to do it programmatically because the properties we // care about are less hidden. + + // Set the base style mask to be HUD Window (which satisfies the AeroSpace + // window manager's heuristics allowing the quick terminal window to be + // unaffected by AeroSpace's window positioning and resizing) + self.styleMask = .hudWindow // Remove the title completely. This will make the window square. One // downside is it also hides the cursor indications of resize but the From 5a1d09bcc6ec02a97b446ab5d6774c53e6e50cda Mon Sep 17 00:00:00 2001 From: Mohammad Al-Ahdal Date: Fri, 25 Oct 2024 03:57:39 -0600 Subject: [PATCH 04/18] Suggestion to use window identifier instead of trying to appease heuristics --- .../Features/QuickTerminal/QuickTerminalWindow.swift | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift b/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift index 37f80260c..46707e255 100644 --- a/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift +++ b/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift @@ -13,10 +13,9 @@ class QuickTerminalWindow: NSWindow { // but I prefer to do it programmatically because the properties we // care about are less hidden. - // Set the base style mask to be HUD Window (which satisfies the AeroSpace - // window manager's heuristics allowing the quick terminal window to be - // unaffected by AeroSpace's window positioning and resizing) - self.styleMask = .hudWindow + // Add a custom identifier so third party apps can use the Accessibility + // API to apply special rules to the quick terminal. + self.identifier = .init(rawValue: "com.mitchellh.ghostty.quickTerminal") // Remove the title completely. This will make the window square. One // downside is it also hides the cursor indications of resize but the From 520dda65cbd50a48b45acf834b830f2938749492 Mon Sep 17 00:00:00 2001 From: axdank Date: Fri, 25 Oct 2024 08:07:11 -0300 Subject: [PATCH 05/18] apply review changes --- src/Surface.zig | 4 ++-- src/apprt/action.zig | 8 +++++--- src/apprt/glfw.zig | 2 +- src/apprt/gtk/App.zig | 8 ++++---- src/apprt/gtk/Window.zig | 2 +- src/apprt/gtk/notebook.zig | 32 +++++++++++++++++--------------- src/input/Binding.zig | 8 +++++--- 7 files changed, 35 insertions(+), 29 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index dca9c6316..0d39e6dcd 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -3913,9 +3913,9 @@ pub fn performBindingAction(self: *Surface, action: input.Binding.Action) !bool }, ), - .move_current_tab => |position| try self.rt_app.performAction( + .move_tab => |position| try self.rt_app.performAction( .{ .surface = self }, - .move_current_tab, + .move_tab, position, ), diff --git a/src/apprt/action.zig b/src/apprt/action.zig index 703becbd8..20d480e63 100644 --- a/src/apprt/action.zig +++ b/src/apprt/action.zig @@ -100,8 +100,10 @@ pub const Action = union(Key) { /// Toggle the visibility of all Ghostty terminal windows. toggle_visibility, - /// Move current tab given a position - move_current_tab: isize, + /// Moves a tab by a relative offset. + /// Adjusts the tab position based on `offset` (e.g., -1 for left, +1 for right). + /// If the new position is out of bounds, it wraps around cyclically within the tab range. + move_tab: isize, /// Jump to a specific tab. Must handle the scenario that the tab /// value is invalid. @@ -193,7 +195,7 @@ pub const Action = union(Key) { toggle_window_decorations, toggle_quick_terminal, toggle_visibility, - move_current_tab, + move_tab, goto_tab, goto_split, resize_split, diff --git a/src/apprt/glfw.zig b/src/apprt/glfw.zig index 49eecab32..1dde97c9c 100644 --- a/src/apprt/glfw.zig +++ b/src/apprt/glfw.zig @@ -213,7 +213,7 @@ pub const App = struct { .toggle_quick_terminal, .toggle_visibility, .goto_tab, - .move_current_tab, + .move_tab, .inspector, .render_inspector, .quit_timer, diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 595e8e5b6..6b302493f 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -456,7 +456,7 @@ pub fn performAction( .new_tab => try self.newTab(target), .goto_tab => self.gotoTab(target, value), - .move_current_tab => self.moveCurrentTab(target, value), + .move_tab => self.moveTab(target, value), .new_split => try self.newSplit(target, value), .resize_split => self.resizeSplit(target, value), .equalize_splits => self.equalizeSplits(target), @@ -528,19 +528,19 @@ fn gotoTab(_: *App, target: apprt.Target, tab: apprt.action.GotoTab) void { } } -fn moveCurrentTab(_: *App, target: apprt.Target, position: isize) void { +fn moveTab(_: *App, target: apprt.Target, position: isize) void { switch (target) { .app => {}, .surface => |v| { const window = v.rt_surface.container.window() orelse { log.info( - "moveCurrentTab invalid for container={s}", + "moveTab invalid for container={s}", .{@tagName(v.rt_surface.container)}, ); return; }; - window.moveCurrentTab(v.rt_surface, @intCast(position)); + window.moveTab(v.rt_surface, @intCast(position)); }, } } diff --git a/src/apprt/gtk/Window.zig b/src/apprt/gtk/Window.zig index 8fb6cac77..1ef6f9c24 100644 --- a/src/apprt/gtk/Window.zig +++ b/src/apprt/gtk/Window.zig @@ -457,7 +457,7 @@ pub fn gotoNextTab(self: *Window, surface: *Surface) void { } /// Move the current tab for a surface. -pub fn moveCurrentTab(self: *Window, surface: *Surface, position: c_int) void { +pub fn moveTab(self: *Window, surface: *Surface, position: c_int) void { const tab = surface.container.tab() orelse { log.info("surface is not attached to a tab bar, cannot navigate", .{}); return; diff --git a/src/apprt/gtk/notebook.zig b/src/apprt/gtk/notebook.zig index dce729989..46245ce99 100644 --- a/src/apprt/gtk/notebook.zig +++ b/src/apprt/gtk/notebook.zig @@ -184,6 +184,22 @@ pub const Notebook = union(enum) { } pub fn moveTab(self: Notebook, tab: *Tab, position: c_int) void { + const page_idx = self.getTabPosition(tab) orelse return; + + const max = self.nPages() -| 1; + var new_position: c_int = page_idx + position; + + if (new_position < 0) { + new_position = max + new_position + 1; + } else if (new_position > max) { + new_position = new_position - max - 1; + } + + if (new_position == page_idx) return; + self.reorderPage(tab, new_position); + } + + pub fn reorderPage(self: Notebook, tab: *Tab, position: c_int) void { switch (self) { .gtk_notebook => |notebook| { c.gtk_notebook_reorder_child(notebook, @ptrCast(tab.box), position); @@ -191,21 +207,7 @@ pub const Notebook = union(enum) { .adw_tab_view => |tab_view| { if (comptime !adwaita.versionAtLeast(0, 0, 0)) unreachable; const page = c.adw_tab_view_get_page(tab_view, @ptrCast(tab.box)); - - const page_idx = self.getTabPosition(tab) orelse return; - - const max = self.nPages() -| 1; - var new_position: c_int = page_idx + position; - - if (new_position < 0) { - new_position = max; - } else if (new_position > max) { - new_position = 0; - } - - if (new_position == page_idx) return; - - _ = c.adw_tab_view_reorder_page(tab_view, page, new_position); + _ = c.adw_tab_view_reorder_page(tab_view, page, position); }, } } diff --git a/src/input/Binding.zig b/src/input/Binding.zig index 943a704c4..c4d5c6cdf 100644 --- a/src/input/Binding.zig +++ b/src/input/Binding.zig @@ -300,8 +300,10 @@ pub const Action = union(enum) { /// Go to the tab with the specific number, 1-indexed. goto_tab: usize, - /// Move current tab to a position - move_current_tab: isize, + /// Moves a tab by a relative offset. + /// Adjusts the tab position based on `offset` (e.g., -1 for left, +1 for right). + /// If the new position is out of bounds, it wraps around cyclically within the tab range. + move_tab: isize, /// Toggle the tab overview. /// This only works with libadwaita enabled currently. @@ -649,7 +651,7 @@ pub const Action = union(enum) { .next_tab, .last_tab, .goto_tab, - .move_current_tab, + .move_tab, .toggle_tab_overview, .new_split, .goto_split, From 88119d0c17827ae28d9ab1d5bfb7cf0ccd119d4a Mon Sep 17 00:00:00 2001 From: Mohammad Al-Ahdal Date: Fri, 25 Oct 2024 05:37:31 -0600 Subject: [PATCH 06/18] default AXSubrole to .floatingWindow --- .../Sources/Features/QuickTerminal/QuickTerminalWindow.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift b/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift index 46707e255..ed3a7f781 100644 --- a/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift +++ b/macos/Sources/Features/QuickTerminal/QuickTerminalWindow.swift @@ -16,6 +16,10 @@ class QuickTerminalWindow: NSWindow { // Add a custom identifier so third party apps can use the Accessibility // API to apply special rules to the quick terminal. self.identifier = .init(rawValue: "com.mitchellh.ghostty.quickTerminal") + + // Set the correct AXSubrole of kAXFloatingWindowSubrole (allows + // AeroSpace to treat the Quick Terminal as a floating window) + self.setAccessibilitySubrole(.floatingWindow) // Remove the title completely. This will make the window square. One // downside is it also hides the cursor indications of resize but the From 4885ffd04287e65b0781e01045d3fde1a360c3f3 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 25 Oct 2024 09:34:42 -0700 Subject: [PATCH 07/18] input: note that toggle_split_zoom is macOS only --- src/input/Binding.zig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/input/Binding.zig b/src/input/Binding.zig index 64016659a..1615cf035 100644 --- a/src/input/Binding.zig +++ b/src/input/Binding.zig @@ -312,7 +312,8 @@ pub const Action = union(enum) { /// Focus on a split in a given direction. goto_split: SplitFocusDirection, - /// zoom/unzoom the current split. + /// zoom/unzoom the current split. This is currently only supported + /// on macOS. Contributions welcome for other platforms. toggle_split_zoom: void, /// Resize the current split by moving the split divider in the given From de5ec5d83e19e0026a5b76f2cd18951e7c129b7f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 25 Oct 2024 11:53:45 -0700 Subject: [PATCH 08/18] macos: make move_tab work --- include/ghostty.h | 8 ++++ macos/Ghostty.xcodeproj/project.pbxproj | 4 ++ .../Terminal/TerminalController.swift | 43 +++++++++++++++++++ macos/Sources/Ghostty/Ghostty.Action.swift | 15 +++++++ macos/Sources/Ghostty/Ghostty.App.swift | 28 ++++++++++++ macos/Sources/Ghostty/Package.swift | 10 ++++- src/Surface.zig | 2 +- src/apprt/action.zig | 12 ++++-- 8 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 macos/Sources/Ghostty/Ghostty.Action.swift diff --git a/include/ghostty.h b/include/ghostty.h index 0f4c65f56..ca70456d8 100644 --- a/include/ghostty.h +++ b/include/ghostty.h @@ -15,6 +15,7 @@ extern "C" { #include #include #include +#include //------------------------------------------------------------------- // Macros @@ -379,6 +380,11 @@ typedef struct { ghostty_action_resize_split_direction_e direction; } ghostty_action_resize_split_s; +// apprt.action.MoveTab +typedef struct { + ssize_t amount; +} ghostty_action_move_tab_s; + // apprt.action.GotoTab typedef enum { GHOSTTY_GOTO_TAB_PREVIOUS = -1, @@ -517,6 +523,7 @@ typedef enum { GHOSTTY_ACTION_TOGGLE_WINDOW_DECORATIONS, GHOSTTY_ACTION_TOGGLE_QUICK_TERMINAL, GHOSTTY_ACTION_TOGGLE_VISIBILITY, + GHOSTTY_ACTION_MOVE_TAB, GHOSTTY_ACTION_GOTO_TAB, GHOSTTY_ACTION_GOTO_SPLIT, GHOSTTY_ACTION_RESIZE_SPLIT, @@ -543,6 +550,7 @@ typedef enum { typedef union { ghostty_action_split_direction_e new_split; ghostty_action_fullscreen_e toggle_fullscreen; + ghostty_action_move_tab_s move_tab; ghostty_action_goto_tab_e goto_tab; ghostty_action_goto_split_e goto_split; ghostty_action_resize_split_s resize_split; diff --git a/macos/Ghostty.xcodeproj/project.pbxproj b/macos/Ghostty.xcodeproj/project.pbxproj index 57070dc47..414719f10 100644 --- a/macos/Ghostty.xcodeproj/project.pbxproj +++ b/macos/Ghostty.xcodeproj/project.pbxproj @@ -32,6 +32,7 @@ A5333E242B5A22D9008AEFF7 /* Ghostty.Shell.swift in Sources */ = {isa = PBXBuildFile; fileRef = A56D58852ACDDB4100508D2C /* Ghostty.Shell.swift */; }; A53426352A7DA53D00EBB7A2 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = A53426342A7DA53D00EBB7A2 /* AppDelegate.swift */; }; A535B9DA299C569B0017E2E4 /* ErrorView.swift in Sources */ = {isa = PBXBuildFile; fileRef = A535B9D9299C569B0017E2E4 /* ErrorView.swift */; }; + A53A6C032CCC1B7F00943E98 /* Ghostty.Action.swift in Sources */ = {isa = PBXBuildFile; fileRef = A53A6C022CCC1B7D00943E98 /* Ghostty.Action.swift */; }; A53D0C8E2B53B0EA00305CE6 /* GhosttyKit.xcframework in Frameworks */ = {isa = PBXBuildFile; fileRef = A5D495A1299BEC7E00DD1313 /* GhosttyKit.xcframework */; }; A53D0C942B53B43700305CE6 /* iOSApp.swift in Sources */ = {isa = PBXBuildFile; fileRef = A53D0C932B53B43700305CE6 /* iOSApp.swift */; }; A53D0C952B53B4D800305CE6 /* Assets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = A5B30538299BEAAB0047F10C /* Assets.xcassets */; }; @@ -115,6 +116,7 @@ A5333E212B5A2128008AEFF7 /* SurfaceView_AppKit.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SurfaceView_AppKit.swift; sourceTree = ""; }; A53426342A7DA53D00EBB7A2 /* AppDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AppDelegate.swift; sourceTree = ""; }; A535B9D9299C569B0017E2E4 /* ErrorView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ErrorView.swift; sourceTree = ""; }; + A53A6C022CCC1B7D00943E98 /* Ghostty.Action.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Ghostty.Action.swift; sourceTree = ""; }; A53D0C932B53B43700305CE6 /* iOSApp.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = iOSApp.swift; sourceTree = ""; }; A53D0C992B543F3B00305CE6 /* Ghostty.App.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Ghostty.App.swift; sourceTree = ""; }; A54D786B2CA79788001B19B1 /* BaseTerminalController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BaseTerminalController.swift; sourceTree = ""; }; @@ -317,6 +319,7 @@ A59FB5CE2AE0DB50009128F3 /* InspectorView.swift */, A53D0C992B543F3B00305CE6 /* Ghostty.App.swift */, A514C8D52B54A16400493A16 /* Ghostty.Config.swift */, + A53A6C022CCC1B7D00943E98 /* Ghostty.Action.swift */, A5278A9A2AA05B2600CD3039 /* Ghostty.Input.swift */, A56D58852ACDDB4100508D2C /* Ghostty.Shell.swift */, A59630A32AF059BB00D64628 /* Ghostty.SplitNode.swift */, @@ -601,6 +604,7 @@ A57D79272C9C879B001D522E /* SecureInput.swift in Sources */, A5CEAFDC29B8009000646FDA /* SplitView.swift in Sources */, A5CDF1932AAF9E0800513312 /* ConfigurationErrorsController.swift in Sources */, + A53A6C032CCC1B7F00943E98 /* Ghostty.Action.swift in Sources */, A59FB5D12AE0DEA7009128F3 /* MetalView.swift in Sources */, A55685E029A03A9F004303CE /* AppError.swift in Sources */, A52FFF572CA90484000C6A5B /* QuickTerminalScreen.swift in Sources */, diff --git a/macos/Sources/Features/Terminal/TerminalController.swift b/macos/Sources/Features/Terminal/TerminalController.swift index 0b1ff3b72..b5bdc43df 100644 --- a/macos/Sources/Features/Terminal/TerminalController.swift +++ b/macos/Sources/Features/Terminal/TerminalController.swift @@ -40,6 +40,11 @@ class TerminalController: BaseTerminalController { selector: #selector(onToggleFullscreen), name: Ghostty.Notification.ghosttyToggleFullscreen, object: nil) + center.addObserver( + self, + selector: #selector(onMoveTab), + name: .ghosttyMoveTab, + object: nil) center.addObserver( self, selector: #selector(onGotoTab), @@ -474,6 +479,44 @@ class TerminalController: BaseTerminalController { //MARK: - Notifications + @objc private func onMoveTab(notification: SwiftUI.Notification) { + guard let target = notification.object as? Ghostty.SurfaceView else { return } + guard target == self.focusedSurface else { return } + guard let window = self.window else { return } + + // Get the move action + guard let action = notification.userInfo?[Notification.Name.GhosttyMoveTabKey] as? Ghostty.Action.MoveTab else { return } + guard action.amount != 0 else { return } + + // Determine our current selected index + guard let windowController = window.windowController else { return } + guard let tabGroup = windowController.window?.tabGroup else { return } + guard let selectedWindow = tabGroup.selectedWindow else { return } + let tabbedWindows = tabGroup.windows + guard tabbedWindows.count > 0 else { return } + guard let selectedIndex = tabbedWindows.firstIndex(where: { $0 == selectedWindow }) else { return } + + // Determine the final index we want to insert our tab + let finalIndex: Int + if action.amount < 0 { + finalIndex = selectedIndex - min(selectedIndex, -action.amount) + } else { + let remaining: Int = tabbedWindows.count - 1 - selectedIndex + finalIndex = selectedIndex + min(remaining, action.amount) + } + + // If our index is the same we do nothing + guard finalIndex != selectedIndex else { return } + + // Get our parent + let parent = tabbedWindows[finalIndex] + + // Move our current selected window to the proper index + tabGroup.removeWindow(selectedWindow) + parent.addTabbedWindow(selectedWindow, ordered: action.amount < 0 ? .below : .above) + selectedWindow.makeKeyAndOrderFront(nil) + } + @objc private func onGotoTab(notification: SwiftUI.Notification) { guard let target = notification.object as? Ghostty.SurfaceView else { return } guard target == self.focusedSurface else { return } diff --git a/macos/Sources/Ghostty/Ghostty.Action.swift b/macos/Sources/Ghostty/Ghostty.Action.swift new file mode 100644 index 000000000..d9e58b28c --- /dev/null +++ b/macos/Sources/Ghostty/Ghostty.Action.swift @@ -0,0 +1,15 @@ +import GhosttyKit + +extension Ghostty { + struct Action {} +} + +extension Ghostty.Action { + struct MoveTab { + let amount: Int + + init(c: ghostty_action_move_tab_s) { + self.amount = c.amount + } + } +} diff --git a/macos/Sources/Ghostty/Ghostty.App.swift b/macos/Sources/Ghostty/Ghostty.App.swift index a2ed8903a..e5320a24a 100644 --- a/macos/Sources/Ghostty/Ghostty.App.swift +++ b/macos/Sources/Ghostty/Ghostty.App.swift @@ -458,6 +458,9 @@ extension Ghostty { case GHOSTTY_ACTION_TOGGLE_FULLSCREEN: toggleFullscreen(app, target: target, mode: action.action.toggle_fullscreen) + case GHOSTTY_ACTION_MOVE_TAB: + moveTab(app, target: target, move: action.action.move_tab) + case GHOSTTY_ACTION_GOTO_TAB: gotoTab(app, target: target, tab: action.action.goto_tab) @@ -666,6 +669,31 @@ extension Ghostty { appDelegate.toggleVisibility(self) } + private static func moveTab( + _ app: ghostty_app_t, + target: ghostty_target_s, + move: ghostty_action_move_tab_s) { + switch (target.tag) { + case GHOSTTY_TARGET_APP: + Ghostty.logger.warning("move tab does nothing with an app target") + return + + case GHOSTTY_TARGET_SURFACE: + guard let surface = target.target.surface else { return } + guard let surfaceView = self.surfaceView(from: surface) else { return } + NotificationCenter.default.post( + name: .ghosttyMoveTab, + object: surfaceView, + userInfo: [ + SwiftUI.Notification.Name.GhosttyMoveTabKey: Action.MoveTab(c: move), + ] + ) + + default: + assertionFailure() + } + } + private static func gotoTab( _ app: ghostty_app_t, target: ghostty_target_s, diff --git a/macos/Sources/Ghostty/Package.swift b/macos/Sources/Ghostty/Package.swift index f6fab532e..1be99d4a5 100644 --- a/macos/Sources/Ghostty/Package.swift +++ b/macos/Sources/Ghostty/Package.swift @@ -196,8 +196,16 @@ extension Ghostty { } } -// MARK: Surface Notifications +// MARK: Surface Notification +extension Notification.Name { + /// Goto tab. Has tab index in the userinfo. + static let ghosttyMoveTab = Notification.Name("com.mitchellh.ghostty.moveTab") + static let GhosttyMoveTabKey = ghosttyMoveTab.rawValue +} + +// NOTE: I am moving all of these to Notification.Name extensions over time. This +// namespace was the old namespace. extension Ghostty.Notification { /// Used to pass a configuration along when creating a new tab/window/split. static let NewSurfaceConfigKey = "com.mitchellh.ghostty.newSurfaceConfig" diff --git a/src/Surface.zig b/src/Surface.zig index 0d39e6dcd..b984ea997 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -3916,7 +3916,7 @@ pub fn performBindingAction(self: *Surface, action: input.Binding.Action) !bool .move_tab => |position| try self.rt_app.performAction( .{ .surface = self }, .move_tab, - position, + .{ .amount = position }, ), .new_split => |direction| try self.rt_app.performAction( diff --git a/src/apprt/action.zig b/src/apprt/action.zig index 20d480e63..2c37ca270 100644 --- a/src/apprt/action.zig +++ b/src/apprt/action.zig @@ -101,9 +101,11 @@ pub const Action = union(Key) { toggle_visibility, /// Moves a tab by a relative offset. - /// Adjusts the tab position based on `offset` (e.g., -1 for left, +1 for right). - /// If the new position is out of bounds, it wraps around cyclically within the tab range. - move_tab: isize, + /// + /// Adjusts the tab position based on `offset` (e.g., -1 for left, +1 + /// for right). If the new position is out of bounds, it wraps around + /// cyclically within the tab range. + move_tab: MoveTab, /// Jump to a specific tab. Must handle the scenario that the tab /// value is invalid. @@ -314,6 +316,10 @@ pub const ResizeSplit = extern struct { }; }; +pub const MoveTab = extern struct { + amount: isize, +}; + /// The tab to jump to. This is non-exhaustive so that integer values represent /// the index (zero-based) of the tab to jump to. Negative values are special /// values. From e7bd60b28ef83e01a15f2561316cc0a463dea1fa Mon Sep 17 00:00:00 2001 From: axdank Date: Fri, 25 Oct 2024 16:53:13 -0300 Subject: [PATCH 09/18] fix type mismatch in `moveTab` function parameter --- src/apprt/gtk/App.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 6b302493f..af664d720 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -528,7 +528,7 @@ fn gotoTab(_: *App, target: apprt.Target, tab: apprt.action.GotoTab) void { } } -fn moveTab(_: *App, target: apprt.Target, position: isize) void { +fn moveTab(_: *App, target: apprt.Target, move_tab: apprt.action.MoveTab) void { switch (target) { .app => {}, .surface => |v| { @@ -540,7 +540,7 @@ fn moveTab(_: *App, target: apprt.Target, position: isize) void { return; }; - window.moveTab(v.rt_surface, @intCast(position)); + window.moveTab(v.rt_surface, @intCast(move_tab.amount)); }, } } From f4ba95b2c53487067391117f43fc6c289099e6ed Mon Sep 17 00:00:00 2001 From: Paul Miller Date: Thu, 24 Oct 2024 11:47:20 -0500 Subject: [PATCH 10/18] add quick-terminal-animate-duration option --- .../Features/QuickTerminal/QuickTerminalController.swift | 4 ++-- macos/Sources/Ghostty/Ghostty.Config.swift | 8 ++++++++ src/config/Config.zig | 4 ++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift b/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift index 959f17d4a..8e9039253 100644 --- a/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift +++ b/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift @@ -197,7 +197,7 @@ class QuickTerminalController: BaseTerminalController { // Run the animation that moves our window into the proper place and makes // it visible. NSAnimationContext.runAnimationGroup({ context in - context.duration = 0.2 + context.duration = ghostty.config.quickTerminalAnimateDuration context.timingFunction = .init(name: .easeIn) position.setFinal(in: window.animator(), on: screen) }, completionHandler: { @@ -287,7 +287,7 @@ class QuickTerminalController: BaseTerminalController { } NSAnimationContext.runAnimationGroup({ context in - context.duration = 0.2 + context.duration = ghostty.config.quickTerminalAnimateDuration context.timingFunction = .init(name: .easeIn) position.setInitial(in: window.animator(), on: screen) }, completionHandler: { diff --git a/macos/Sources/Ghostty/Ghostty.Config.swift b/macos/Sources/Ghostty/Ghostty.Config.swift index 29639c39e..6cc543b84 100644 --- a/macos/Sources/Ghostty/Ghostty.Config.swift +++ b/macos/Sources/Ghostty/Ghostty.Config.swift @@ -343,6 +343,14 @@ extension Ghostty { let str = String(cString: ptr) return QuickTerminalScreen(fromGhosttyConfig: str) ?? .main } + + var quickTerminalAnimateDuration: Double { + guard let config = self.config else { return 0.2 } + var v: Double = 0.2 + let key = "quick-terminal-animate-duration" + _ = ghostty_config_get(config, &v, key, UInt(key.count)) + return v + } #endif var resizeOverlay: ResizeOverlay { diff --git a/src/config/Config.zig b/src/config/Config.zig index 9097051ad..dfba718b7 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1270,6 +1270,10 @@ keybind: Keybinds = .{}, /// by the operating system. @"quick-terminal-screen": QuickTerminalScreen = .main, +/// Duration (in seconds) of the quick terminal enter and exit animation. +/// Set it to 0 to disable animation. +@"quick-terminal-animate-duration": f64 = 0.2, + /// Whether to enable shell integration auto-injection or not. Shell integration /// greatly enhances the terminal experience by enabling a number of features: /// From 3c8fc86d6fffec831010e0099603259d5b45b7ae Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 25 Oct 2024 19:23:13 -0700 Subject: [PATCH 11/18] small rename --- .../Features/QuickTerminal/QuickTerminalController.swift | 4 ++-- macos/Sources/Ghostty/Ghostty.Config.swift | 4 ++-- src/config/Config.zig | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift b/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift index 8e9039253..c382a62a0 100644 --- a/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift +++ b/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift @@ -197,7 +197,7 @@ class QuickTerminalController: BaseTerminalController { // Run the animation that moves our window into the proper place and makes // it visible. NSAnimationContext.runAnimationGroup({ context in - context.duration = ghostty.config.quickTerminalAnimateDuration + context.duration = ghostty.config.quickTerminalAnimationDuration context.timingFunction = .init(name: .easeIn) position.setFinal(in: window.animator(), on: screen) }, completionHandler: { @@ -287,7 +287,7 @@ class QuickTerminalController: BaseTerminalController { } NSAnimationContext.runAnimationGroup({ context in - context.duration = ghostty.config.quickTerminalAnimateDuration + context.duration = ghostty.config.quickTerminalAnimationDuration context.timingFunction = .init(name: .easeIn) position.setInitial(in: window.animator(), on: screen) }, completionHandler: { diff --git a/macos/Sources/Ghostty/Ghostty.Config.swift b/macos/Sources/Ghostty/Ghostty.Config.swift index 6cc543b84..3e86d8b28 100644 --- a/macos/Sources/Ghostty/Ghostty.Config.swift +++ b/macos/Sources/Ghostty/Ghostty.Config.swift @@ -344,10 +344,10 @@ extension Ghostty { return QuickTerminalScreen(fromGhosttyConfig: str) ?? .main } - var quickTerminalAnimateDuration: Double { + var quickTerminalAnimationDuration: Double { guard let config = self.config else { return 0.2 } var v: Double = 0.2 - let key = "quick-terminal-animate-duration" + let key = "quick-terminal-animation-duration" _ = ghostty_config_get(config, &v, key, UInt(key.count)) return v } diff --git a/src/config/Config.zig b/src/config/Config.zig index dfba718b7..fec758648 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1271,8 +1271,9 @@ keybind: Keybinds = .{}, @"quick-terminal-screen": QuickTerminalScreen = .main, /// Duration (in seconds) of the quick terminal enter and exit animation. -/// Set it to 0 to disable animation. -@"quick-terminal-animate-duration": f64 = 0.2, +/// Set it to 0 to disable animation completely. This can be changed at +/// runtime. +@"quick-terminal-animation-duration": f64 = 0.2, /// Whether to enable shell integration auto-injection or not. Shell integration /// greatly enhances the terminal experience by enabling a number of features: From 1aa932f810a11617e9c9751c72b87aff8e55e281 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 25 Oct 2024 21:25:15 -0700 Subject: [PATCH 12/18] font/coretext: use CTFontCreateForString for final codepoint fallback Fixes #2499 We rely on CoreText's font discovery to find the best font for a fallback by using the character set attribute. It appears that for some codepoints, the character set attribute is not enough to find a font that supports the codepoint. In this case, we use CTFontCreateForString to find the font that CoreText would use. The one subtlety here is we need to ignore the last resort font, which just has replacement glyphs for all codepoints. We already had a function to do this for CJK characters (#1637) thankfully so we can just reuse that! This also fixes a bug where CTFontCreateForString range param expects the range length to be utf16 code units, not utf32. --- pkg/macos/text/font.zig | 4 +++ src/font/discovery.zig | 56 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/pkg/macos/text/font.zig b/pkg/macos/text/font.zig index 85f7de47e..67a303018 100644 --- a/pkg/macos/text/font.zig +++ b/pkg/macos/text/font.zig @@ -160,6 +160,10 @@ pub const Font = opaque { return @ptrFromInt(@intFromPtr(c.CTFontCopyDisplayName(@ptrCast(self)))); } + pub fn copyPostScriptName(self: *Font) *foundation.String { + return @ptrFromInt(@intFromPtr(c.CTFontCopyPostScriptName(@ptrCast(self)))); + } + pub fn getSymbolicTraits(self: *Font) text.FontSymbolicTraits { return @bitCast(c.CTFontGetSymbolicTraits(@ptrCast(self))); } diff --git a/src/font/discovery.zig b/src/font/discovery.zig index 6f43fcb7d..3aa16eebf 100644 --- a/src/font/discovery.zig +++ b/src/font/discovery.zig @@ -424,7 +424,30 @@ pub const CoreText = struct { }; } - return try self.discover(alloc, desc); + const it = try self.discover(alloc, desc); + + // If our normal discovery doesn't find anything and we have a specific + // codepoint, then fallback to using CTFontCreateForString to find a + // matching font CoreText wants to use. See: + // https://github.com/ghostty-org/ghostty/issues/2499 + if (it.list.len == 0 and desc.codepoint > 0) codepoint: { + const ct_desc = try self.discoverCodepoint( + collection, + desc, + ) orelse break :codepoint; + + const list = try alloc.alloc(*macos.text.FontDescriptor, 1); + errdefer alloc.free(list); + list[0] = ct_desc; + + return DiscoverIterator{ + .alloc = alloc, + .list = list, + .i = 0, + }; + } + + return it; } /// Discover a font for a specific codepoint using the CoreText @@ -491,16 +514,45 @@ pub const CoreText = struct { ); defer str.release(); + // Get our range length for CTFontCreateForString. It looks like + // the range uses UTF-16 codepoints and not UTF-32 codepoints. + const range_len: usize = range_len: { + var unichars: [2]u16 = undefined; + const pair = macos.foundation.stringGetSurrogatePairForLongCharacter( + desc.codepoint, + &unichars, + ); + break :range_len if (pair) 2 else 1; + }; + // Get our font const font = original.font.createForString( str, - macos.foundation.Range.init(0, 1), + macos.foundation.Range.init(0, range_len), ) orelse return null; defer font.release(); + // Do not allow the last resort font to go through. This is the + // last font used by CoreText if it can't find anything else and + // only contains replacement characters. + last_resort: { + const name_str = font.copyPostScriptName(); + defer name_str.release(); + + // If the name doesn't fit in our buffer, then it can't + // be the last resort font so we break out. + var name_buf: [64]u8 = undefined; + const name: []const u8 = name_str.cstring(&name_buf, .utf8) orelse + break :last_resort; + + // If the name is "LastResort" then we don't want to use it. + if (std.mem.eql(u8, "LastResort", name)) return null; + } + // Get the descriptor return font.copyDescriptor(); } + fn copyMatchingDescriptors( alloc: Allocator, list: *macos.foundation.Array, From c625d4381891637d22f38182c7e1dd1a6ea6959c Mon Sep 17 00:00:00 2001 From: Rithul Kamesh Date: Sun, 27 Oct 2024 19:47:11 +0530 Subject: [PATCH 13/18] build: require exact major/minor Zig version --- build.zig | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/build.zig b/build.zig index f581bb96c..d4b0df667 100644 --- a/build.zig +++ b/build.zig @@ -21,17 +21,20 @@ const XCFrameworkStep = @import("src/build/XCFrameworkStep.zig"); const Version = @import("src/build/Version.zig"); const Command = @import("src/Command.zig"); -// Do a comptime Zig version requirement. This is the minimum required -// Zig version. We don't check a maximum so that devs can try newer -// versions but this is the only version we guarantee to work. comptime { + // This is the required Zig version for building this project. We allow + // any patch version but the major and minor must match exactly. const required_zig = "0.13.0"; - const current_zig = builtin.zig_version; - const min_zig = std.SemanticVersion.parse(required_zig) catch unreachable; - if (current_zig.order(min_zig) == .lt) { + + // Fail compilation if the current Zig version doesn't meet requirements. + const current_vsn = builtin.zig_version; + const required_vsn = std.SemanticVersion.parse(required_zig) catch unreachable; + if (current_vsn.major != required_vsn.major or + current_vsn.minor != required_vsn.minor) + { @compileError(std.fmt.comptimePrint( - "Your Zig version v{} does not meet the minimum build requirement of v{}", - .{ current_zig, min_zig }, + "Your Zig version v{} does not meet the required build version of v{}", + .{ current_vsn, required_vsn }, )); } } From 9190dd3ad716e113017b07405132cbe322c46aa9 Mon Sep 17 00:00:00 2001 From: Anmol Wadhwani Date: Sun, 27 Oct 2024 20:51:21 +0530 Subject: [PATCH 14/18] Add an out of bounds check for mouse-selected themes in +list-themes --- src/cli/list_themes.zig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cli/list_themes.zig b/src/cli/list_themes.zig index 9782951db..aa77c92c1 100644 --- a/src/cli/list_themes.zig +++ b/src/cli/list_themes.zig @@ -515,7 +515,9 @@ const Preview = struct { } if (theme_list.hasMouse(mouse)) |_| { if (mouse.button == .left and mouse.type == .release) { - self.current = self.window + mouse.row; + if (self.window + mouse.row < self.filtered.items.len) { + self.current = self.window + mouse.row; + } } highlight = mouse.row; } From 70d850f8e438f314149a279b022d53d1a4474679 Mon Sep 17 00:00:00 2001 From: Anmol Wadhwani Date: Sun, 27 Oct 2024 21:40:09 +0530 Subject: [PATCH 15/18] make selection a const --- src/cli/list_themes.zig | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cli/list_themes.zig b/src/cli/list_themes.zig index aa77c92c1..92cb57be2 100644 --- a/src/cli/list_themes.zig +++ b/src/cli/list_themes.zig @@ -515,8 +515,9 @@ const Preview = struct { } if (theme_list.hasMouse(mouse)) |_| { if (mouse.button == .left and mouse.type == .release) { - if (self.window + mouse.row < self.filtered.items.len) { - self.current = self.window + mouse.row; + const selection = self.window + mouse.row; + if (selection < self.filtered.items.len) { + self.current = selection; } } highlight = mouse.row; From 348287c620819f96ebef8360458ff9fc9df5e968 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Sun, 27 Oct 2024 15:58:51 -0500 Subject: [PATCH 16/18] core: add a .txt extenstion to scrollback file This should make it easier for open or xdg-open to find an appropriate application to open the scrollback file with. --- src/Surface.zig | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index b984ea997..82d1240eb 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -4118,9 +4118,13 @@ fn writeScreenFile( var tmp_dir = try internal_os.TempDir.init(); errdefer tmp_dir.deinit(); + var filename_buf: [std.fs.max_path_bytes]u8 = undefined; + const filename = try std.fmt.bufPrint(&filename_buf, "{s}.txt", .{@tagName(loc)}); + // Open our scrollback file - var file = try tmp_dir.dir.createFile(@tagName(loc), .{}); + var file = try tmp_dir.dir.createFile(filename, .{}); defer file.close(); + // Screen.dumpString writes byte-by-byte, so buffer it var buf_writer = std.io.bufferedWriter(file.writer()); @@ -4179,7 +4183,7 @@ fn writeScreenFile( // Get the final path var path_buf: [std.fs.max_path_bytes]u8 = undefined; - const path = try tmp_dir.dir.realpath(@tagName(loc), &path_buf); + const path = try tmp_dir.dir.realpath(filename, &path_buf); switch (write_action) { .open => try internal_os.open(self.alloc, path), From 6109edd00bdf3fc07f89c96efb8a05c0634cdae3 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 28 Oct 2024 15:19:17 -0700 Subject: [PATCH 17/18] terminal: refactor hyperlink memory management Fixes #2500 Based on #2508 This separates out the concept of a "hyperlink" from a "hyperlink page entry." The difference is that the former has real Zig slices into things like strings and the latter has offsets into terminal page memory. From this separation, the Page structure now has an `insertHyperlink` function that takes a hyperlink and converts it to a page entry. This does a couple things: (1) it moves page memory management out of Screen and into Page which is historically more appropriate and (2) it let's us more easily test hyperlinks from the Page unit tests. Finally, this PR makes some error sets explicit. --- src/terminal/Screen.zig | 152 +++++++++++++------------------------ src/terminal/hyperlink.zig | 75 +++++++++++++++--- src/terminal/page.zig | 141 +++++++++++++++++++++++++++++++++- 3 files changed, 257 insertions(+), 111 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index f1db3dd52..e9d2fccd8 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -125,7 +125,7 @@ pub const Cursor = struct { /// the cursor page pin changes. We can't get it from the old screen /// state because the page may be cleared. This is heap allocated /// because its most likely null. - hyperlink: ?*Hyperlink = null, + hyperlink: ?*hyperlink.Hyperlink = null, /// The pointers into the page list where the cursor is currently /// located. This makes it faster to move the cursor. @@ -134,7 +134,10 @@ pub const Cursor = struct { page_cell: *pagepkg.Cell, pub fn deinit(self: *Cursor, alloc: Allocator) void { - if (self.hyperlink) |link| link.destroy(alloc); + if (self.hyperlink) |link| { + link.deinit(alloc); + alloc.destroy(link); + } } }; @@ -182,31 +185,6 @@ pub const CharsetState = struct { const CharsetArray = std.EnumArray(charsets.Slots, charsets.Charset); }; -pub const Hyperlink = struct { - id: ?[]const u8, - uri: []const u8, - - pub fn create( - alloc: Allocator, - uri: []const u8, - id: ?[]const u8, - ) !*Hyperlink { - const self = try alloc.create(Hyperlink); - errdefer alloc.destroy(self); - self.id = if (id) |v| try alloc.dupe(u8, v) else null; - errdefer if (self.id) |v| alloc.free(v); - self.uri = try alloc.dupe(u8, uri); - errdefer alloc.free(self.uri); - return self; - } - - pub fn destroy(self: *Hyperlink, alloc: Allocator) void { - if (self.id) |id| alloc.free(id); - alloc.free(self.uri); - alloc.destroy(self); - } -}; - /// Initialize a new screen. /// /// max_scrollback is the amount of scrollback to keep in bytes. This @@ -471,10 +449,11 @@ pub fn adjustCapacity( self.cursor.hyperlink = null; // Re-add - self.startHyperlinkOnce(link.uri, link.id) catch unreachable; + self.startHyperlinkOnce(link.*) catch unreachable; // Remove our old link - link.destroy(self.alloc); + link.deinit(self.alloc); + self.alloc.destroy(link); } // Reload the cursor information because the pin changed. @@ -1023,7 +1002,10 @@ fn cursorChangePin(self: *Screen, new: Pin) void { self.cursor.hyperlink = null; // Re-add - self.startHyperlink(link.uri, link.id) catch |err| { + self.startHyperlink(link.uri, switch (link.id) { + .explicit => |v| v, + .implicit => null, + }) catch |err| { // This shouldn't happen because startHyperlink should handle // resizing. This only happens if we're truly out of RAM. Degrade // to forgetting the hyperlink. @@ -1031,7 +1013,8 @@ fn cursorChangePin(self: *Screen, new: Pin) void { }; // Remove our old link - link.destroy(self.alloc); + link.deinit(self.alloc); + self.alloc.destroy(link); } } @@ -1550,7 +1533,10 @@ fn resizeInternal( // Fix up our hyperlink if we had one. if (hyperlink_) |link| { - self.startHyperlink(link.uri, link.id) catch |err| { + self.startHyperlink(link.uri, switch (link.id) { + .explicit => |v| v, + .implicit => null, + }) catch |err| { // This shouldn't happen because startHyperlink should handle // resizing. This only happens if we're truly out of RAM. Degrade // to forgetting the hyperlink. @@ -1558,7 +1544,8 @@ fn resizeInternal( }; // Remove our old link - link.destroy(self.alloc); + link.deinit(self.alloc); + self.alloc.destroy(link); } } @@ -1805,6 +1792,8 @@ pub fn appendGrapheme(self: *Screen, cell: *Cell, cp: u21) !void { }; } +pub const StartHyperlinkError = Allocator.Error || PageList.AdjustCapacityError; + /// Start the hyperlink state. Future cells will be marked as hyperlinks with /// this state. Note that various terminal operations may clear the hyperlink /// state, such as switching screens (alt screen). @@ -1812,14 +1801,29 @@ pub fn startHyperlink( self: *Screen, uri: []const u8, id_: ?[]const u8, -) !void { +) StartHyperlinkError!void { + // Create our pending entry. + const link: hyperlink.Hyperlink = .{ + .uri = uri, + .id = if (id_) |id| .{ + .explicit = id, + } else implicit: { + defer self.cursor.hyperlink_implicit_id += 1; + break :implicit .{ .implicit = self.cursor.hyperlink_implicit_id }; + }, + }; + errdefer switch (link.id) { + .explicit => {}, + .implicit => self.cursor.hyperlink_implicit_id -= 1, + }; + // Loop until we have enough page memory to add the hyperlink while (true) { - if (self.startHyperlinkOnce(uri, id_)) { + if (self.startHyperlinkOnce(link)) { return; } else |err| switch (err) { // An actual self.alloc OOM is a fatal error. - error.RealOutOfMemory => return error.OutOfMemory, + error.OutOfMemory => return error.OutOfMemory, // strings table is out of memory, adjust it up error.StringsOutOfMemory => _ = try self.adjustCapacity( @@ -1849,74 +1853,21 @@ pub fn startHyperlink( /// all the previous state and try again. fn startHyperlinkOnce( self: *Screen, - uri: []const u8, - id_: ?[]const u8, -) !void { + source: hyperlink.Hyperlink, +) (Allocator.Error || Page.InsertHyperlinkError)!void { // End any prior hyperlink self.endHyperlink(); - // Create our hyperlink state. - const link = Hyperlink.create(self.alloc, uri, id_) catch |err| switch (err) { - error.OutOfMemory => return error.RealOutOfMemory, - }; - errdefer link.destroy(self.alloc); + // Allocate our new Hyperlink entry in non-page memory. This + // lets us quickly get access to URI, ID. + const link = try self.alloc.create(hyperlink.Hyperlink); + errdefer self.alloc.destroy(link); + link.* = try source.dupe(self.alloc); + errdefer link.deinit(self.alloc); - // Copy our URI into the page memory. + // Insert the hyperlink into page memory var page = &self.cursor.page_pin.page.data; - const string_alloc = &page.string_alloc; - const page_uri: Offset(u8).Slice = uri: { - const buf = string_alloc.alloc(u8, page.memory, uri.len) catch |err| switch (err) { - error.OutOfMemory => return error.StringsOutOfMemory, - }; - errdefer string_alloc.free(page.memory, buf); - @memcpy(buf, uri); - - break :uri .{ - .offset = size.getOffset(u8, page.memory, &buf[0]), - .len = uri.len, - }; - }; - errdefer string_alloc.free( - page.memory, - page_uri.offset.ptr(page.memory)[0..page_uri.len], - ); - - // Copy our ID into page memory or create an implicit ID via the counter - const page_id: hyperlink.Hyperlink.Id = if (id_) |id| explicit: { - const buf = string_alloc.alloc(u8, page.memory, id.len) catch |err| switch (err) { - error.OutOfMemory => return error.StringsOutOfMemory, - }; - errdefer string_alloc.free(page.memory, buf); - @memcpy(buf, id); - - break :explicit .{ - .explicit = .{ - .offset = size.getOffset(u8, page.memory, &buf[0]), - .len = id.len, - }, - }; - } else implicit: { - defer self.cursor.hyperlink_implicit_id += 1; - break :implicit .{ .implicit = self.cursor.hyperlink_implicit_id }; - }; - errdefer switch (page_id) { - .implicit => self.cursor.hyperlink_implicit_id -= 1, - .explicit => |slice| string_alloc.free( - page.memory, - slice.offset.ptr(page.memory)[0..slice.len], - ), - }; - - // Put our hyperlink into the hyperlink set to get an ID - const id = page.hyperlink_set.addContext( - page.memory, - .{ .id = page_id, .uri = page_uri }, - .{ .page = page }, - ) catch |err| switch (err) { - error.OutOfMemory => return error.SetOutOfMemory, - error.NeedsRehash => return error.SetNeedsRehash, - }; - errdefer page.hyperlink_set.release(page.memory, id); + const id: hyperlink.Id = try page.insertHyperlink(link.*); // Save it all self.cursor.hyperlink = link; @@ -1944,7 +1895,8 @@ pub fn endHyperlink(self: *Screen) void { // will be called. var page = &self.cursor.page_pin.page.data; page.hyperlink_set.release(page.memory, self.cursor.hyperlink_id); - self.cursor.hyperlink.?.destroy(self.alloc); + self.cursor.hyperlink.?.deinit(self.alloc); + self.alloc.destroy(self.cursor.hyperlink.?); self.cursor.hyperlink_id = 0; self.cursor.hyperlink = null; } diff --git a/src/terminal/hyperlink.zig b/src/terminal/hyperlink.zig index 53d776ad5..1ab3c5ea7 100644 --- a/src/terminal/hyperlink.zig +++ b/src/terminal/hyperlink.zig @@ -1,4 +1,5 @@ const std = @import("std"); +const Allocator = std.mem.Allocator; const assert = std.debug.assert; const hash_map = @import("hash_map.zig"); const AutoOffsetHashMap = hash_map.AutoOffsetHashMap; @@ -21,9 +22,63 @@ pub const Id = size.CellCountInt; // the hyperlink ID in the cell itself. pub const Map = AutoOffsetHashMap(Offset(Cell), Id); -/// The main entry for hyperlinks. +/// A fully decoded hyperlink that may or may not have its +/// memory within a page. The memory location of this is dependent +/// on the context so users should check with the source of the +/// hyperlink. pub const Hyperlink = struct { id: Hyperlink.Id, + uri: []const u8, + + /// See PageEntry.Id + pub const Id = union(enum) { + explicit: []const u8, + implicit: size.OffsetInt, + }; + + /// Deinit and deallocate all the pointers using the given + /// allocator. + /// + /// WARNING: This should only be called if the hyperlink was + /// heap-allocated. This DOES NOT need to be unconditionally + /// called. + pub fn deinit(self: *const Hyperlink, alloc: Allocator) void { + alloc.free(self.uri); + switch (self.id) { + .implicit => {}, + .explicit => |v| alloc.free(v), + } + } + + /// Duplicate a hyperlink by allocating all values with the + /// given allocator. The returned hyperlink should have deinit + /// called. + pub fn dupe( + self: *const Hyperlink, + alloc: Allocator, + ) Allocator.Error!Hyperlink { + const uri = try alloc.dupe(u8, self.uri); + errdefer alloc.free(uri); + + const id: Hyperlink.Id = switch (self.id) { + .implicit => self.id, + .explicit => |v| .{ .explicit = try alloc.dupe(u8, v) }, + }; + errdefer switch (id) { + .implicit => {}, + .explicit => |v| alloc.free(v), + }; + + return .{ .id = id, .uri = uri }; + } +}; + +/// A hyperlink that has been committed to page memory. This +/// is a "page entry" because while it represents a hyperlink, +/// some decoding (pointer chasing) is still necessary to get the +/// fully realized ID, URI, etc. +pub const PageEntry = struct { + id: PageEntry.Id, uri: Offset(u8).Slice, pub const Id = union(enum) { @@ -37,10 +92,10 @@ pub const Hyperlink = struct { /// Duplicate this hyperlink from one page to another. pub fn dupe( - self: *const Hyperlink, + self: *const PageEntry, self_page: *const Page, dst_page: *Page, - ) error{OutOfMemory}!Hyperlink { + ) error{OutOfMemory}!PageEntry { var copy = self.*; // If the pages are the same then we can return a shallow copy. @@ -85,7 +140,7 @@ pub const Hyperlink = struct { return copy; } - pub fn hash(self: *const Hyperlink, base: anytype) u64 { + pub fn hash(self: *const PageEntry, base: anytype) u64 { var hasher = Wyhash.init(0); autoHash(&hasher, std.meta.activeTag(self.id)); switch (self.id) { @@ -105,9 +160,9 @@ pub const Hyperlink = struct { } pub fn eql( - self: *const Hyperlink, + self: *const PageEntry, self_base: anytype, - other: *const Hyperlink, + other: *const PageEntry, other_base: anytype, ) bool { if (std.meta.activeTag(self.id) != std.meta.activeTag(other.id)) return false; @@ -135,21 +190,21 @@ pub const Hyperlink = struct { /// The set of hyperlinks. This is ref-counted so that a set of cells /// can share the same hyperlink without duplicating the data. pub const Set = RefCountedSet( - Hyperlink, + PageEntry, Id, size.CellCountInt, struct { page: ?*Page = null, - pub fn hash(self: *const @This(), link: Hyperlink) u64 { + pub fn hash(self: *const @This(), link: PageEntry) u64 { return link.hash(self.page.?.memory); } - pub fn eql(self: *const @This(), a: Hyperlink, b: Hyperlink) bool { + pub fn eql(self: *const @This(), a: PageEntry, b: PageEntry) bool { return a.eql(self.page.?.memory, &b, self.page.?.memory); } - pub fn deleted(self: *const @This(), link: Hyperlink) void { + pub fn deleted(self: *const @This(), link: PageEntry) void { const page = self.page.?; const alloc = &page.string_alloc; switch (link.id) { diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 7231550e7..8c470d726 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -808,7 +808,7 @@ pub const Page = struct { // If our page can't support an additional cell with // a hyperlink then we have to return an error. - if (self.hyperlinkCount() >= self.hyperlinkCapacity() - 1) { + if (self.hyperlinkCount() >= self.hyperlinkCapacity()) { // The hyperlink map capacity needs to be increased. return error.HyperlinkMapOutOfMemory; } @@ -1142,6 +1142,101 @@ pub const Page = struct { row.hyperlink = false; } + pub const InsertHyperlinkError = error{ + /// string_alloc errors + StringsOutOfMemory, + + /// hyperlink_set errors + SetOutOfMemory, + SetNeedsRehash, + }; + + /// Convert a hyperlink into a page entry, returning the ID. + /// + /// This does not de-dupe any strings, so if the URI, explicit ID, + /// etc. is already in the strings table this will duplicate it. + /// + /// To release the memory associated with the given hyperlink, + /// release the ID from the `hyperlink_set`. If the refcount reaches + /// zero and the slot is needed then the context will reap the + /// memory. + pub fn insertHyperlink( + self: *Page, + link: hyperlink.Hyperlink, + ) InsertHyperlinkError!hyperlink.Id { + // Insert our URI into the page strings table. + const page_uri: Offset(u8).Slice = uri: { + const buf = self.string_alloc.alloc( + u8, + self.memory, + link.uri.len, + ) catch |err| switch (err) { + error.OutOfMemory => return error.StringsOutOfMemory, + }; + errdefer self.string_alloc.free(self.memory, buf); + @memcpy(buf, link.uri); + + break :uri .{ + .offset = size.getOffset(u8, self.memory, &buf[0]), + .len = link.uri.len, + }; + }; + errdefer self.string_alloc.free( + self.memory, + page_uri.offset.ptr(self.memory)[0..page_uri.len], + ); + + // Allocate an ID for our page memory if we have to. + const page_id: hyperlink.PageEntry.Id = switch (link.id) { + .explicit => |id| explicit: { + const buf = self.string_alloc.alloc( + u8, + self.memory, + id.len, + ) catch |err| switch (err) { + error.OutOfMemory => return error.StringsOutOfMemory, + }; + errdefer self.string_alloc.free(self.memory, buf); + @memcpy(buf, id); + + break :explicit .{ + .explicit = .{ + .offset = size.getOffset(u8, self.memory, &buf[0]), + .len = id.len, + }, + }; + }, + + .implicit => |id| .{ .implicit = id }, + }; + errdefer switch (page_id) { + .implicit => {}, + .explicit => |slice| self.string_alloc.free( + self.memory, + slice.offset.ptr(self.memory)[0..slice.len], + ), + }; + + // Build our entry + const entry: hyperlink.PageEntry = .{ + .id = page_id, + .uri = page_uri, + }; + + // Put our hyperlink into the hyperlink set to get an ID + const id = self.hyperlink_set.addContext( + self.memory, + entry, + .{ .page = self }, + ) catch |err| switch (err) { + error.OutOfMemory => return error.SetOutOfMemory, + error.NeedsRehash => return error.SetNeedsRehash, + }; + errdefer self.hyperlink_set.release(self.memory, id); + + return id; + } + /// Set the hyperlink for the given cell. If the cell already has a /// hyperlink, then this will handle memory management and refcount /// update for the prior hyperlink. @@ -2237,6 +2332,50 @@ test "Page cloneFrom partial" { } } +test "Page cloneFrom hyperlinks exact capacity" { + var page = try Page.init(.{ + .cols = 50, + .rows = 50, + }); + defer page.deinit(); + + // Ensure our page can accommodate the capacity. + const hyperlink_cap = page.hyperlinkCapacity(); + try testing.expect(hyperlink_cap <= page.size.cols * page.size.rows); + + // Create a hyperlink. + const hyperlink_id = try page.insertHyperlink(.{ + .id = .{ .implicit = 0 }, + .uri = "https://example.com", + }); + + // Fill the exact cap with cells. + fill: for (0..page.size.cols) |x| { + for (0..page.size.rows) |y| { + const rac = page.getRowAndCell(x, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 42 }, + }; + try page.setHyperlink(rac.row, rac.cell, hyperlink_id); + page.hyperlink_set.use(page.memory, hyperlink_id); + + if (page.hyperlinkCount() == hyperlink_cap) { + break :fill; + } + } + } + try testing.expectEqual(page.hyperlinkCount(), page.hyperlinkCapacity()); + + // Clone the full page + var page2 = try Page.init(page.capacity); + defer page2.deinit(); + try page2.cloneFrom(&page, 0, page.size.rows); + + // We should have the same number of hyperlinks + try testing.expectEqual(page2.hyperlinkCount(), page.hyperlinkCount()); +} + test "Page cloneFrom graphemes" { var page = try Page.init(.{ .cols = 10, From 487f08b1ddf7a7e48f4395e103b6cf1b36b423b0 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Sun, 27 Oct 2024 12:45:12 -0400 Subject: [PATCH 18/18] fix(PageList, Page): fix off-by-1 in map capacity checks We're already using `>=`, we don't need to also use `- 1` --- src/terminal/PageList.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 8d9641bfa..d86ebc765 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -896,7 +896,7 @@ const ReflowCursor = struct { // If our page can't support an additional cell with // graphemes then we create a new page for this row. - if (self.page.graphemeCount() >= self.page.graphemeCapacity() - 1) { + if (self.page.graphemeCount() >= self.page.graphemeCapacity()) { try self.moveLastRowToNewPage(list, cap); } else { // Attempt to allocate the space that would be required for @@ -924,7 +924,7 @@ const ReflowCursor = struct { // If our page can't support an additional cell with // a hyperlink ID then we create a new page for this row. - if (self.page.hyperlinkCount() >= self.page.hyperlinkCapacity() - 1) { + if (self.page.hyperlinkCount() >= self.page.hyperlinkCapacity()) { try self.moveLastRowToNewPage(list, cap); }