Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command palette #5623

Open
pluiedev opened this issue Feb 7, 2025 · 20 comments
Open

Command palette #5623

pluiedev opened this issue Feb 7, 2025 · 20 comments
Labels
feature-design Almost-accepted feature request pending design. gui GUI or app issue regardless of platform (i.e. Swift, GTK)

Comments

@pluiedev
Copy link
Contributor

pluiedev commented Feb 7, 2025

Discussed in #4195

Originally posted by Xophmeister December 31, 2024
In tmux, Prefix+: will open a command line input to allow you to enter arbitrary tmux commands. This is similar to Vim, where : enters command line mode.

It would be useful to have such a feature in Ghostty to try out actions, without having to define a keybinding for them.

Maintainer's Note: Instead of a text interface with commands, it would be perhaps more idiomatic for a GUI application to display a pop up command palette instead, à la VS Code, IntelliJ IDEA, Blender, etc. There, we can also display default keybindings, which can be more easily learned by new users.

@tristan957
Copy link
Collaborator

I agree with what you said. Have you looked at GNOME Builder's UX for this for the GTK apprt? I'm hoping this is something libadwaita will provide in the future, but should not block an initial implementation.

@pluiedev
Copy link
Contributor Author

pluiedev commented Feb 7, 2025

Prior art:

Working demo (GTK):

2025-02-07_02-43-47.mp4

@pluiedev
Copy link
Contributor Author

pluiedev commented Feb 7, 2025

Have you looked at GNOME Builder's UX for this for the GTK apprt?

I haven't — there are some technical issues behind this that would make its inclusion in libadwaita somewhat difficult. (Essentially libadwaita doesn't support ListViews for its boxed list pattern, and ListBoxes don't really react dynamically enough to updating queries or are scalable enough for potentially a large number of entries.)

@pluiedev pluiedev added the gui GUI or app issue regardless of platform (i.e. Swift, GTK) label Feb 7, 2025
@jcollie
Copy link
Collaborator

jcollie commented Feb 7, 2025

Working demo (GTK):
2025-02-07_02-43-47.mp4

Can we fix the width of the window? I find the changing size as the results are narrowed down distracting.

@mitchellh
Copy link
Contributor

Great work on the GTK side, looks good!

How would parameterized commands work with this? I suggest we just keep it simple and follow the same format as expected for keybind because that is also the underlying core API we invoke. But how should we show it in the palette?

It'd be great to comptime generate a lot of the metadata that this uses (if we aren't already).

@jcollie jcollie added the feature-design Almost-accepted feature request pending design. label Feb 7, 2025
@jcollie
Copy link
Collaborator

jcollie commented Feb 7, 2025

How would parameterized commands work with this? I suggest we just keep it simple and follow the same format as expected for keybind because that is also the underlying core API we invoke. But how should we show it in the palette?

Maybe show it with a trailing : to indicate that it needs a parameter of some sort.

@pluiedev
Copy link
Contributor Author

pluiedev commented Feb 7, 2025

I went with the approach to have separate commands for each parameterized keybind.

We need some sort of manual list of commands anyway, since autogenerating from either apprt.Action or input.binding.Action isn't really wise — there are some actions in both tagged unions that just don't make sense for a GUI context. (For instance, all the text input binds for input.binding.Action, and the internal apprt actions like renderer_health or render_inspector.)

Image

It even supports showing the custom keybinds the user has set, because this is actually reusing the same key.accelFromTrigger mechanism we already have. (Below is default settings)

Image

@mitchellh
Copy link
Contributor

We need some sort of manual list of commands anyway, since autogenerating from either apprt.Action or input.binding.Action isn't really wise — there are some actions in both tagged unions that just don't make sense for a GUI context.

I haven't thought much about it but the premise makes sense, I do challenge how to manually specify though. I think it could make sense to do a comptime enum subset in src/input/Binding.zig similar to how do for src/apprt/action.zig. It's a little crazy but it enforces a bunch of compiler safety that helps make this a reasonable endeavor.

We could then expose this data via libghostty as well (for macOS and future embedder).

@pluiedev
Copy link
Contributor Author

pluiedev commented Feb 7, 2025

That wouldn't really work that well... Say we have new_split. To parametrize it would mean to split it into five brand new ones: one for each cardinal direction and one for auto split. That would mean we need to add four new entries to the enum.

(From the implementor's perspective, on GTK at least it would be much easier if commands are just enums, since that means we can use AdwEnumListModel and avoid adding data to the list model ourselves. We can then map each command to the corresponding parametrized action, and the human-readable command name.)

@jcollie
Copy link
Collaborator

jcollie commented Feb 7, 2025

With comptime it's easy enough to generate enums, at least for things that have a bounded set of choices. Things like text:, increase_font_size and decrease_font_size would be more problematic.

Maybe a hybrid approach? comptime generate what we can and then augment with some curated manual additions.

@pluiedev
Copy link
Contributor Author

pluiedev commented Feb 7, 2025

We just shouldn't allow stuff like csi or text. The terminal is right there. For {in,de}crease_font_size we can just set the increment/decrement to 1, which works pretty well for me. An alternative idea is to spawn a new dialog with a counter that the user can input into, though that sounds very much overkill.

@jcollie
Copy link
Collaborator

jcollie commented Feb 7, 2025

Yeah csi and text (and esc) should not be included. Basically any action that has an unbounded set of parameters like that should be disallowed. Yeah, for the font size we'd manually add increment/decrement by one (and any others that make sense).

@mitchellh
Copy link
Contributor

That wouldn't really work that well... Say we have new_split. To parametrize it would mean to split it into five brand new ones: one for each cardinal direction and one for auto split. That would mean we need to add four new entries to the enum.

Hm, not exactly what I'm thinking. I mean there would be a subset enum that has the subset of tags we support for the command palette. That subset enum then would fan out to the support command palette entries (either manually or automatically).

Rough pseudo-code:

pub const CommandPaletteEntry = struct {
    name: [:0]const u8,
    description: [:0]const u8,
    action: Action,
};

pub const Action = union(Tag) { 
    // existing input.Binding.Action
};

pub const Tag = enum {
    ...

    // Can return null to indicate the tag doesn't support command palettes.
    pub fn commandPalette(comptime self: Tag) ?[]const CommandPaletteEntry {
        ...

        // We wouldn't have to make this fully manual. Things like `new_split` could
        // call something like `CommandPaletteEntry.generate(...)` to inspect that
        // its a non-exhaustive enum value and generate all the combinations.
    }
};

pub const command_palette: []const CommandPaletteEntry = comptime { };

I'm not sure that's exactly the right shape.

In any case, the goal would be, if we add a new action to Action, we'd get a compiler error telling us we need to ensure we add our commandPalette entry (which may be null). And the full list of entries would be available for all apprts so we can easily export via libghostty and so on.

@mitchellh
Copy link
Contributor

FYI the reason I'm banging on this is because I don't want to see this overly implemented in the apprt or in a way that makes it hacky or duplication-heavy for other apprts. This is a common ask across platforms so I think enabling it from the core is a good approach.

@jcollie
Copy link
Collaborator

jcollie commented Feb 7, 2025

Yeah, I hadn't thought about the specific code yet but this is also along the lines of what I'm thinking.

@pluiedev
Copy link
Contributor Author

pluiedev commented Feb 7, 2025

This would make the GTK side difficult, though. I've managed to avoid needing to feed any data to GTK directly since libadwaita's AdwEnumListModel sort of automagically does that away. This approach would make the data entry part far more difficult than it should be.

@mitchellh
Copy link
Contributor

I'll wait until I see the code then, I don't think I can understand until I see how that works.

@AlexJuca
Copy link
Contributor

@pluiedev Ideally we would want the interface to be similar or as close to the macOS version. Would this implementation only include the GTK version of this?

@jcollie
Copy link
Collaborator

jcollie commented Feb 10, 2025

@pluiedev Ideally we would want the interface to be similar or as close to the macOS version. Would this implementation only include the GTK version of this?

GUI elements should generally reflect the conventions for each individual platform, as long as equivalent functionality is exposed. For example, it would not make any sense to force the macOS "traffic lights" onto the GTK apprt.

@tristan957
Copy link
Collaborator

Here is how Builder's command palette looks:

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-design Almost-accepted feature request pending design. gui GUI or app issue regardless of platform (i.e. Swift, GTK)
Projects
None yet
Development

No branches or pull requests

5 participants