config: window-inherit-working-directory options

[jparise]

window-inherit-working-directory was previously a boolean value. This change makes it a more granular option, allowing you to enable this behavior for one or more surface "kinds": split, tab, window.

The full behavior is only implemented for the embedded runtime and macOS app. The other runtimes still need their own plumbing to track surface kind creation (stubbed as .window by default in this change).

Resolves #1392

[mitchellh]

So far so good. Am I missing where the kind is actually set from the apprts?

[jparise]

So far so good. Am I missing where the kind is actually set from the apprts?

For the embedded runtime, I'm setting it from newSurfaceOptions. For the others, I added lines like this pending a real implementation:

const kind: CoreSurface.Kind = .window; // TODO
var config = try apprt.surface.newConfig(app.app, &app.config, kind);

[jparise]

A few notes on the configuration parsing:

Parsing Bools

To maintain some backwards compatibility with the previous boolean config value, I added logic to the parseCLI implementation. But this could also be generalized for all packed struct parsing with something like this:

diff --git i/src/cli/args.zig w/src/cli/args.zig
index 49c5152a..acf9c839 100644
--- i/src/cli/args.zig
+++ w/src/cli/args.zig
@@ -281,6 +281,8 @@ fn parsePackedStruct(comptime T: type, v: []const u8) !T {
     const info = @typeInfo(T).Struct;
     assert(info.layout == .@"packed");
 
+    const parseBoolFn: ?std.builtin.Type.Fn = if (@hasDecl(T, "parseBool")) @typeInfo(@TypeOf(T.parseBool)).Fn else null;
+
     var result: T = .{};
 
     // We split each value by ","
@@ -306,6 +308,20 @@ fn parsePackedStruct(comptime T: type, v: []const u8) !T {
             }
         }
 
+        if (parseBoolFn) |pb| {
+            const b = parseBool(part) catch |err| switch (err) {
+                error.InvalidValue => continue :loop,
+                else => return err,
+            };
+
+            switch (pb.params.len) {
+                2 => try result.parseBool(b),
+                else => @compileError("parseBool invalid argument count"),
+            }
+
+            continue :loop;
+        }
+
         // No field matched
         return error.InvalidValue;
     }

This felt like the most flexible approach, but there are two other related options we could take:

1. Always support boolean values in packed struct config options, where "true" means "flip all boolean fields to on" and "false" means the opposite.
2. Opt in to (1) based on the presence of some declaration in the target struct rather than it being automatic for all packed structs.

Default Field Values

The current parsePackedStruct parses and sets fields on top of the struct's default field values. In this case, we want to default all of the window-inherit-working-directory fields to true, but that means that seeing a configuration value like split will only visit that one field (leaving the other fields set to true), which doesn't seem like the expected behavior given an explicit configuration value. Instead, the user would need to spell out split,no-tab,no-window (or just no-tab,no-window) if they only want the split behavior.

This is why I have this line in my custom parseCLI function:

self.* = .{ .split = false, .tab = false, .window = false };
var iter = std.mem.splitSequence(u8, value, ",");
// ...

I can't decide if this is a shortcoming of the currently parsePackedStruct implementation or intentional behavior because it sort of depends on how we've viewed the default field values and the no-... value support (i.e. the user knows the full defaults and sets a configuration value string to specifically alter the ones they want to change).

If the former, it wouldn't be hard to fix -- reset all fields to "off" before parsing the new, explicit values -- but I thought I'd bring it up in context here first.

---

With some blend of the above, configuration types like this new WindowInheritWorkingDirectory would be a lot easier to implement.

[jparise]

In the interest of moving this forward, I simplified this to use the standard packed struct config behavior. This means there is no backward compatibility for true / false boolean values, and because all of the field values default to true, people will primarily use the configuration to turn things off (e.g. no-tab,no-window).

[mitchellh]

Thanks @jparise. My main delay in merging this was that plus one more thing: I don't super love how information is flowing through this. It may be correct, though, but I want to spend some time thinking about it and just haven't yet... annoyingly, so sorry.

[jparise]

Thanks @jparise. My main delay in merging this was that plus one more thing: I don't super love how information is flowing through this. It may be correct, though, but I want to spend some time thinking about it and just haven't yet... annoyingly, so sorry.

No worries! Happy to take your feedback (once you find time for it), or if you'd prefer to restructure things yourself, that sounds great, too.

[jparise]

Thanks @jparise. My main delay in merging this was that plus one more thing: I don't super love how information is flowing through this. It may be correct, though, but I want to spend some time thinking about it and just haven't yet... annoyingly, so sorry.

I tried out another approach locally that moves the window-inherit-working-directory logic out of newConfig and into (for the embedded apprt) newSurfaceOptions, placing it alongside the window-inherit-font-size logic.

const font_size: f32 = font_size: { // ...

const working_directory: [*:0]const u8 = working_directory: { // ...

return .{
    .font_size = font_size,
    .working_directory = working_directory,
};

1. It removes CoreSurface.Kind. The apprt can just use whatever signally mechanism it wants between callbacks like newSplit and its surface creation routines.
2. At least in the embedded apprt world, it makes use of the existing working_directory surface creation option.
3. newConfig can go away because it's now empty. apprts could just call config.shallowClone themselves.

... but the downside is that it makes each apprt responsible for the window-inherit-working-directory config + pwd logic. (Maybe I could hoist some of it into a common function in a apprt/surface.zig they could all call.)

Does this feel like a nicer approach to pursue?

[jparise]

I'm back to playing with this approach:

I tried out another approach locally that moves the window-inherit-working-directory logic out of newConfig and into (for the embedded apprt) newSurfaceOptions, placing it alongside the window-inherit-font-size logic.

const font_size: f32 = font_size: { // ...

const working_directory: [*:0]const u8 = working_directory: { // ...

return .{
    .font_size = font_size,
    .working_directory = working_directory,
};

I generally prefer it, but I'm not sure "who" should own the working_directory allocation, which comes from CoreSurface.pwd (copied from the IO thread). Surface.Options is an extern struct, so I'm looking for a nice pattern for the Surface.Options result (from newSurfaceOptions) to reference that [*:0]const u8 copy (without owning it), allowing the e.g. Swift runtime to safely overwrite that field with a new reference.

All of these objects have different lifetimes, and I don't think I can add an allocator to Surface.Options itself (because extern struct), so I'm not sure if that's a sign that this overall approach is flawed.

Any advice, @mitchellh?

[jparise]

All of these objects have different lifetimes, and I don't think I can add an allocator to Surface.Options itself (because extern struct), so I'm not sure if that's a sign that this overall approach is flawed.

I suppose I could try stuffing the ArenaAllocator into Surface.Options as an *anyopaque pointer, but that seems... unsafe and gross.

[mitchellh]

Sorry for neglecting this PR. I'm still on the fence about the approach. I go back and forth on it. With the recent tmux control mode issue, I've been considering making the core more aware of the window/tab/split hierarchy. That would make doing something like this a lot simpler I think (I mean, this PR is already only +/- like 80 lines, but I'm not sure about all the data ownership).

I've also considered making the window vs tab vs split config related to the "conditional config" we've talked about in the issue for system theme changes. For example, I could imagine hypothetically letting you do window-inherit-working-directory[split] = true or something (hypothetical syntax, none of it is real). And that would be the same syntax as say theme[system-theme=dark] = whatever. So if we implemented that system, it's possible you get this feature "for free" so to speak.

That's my current line of thinking... anyways... sorry for the delay.

[jparise]

Sorry for neglecting this PR. I'm still on the fence about the approach. I go back and forth on it. With the recent tmux control mode issue, I've been considering making the core more aware of the window/tab/split hierarchy. That would make doing something like this a lot simpler I think (I mean, this PR is already only +/- like 80 lines, but I'm not sure about all the data ownership).

That sounds good, although the trickier bits here have been more about configuration data flow and ownership rather than tracking the surface types. Any architectural improvements here would be easy to build upon, though!

I've also considered making the window vs tab vs split config related to the "conditional config" we've talked about in the issue for system theme changes. For example, I could imagine hypothetically letting you do window-inherit-working-directory[split] = true or something (hypothetical syntax, none of it is real). And that would be the same syntax as say theme[system-theme=dark] = whatever. So if we implemented that system, it's possible you get this feature "for free" so to speak.

Is the applicable idea that we'd have a general surface (or kind) dimension that could be applied to all (or a distinct subset of) configuration fields (e.g. window-inherit-working-directory[surface=split] = true, window-inherit-font-size[surface=split] = true, etc.)?

[jparise]

All of these objects have different lifetimes, and I don't think I can add an allocator to Surface.Options itself (because extern struct), so I'm not sure if that's a sign that this overall approach is flawed.

I suppose I could try stuffing the ArenaAllocator into Surface.Options as an *anyopaque pointer, but that seems... unsafe and gross.

I pushed an initial (rough) implementation of this approach for reference. Understood that it's probably not the approach we ultimately want to take with this.

[jparise]

I acknowledge this is gross for multiple reasons.

[jparise]

I've also considered making the window vs tab vs split config related to the "conditional config" we've talked about in the issue for system theme changes. For example, I could imagine hypothetically letting you do window-inherit-working-directory[split] = true or something (hypothetical syntax, none of it is real). And that would be the same syntax as say theme[system-theme=dark] = whatever. So if we implemented that system, it's possible you get this feature "for free" so to speak.

Nothing that the work started in #2735 might provide what we need to unblock this aspect of this change.

[Snikimonkd]

maybe it's better to make this as a parameter (enum) for new_split and new_tab with values like cwd and home for example
so then we can create keybinds like this:

keybind = super+d=new_split:home

in this approach surface does not need to know anything about splits or tabs

[mitchellh]

Chatted with @jparise on Discord awhile back about next steps for this PR and I think we're on the same page so I'm going to close this for now.

The config bike shed isn't too hard, the hard part is actually the plumbing of awareness of what a thing is. Ghostty core isn't aware of what is a tab split window etc. That's all in the apprts. So plumbing that through in a thoughtful way is the "hard" part (not technically hard, but hard to design well).

[jparise]

Ghostty core isn't aware of what is a tab split window etc. That's all in the apprts. So plumbing that through in a thoughtful way is the "hard" part (not technically hard, but hard to design well).

That's exactly right. I've tried a few versions of it but haven't landed on something that I liked enough to submit.