themes: finish refactor

src/cli/list_themes.zig

@@ -2,13 +2,15 @@ const std = @import("std");
 const inputpkg = @import("../input.zig");
 const args = @import("args.zig");
 const Action = @import("action.zig").Action;
-const Arena = std.heap.ArenaAllocator;
-const Allocator = std.mem.Allocator;
 const Config = @import("../config/Config.zig");
+const themepkg = @import("../config/theme.zig");
 const internal_os = @import("../os/main.zig");
 const global_state = &@import("../global.zig").state;
 
 pub const Options = struct {
+    /// If true, print the full path to the theme.
+    path: bool = false,
+
     /// If true, show a small preview of the theme.
     preview: bool = false,
 
@@ -45,17 +47,21 @@ pub const Options = struct {
 ///
 /// Flags:
 ///
+///   * `--path`: Show the full path to the theme.
 ///   * `--preview`: Show a short preview of the theme colors.
-pub fn run(alloc: Allocator) !u8 {
+pub fn run(gpa_alloc: std.mem.Allocator) !u8 {
     var opts: Options = .{};
     defer opts.deinit();
 
     {
-        var iter = try std.process.argsWithAllocator(alloc);
+        var iter = try std.process.argsWithAllocator(gpa_alloc);
         defer iter.deinit();
-        try args.parse(Options, alloc, &opts, &iter);
+        try args.parse(Options, gpa_alloc, &opts, &iter);
     }
 
+    var arena = std.heap.ArenaAllocator.init(gpa_alloc);
+    const alloc = arena.allocator();
+
     const stderr = std.io.getStdErr().writer();
     const stdout = std.io.getStdOut().writer();
 
@@ -63,29 +69,12 @@ pub fn run(alloc: Allocator) !u8 {
         try stderr.print("Could not find the Ghostty resources directory. Please ensure " ++
             "that Ghostty is installed correctly.\n", .{});
 
-    const paths: []const struct {
-        type: Config.ThemeDirType,
-        dir: ?[]const u8,
-    } = &.{
-        .{
-            .type = .user,
-            .dir = Config.themeDir(alloc, .user),
-        },
-        .{
-            .type = .system,
-            .dir = Config.themeDir(alloc, .system),
-        },
-    };
-
     const ThemeListElement = struct {
-        type: Config.ThemeDirType,
+        location: themepkg.Location,
         path: []const u8,
         theme: []const u8,
-        fn deinit(self: *const @This(), alloc_: std.mem.Allocator) void {
-            alloc_.free(self.path);
-            alloc_.free(self.theme);
-        }
         fn lessThan(_: void, lhs: @This(), rhs: @This()) bool {
+            // TODO: use Unicode-aware comparison
             return std.ascii.orderIgnoreCase(lhs.theme, rhs.theme) == .lt;
         }
     };
@@ -93,40 +82,41 @@ pub fn run(alloc: Allocator) !u8 {
     var count: usize = 0;
 
     var themes = std.ArrayList(ThemeListElement).init(alloc);
-    defer {
-        for (themes.items) |v| v.deinit(alloc);
-        themes.deinit();
-    }
 
-    for (paths) |path| {
+    var it = themepkg.LocationIterator{ .arena = &arena };
-        if (path.dir) |p| {
-            defer alloc.free(p);
 
-            var dir = try std.fs.cwd().openDir(p, .{ .iterate = true });
+    while (try it.next()) |loc| {
-            defer dir.close();
+        var dir = std.fs.cwd().openDir(loc.dir, .{ .iterate = true }) catch |err| switch (err) {
+            error.FileNotFound => continue,
+            else => {
+                std.debug.print("err: {}\n", .{err});
+                continue;
+            },
+        };
+        defer dir.close();
+        var walker = dir.iterate();
 
-            var walker = try dir.walk(alloc);
+        while (try walker.next()) |entry| {
-            defer walker.deinit();
+            if (entry.kind != .file) continue;
-
+            count += 1;
-            while (try walker.next()) |entry| {
+            try themes.append(.{
-                if (entry.kind != .file) continue;
+                .location = loc.location,
-                count += 1;
+                .path = try std.fs.path.join(alloc, &.{ loc.dir, entry.name }),
-                try themes.append(.{
+                .theme = try alloc.dupe(u8, entry.name),
-                    .type = path.type,
+            });
-                    .path = try std.fs.path.join(alloc, &.{ p, entry.basename }),
-                    .theme = try alloc.dupe(u8, entry.basename),
-                });
-            }
         }
     }
 
     std.mem.sortUnstable(ThemeListElement, themes.items, {}, ThemeListElement.lessThan);
 
     for (themes.items) |theme| {
-        try stdout.print("{s} ({s})\n", .{ theme.theme, @tagName(theme.type) });
+        if (opts.path)
+            try stdout.print("{s} ({s}) {s}\n", .{ theme.theme, @tagName(theme.location), theme.path })
+        else
+            try stdout.print("{s} ({s})\n", .{ theme.theme, @tagName(theme.location) });
 
         if (opts.preview) {
-            var config = try Config.default(alloc);
+            var config = try Config.default(gpa_alloc);
             defer config.deinit();
             if (config.loadFile(config._arena.?.allocator(), theme.path)) |_| {
                 if (!config._errors.empty()) {

src/config/Config.zig

@@ -247,7 +247,8 @@ const c = @cImport({
 ///
 /// If the theme is not an absolute pathname, two different directories will be
 /// searched for a file name that matches the theme. This is case sensitive on
-/// systems with case-sensitive filesystems.
+/// systems with case-sensitive filesystems. It is an error for it to include
+/// path separators.
 ///
 /// The first directory is the `themes` subdirectory of your Ghostty
 /// configuration directory. This is `$XDG_CONFIG_DIR/ghostty/themes` or
@@ -2204,11 +2205,9 @@ pub fn themeDir(alloc: std.mem.Allocator, type_: ThemeDirType) ?[]const u8 {
 }
 
 fn loadTheme(self: *Config, theme: []const u8) !void {
-    const alloc = self._arena.?.allocator();
-
     // Find our theme file and open it. See the open function for details.
     const file: std.fs.File = (try themepkg.open(
-        alloc,
+        &self._arena.?,
         theme,
         &self._errors,
     )) orelse return;

src/config/theme.zig

@@ -1,48 +1,43 @@
 const std = @import("std");
 const builtin = @import("builtin");
 const assert = std.debug.assert;
-const Allocator = std.mem.Allocator;
+const ArenaAllocator = std.heap.ArenaAllocator;
 const global_state = &@import("../main.zig").state;
 const internal_os = @import("../os/main.zig");
 const ErrorList = @import("ErrorList.zig");
 
-/// Location of possible themes. The order of this enum matters because
+/// Location of possible themes. The order of this enum matters because it
-/// it defines the priority of theme search (from top to bottom).
+/// defines the priority of theme search (from top to bottom).
 pub const Location = enum {
-    user, // xdg config dir
+    user, // XDG config dir
     resources, // Ghostty resources dir
 
     /// Returns the directory for the given theme based on this location type.
     ///
-    /// This will return null with no error if the directory type doesn't
+    /// This will return null with no error if the directory type doesn't exist
-    /// exist or is invalid for any reason. For example, it is perfectly
+    /// or is invalid for any reason. For example, it is perfectly valid to
-    /// valid to install and run Ghostty without the resources directory.
+    /// install and run Ghostty without the resources directory.
     ///
-    /// This may allocate memory but it isn't guaranteed so the allocator
+    /// Due to the way allocations are handled, a pointer to an Arena allocator
-    /// should be something like an arena. It isn't safe to always free the
+    /// must be used.
-    /// resulting pointer.
     pub fn dir(
         self: Location,
-        alloc_arena: Allocator,
+        arena: *ArenaAllocator,
-        theme: []const u8,
     ) error{OutOfMemory}!?[]const u8 {
-        if (comptime std.debug.runtime_safety) {
+        const alloc = arena.allocator();
-            assert(!std.fs.path.isAbsolute(theme));
+
-        }
+        // if (comptime std.debug.runtime_safety) {
+        //     assert(!std.fs.path.isAbsolute(theme));
+        // }
 
         return switch (self) {
             .user => user: {
-                var buf: [std.fs.max_path_bytes]u8 = undefined;
+                const subdir = std.fs.path.join(alloc, &.{
-                const subdir = std.fmt.bufPrint(
+                    "ghostty", "themes",
-                    &buf,
+                }) catch return error.OutOfMemory;
-                    "ghostty/themes/{s}",
-                    .{theme},
-                ) catch |err| switch (err) {
-                    error.NoSpaceLeft => return error.OutOfMemory,
-                };
 
                 break :user internal_os.xdg.config(
-                    alloc_arena,
+                    alloc,
                     .{ .subdir = subdir },
                 ) catch |err| switch (err) {
                     error.OutOfMemory => return error.OutOfMemory,
@@ -55,32 +50,37 @@ pub const Location = enum {
                 };
             },
 
-            .resources => try std.fs.path.join(alloc_arena, &.{
+            .resources => try std.fs.path.join(alloc, &.{
                 global_state.resources_dir orelse return null,
                 "themes",
-                theme,
             }),
         };
     }
 };
 
-/// An iterator that returns all possible locations for a theme in order
+/// An iterator that returns all possible directories for finding themes in
-/// of priority.
+/// order of priority.
 pub const LocationIterator = struct {
-    alloc_arena: Allocator,
+    arena: *ArenaAllocator,
-    theme: []const u8,
     i: usize = 0,
 
-    pub fn next(self: *LocationIterator) !?[]const u8 {
+    pub fn next(self: *LocationIterator) !?struct {
+        location: Location,
+        dir: []const u8,
+    } {
         const max = @typeInfo(Location).Enum.fields.len;
-        while (true) {
+        std.debug.print("a: {d} {d}\n", .{ self.i, max });
-            if (self.i >= max) return null;
+        while (self.i < max) {
-            const loc: Location = @enumFromInt(self.i);
+            std.debug.print("b: {d}\n", .{self.i});
+            const location: Location = @enumFromInt(self.i);
             self.i += 1;
-            const dir_ = try loc.dir(self.alloc_arena, self.theme);
+            if (try location.dir(self.arena)) |dir|
-            const dir = dir_ orelse continue;
+                return .{
-            return dir;
+                    .location = location,
+                    .dir = dir,
+                };
         }
+        return null;
     }
 
     pub fn reset(self: *LocationIterator) void {
@@ -94,23 +94,42 @@ pub const LocationIterator = struct {
 ///
 /// One error that is not recoverable and may be returned is OOM. This is
 /// always a critical error for configuration loading so it is returned.
+///
+/// Due to the way allocations are handled, a pointer to an Arena allocator
+/// must be used.
 pub fn open(
-    alloc_arena: Allocator,
+    arena: *ArenaAllocator,
     theme: []const u8,
     errors: *ErrorList,
 ) error{OutOfMemory}!?std.fs.File {
+
     // Absolute themes are loaded a different path.
     if (std.fs.path.isAbsolute(theme)) return try openAbsolute(
-        alloc_arena,
+        arena,
         theme,
         errors,
     );
 
+    const alloc = arena.allocator();
+
+    const basename = std.fs.path.basename(theme);
+    if (!std.mem.eql(u8, theme, basename)) {
+        try errors.add(alloc, .{
+            .message = try std.fmt.allocPrintZ(
+                alloc,
+                "theme \"{s}\" cannot include path separators unless it is an absolute path",
+                .{theme},
+            ),
+        });
+        return null;
+    }
+
     // Iterate over the possible locations to try to find the
     // one that exists.
-    var it: LocationIterator = .{ .alloc_arena = alloc_arena, .theme = theme };
+    var it: LocationIterator = .{ .arena = arena };
     const cwd = std.fs.cwd();
-    while (try it.next()) |path| {
+    while (try it.next()) |loc| {
+        const path = try std.fs.path.join(alloc, &.{ loc.dir, theme });
         if (cwd.openFile(path, .{})) |file| {
             return file;
         } else |err| switch (err) {
@@ -119,9 +138,9 @@ pub fn open(
 
             // Anything else is an error we log and give up on.
             else => {
-                try errors.add(alloc_arena, .{
+                try errors.add(alloc, .{
                     .message = try std.fmt.allocPrintZ(
-                        alloc_arena,
+                        alloc,
                         "failed to load theme \"{s}\" from the file \"{s}\": {}",
                         .{ theme, path, err },
                     ),
@@ -137,10 +156,11 @@ pub fn open(
     // This does double allocate some memory but for errors I think thats
     // fine.
     it.reset();
-    while (try it.next()) |path| {
+    while (try it.next()) |loc| {
-        try errors.add(alloc_arena, .{
+        const path = try std.fs.path.join(alloc, &.{ loc.dir, theme });
+        try errors.add(alloc, .{
             .message = try std.fmt.allocPrintZ(
-                alloc_arena,
+                alloc,
                 "theme \"{s}\" not found, tried path \"{s}\"",
                 .{ theme, path },
             ),
@@ -154,23 +174,27 @@ pub fn open(
 /// then messages will be appended to the given error list and null is
 /// returned. If a non-null return value is returned, there are never any
 /// errors added.
+///
+/// Due to the way allocations are handled, a pointer to an Arena allocator
+/// must be used.
 pub fn openAbsolute(
-    alloc_arena: Allocator,
+    arena: *ArenaAllocator,
     theme: []const u8,
     errors: *ErrorList,
 ) error{OutOfMemory}!?std.fs.File {
+    const alloc = arena.allocator();
     return std.fs.openFileAbsolute(theme, .{}) catch |err| {
         switch (err) {
-            error.FileNotFound => try errors.add(alloc_arena, .{
+            error.FileNotFound => try errors.add(alloc, .{
                 .message = try std.fmt.allocPrintZ(
-                    alloc_arena,
+                    alloc,
                     "failed to load theme from the path \"{s}\"",
                     .{theme},
                 ),
             }),
-            else => try errors.add(alloc_arena, .{
+            else => try errors.add(alloc, .{
                 .message = try std.fmt.allocPrintZ(
-                    alloc_arena,
+                    alloc,
                     "failed to load theme from the path \"{s}\": {}",
                     .{ theme, err },
                 ),