From adcaff7137ef1fedf5f094126032ce7de34d73e1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 29 Dec 2024 19:49:31 -0800 Subject: [PATCH] config: edit opens AppSupport over XDG on macOS, prefers non-empty paths Fixes #3953 Fixes #3284 This fixes two issues. In fixing one issue, the other became apparent so I fixed both in this one commit. The first issue is that on macOS, the `open` command should take the `-t` flag to open text files in a text editor. To do this, the `os.open` function now takes a type hint that is used to better do the right thing. Second, the order of the paths that we attempt to open when editing a config on macOS is wrong. Our priority when loading configs is well documented: https://ghostty.org/docs/config#macos-specific-path-(macos-only). But open_config does the opposite. This makes it too easy for people to have configs that are being overridden without them realizing it. This commit changes the order of the paths to match the documented order. If neither path exists, we prefer AppSupport. --- src/Surface.zig | 6 +-- src/config/edit.zig | 106 +++++++++++++++++++++++++++++++++++--------- src/os/macos.zig | 2 +- src/os/main.zig | 1 + src/os/open.zig | 64 +++++++++++++++++++------- 5 files changed, 140 insertions(+), 39 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 053dec3fd..13e986919 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -3195,7 +3195,7 @@ fn processLinks(self: *Surface, pos: apprt.CursorPos) !bool { .trim = false, }); defer self.alloc.free(str); - try internal_os.open(self.alloc, str); + try internal_os.open(self.alloc, .unknown, str); }, ._open_osc8 => { @@ -3203,7 +3203,7 @@ fn processLinks(self: *Surface, pos: apprt.CursorPos) !bool { log.warn("failed to get URI for OSC8 hyperlink", .{}); return false; }; - try internal_os.open(self.alloc, uri); + try internal_os.open(self.alloc, .unknown, uri); }, } @@ -4303,7 +4303,7 @@ fn writeScreenFile( const path = try tmp_dir.dir.realpath(filename, &path_buf); switch (write_action) { - .open => try internal_os.open(self.alloc, path), + .open => try internal_os.open(self.alloc, .text, path), .paste => self.io.queueMessage(try termio.Message.writeReq( self.alloc, path, diff --git a/src/config/edit.zig b/src/config/edit.zig index 692447594..68d9da88c 100644 --- a/src/config/edit.zig +++ b/src/config/edit.zig @@ -1,31 +1,29 @@ const std = @import("std"); const builtin = @import("builtin"); +const assert = std.debug.assert; const Allocator = std.mem.Allocator; +const ArenaAllocator = std.heap.ArenaAllocator; const internal_os = @import("../os/main.zig"); /// Open the configuration in the OS default editor according to the default /// paths the main config file could be in. +/// +/// On Linux, this will open the file at the XDG config path. This is the +/// only valid path for Linux so we don't need to check for other paths. +/// +/// On macOS, both XDG and AppSupport paths are valid. Because Ghostty +/// prioritizes AppSupport over XDG, we will open AppSupport if it exists, +/// followed by XDG if it exists, and finally AppSupport if neither exist. +/// For the existence check, we also prefer non-empty files over empty +/// files. pub fn open(alloc_gpa: Allocator) !void { - // default location - const config_path = config_path: { - const xdg_config_path = try internal_os.xdg.config(alloc_gpa, .{ .subdir = "ghostty/config" }); + // Use an arena to make memory management easier in here. + var arena = ArenaAllocator.init(alloc_gpa); + defer arena.deinit(); + const alloc = arena.allocator(); - if (comptime builtin.os.tag == .macos) macos: { - // On macOS, use the application support path if the XDG path doesn't exists. - if (std.fs.accessAbsolute(xdg_config_path, .{})) { - break :macos; - } else |err| switch (err) { - error.BadPathName, error.FileNotFound => {}, - else => break :macos, - } - - alloc_gpa.free(xdg_config_path); - break :config_path try internal_os.macos.appSupportDir(alloc_gpa, "config"); - } - - break :config_path xdg_config_path; - }; - defer alloc_gpa.free(config_path); + // Get the path we should open + const config_path = try configPath(alloc); // Create config directory recursively. if (std.fs.path.dirname(config_path)) |config_dir| { @@ -43,5 +41,73 @@ pub fn open(alloc_gpa: Allocator) !void { } }; - try internal_os.open(alloc_gpa, config_path); + try internal_os.open(alloc, .text, config_path); +} + +/// Returns the config path to use for open for the current OS. +/// +/// The allocator must be an arena allocator. No memory is freed by this +/// function and the resulting path is not all the memory that is allocated. +/// +/// NOTE: WHY IS THIS INLINE? This is inline because when this is not +/// inline then Zig 0.13 crashes [most of the time] when trying to compile +/// this file. This is a workaround for that issue. This function is only +/// called from one place that is not performance critical so it is fine +/// to be inline. +inline fn configPath(alloc_arena: Allocator) ![]const u8 { + const paths: []const []const u8 = try configPathCandidates(alloc_arena); + assert(paths.len > 0); + + // Find the first path that exists and is non-empty. If no paths are + // non-empty but at least one exists, we will return the first path that + // exists. + var exists: ?[]const u8 = null; + for (paths) |path| { + const f = std.fs.openFileAbsolute(path, .{}) catch |err| { + switch (err) { + // File doesn't exist, continue. + error.BadPathName, error.FileNotFound => continue, + + // Some other error, assume it exists and return it. + else => return err, + } + }; + defer f.close(); + + // We expect stat to succeed because we just opened the file. + const stat = try f.stat(); + + // If the file is non-empty, return it. + if (stat.size > 0) return path; + + // If the file is empty, remember it exists. + if (exists == null) exists = path; + } + + // No paths are non-empty, return the first path that exists. + if (exists) |v| return v; + + // No paths are non-empty or exist, return the first path. + return paths[0]; +} + +/// Returns a const list of possible paths the main config file could be +/// in for the current OS. +fn configPathCandidates(alloc_arena: Allocator) ![]const []const u8 { + var paths = try std.ArrayList([]const u8).initCapacity(alloc_arena, 2); + errdefer paths.deinit(); + + if (comptime builtin.os.tag == .macos) { + paths.appendAssumeCapacity(try internal_os.macos.appSupportDir( + alloc_arena, + "config", + )); + } + + paths.appendAssumeCapacity(try internal_os.xdg.config( + alloc_arena, + .{ .subdir = "ghostty/config" }, + )); + + return paths.items; } diff --git a/src/os/macos.zig b/src/os/macos.zig index 53dfd1719..b3d0a917c 100644 --- a/src/os/macos.zig +++ b/src/os/macos.zig @@ -24,7 +24,7 @@ pub const AppSupportDirError = Allocator.Error || error{AppleAPIFailed}; pub fn appSupportDir( alloc: Allocator, sub_path: []const u8, -) AppSupportDirError![]u8 { +) AppSupportDirError![]const u8 { comptime assert(builtin.target.isDarwin()); const NSFileManager = objc.getClass("NSFileManager").?; diff --git a/src/os/main.zig b/src/os/main.zig index 3b7007fcb..98e57b4fc 100644 --- a/src/os/main.zig +++ b/src/os/main.zig @@ -41,5 +41,6 @@ pub const home = homedir.home; pub const ensureLocale = locale.ensureLocale; pub const clickInterval = mouse.clickInterval; pub const open = openpkg.open; +pub const OpenType = openpkg.Type; pub const pipe = pipepkg.pipe; pub const resourcesDir = resourcesdir.resourcesDir; diff --git a/src/os/open.zig b/src/os/open.zig index 8df059487..ff7d6049a 100644 --- a/src/os/open.zig +++ b/src/os/open.zig @@ -2,25 +2,26 @@ const std = @import("std"); const builtin = @import("builtin"); const Allocator = std.mem.Allocator; +/// The type of the data at the URL to open. This is used as a hint +/// to potentially open the URL in a different way. +pub const Type = enum { + text, + unknown, +}; + /// Open a URL in the default handling application. /// /// Any output on stderr is logged as a warning in the application logs. /// Output on stdout is ignored. -pub fn open(alloc: Allocator, url: []const u8) !void { - // Some opener commands terminate after opening (macOS open) and some do not - // (xdg-open). For those which do not terminate, we do not want to wait for - // the process to exit to collect stderr. - const argv, const wait = switch (builtin.os.tag) { - .linux => .{ &.{ "xdg-open", url }, false }, - .macos => .{ &.{ "open", url }, true }, - .windows => .{ &.{ "rundll32", "url.dll,FileProtocolHandler", url }, false }, - .ios => return error.Unimplemented, - else => @compileError("unsupported OS"), - }; +pub fn open( + alloc: Allocator, + typ: Type, + url: []const u8, +) !void { + const cmd = try openCommand(alloc, typ, url); - var exe = std.process.Child.init(argv, alloc); - - if (comptime wait) { + var exe = cmd.child; + if (cmd.wait) { // Pipe stdout/stderr so we can collect output from the command exe.stdout_behavior = .Pipe; exe.stderr_behavior = .Pipe; @@ -28,7 +29,7 @@ pub fn open(alloc: Allocator, url: []const u8) !void { try exe.spawn(); - if (comptime wait) { + if (cmd.wait) { // 50 KiB is the default value used by std.process.Child.run const output_max_size = 50 * 1024; @@ -47,3 +48,36 @@ pub fn open(alloc: Allocator, url: []const u8) !void { if (stderr.items.len > 0) std.log.err("open stderr={s}", .{stderr.items}); } } + +const OpenCommand = struct { + child: std.process.Child, + wait: bool = false, +}; + +fn openCommand(alloc: Allocator, typ: Type, url: []const u8) !OpenCommand { + return switch (builtin.os.tag) { + .linux => .{ .child = std.process.Child.init( + &.{ "xdg-open", url }, + alloc, + ) }, + + .windows => .{ .child = std.process.Child.init( + &.{ "rundll32", "url.dll,FileProtocolHandler", url }, + alloc, + ) }, + + .macos => .{ + .child = std.process.Child.init( + switch (typ) { + .text => &.{ "open", "-t", url }, + .unknown => &.{ "open", url }, + }, + alloc, + ), + .wait = true, + }, + + .ios => return error.Unimplemented, + else => @compileError("unsupported OS"), + }; +}