Add goto_window:next/previous binding action #9326
Conversation
| const target_node = switch (direction) { | ||
| .next => node.f_next orelse glist, | ||
| .previous => node.f_prev orelse last: { | ||
| var current = glist; |
There was a problem hiding this comment.
Thanks for all the feedback! I believe the Zig wrapper for this does not expose those two functions, so in order to use them I would have to import the C headers and then cast the wrapper type to the C pointer type. This is my first real Zig project, maybe I got it wrong. I thought the current way would maybe be cleaner. How should I proceed?
There was a problem hiding this comment.
They very much are present: https://ianjohnson.dev/zig-gobject/#glib2.List.last
Although I will be honest and say that the function signatures suck (in the C API, an empty list is represented as a null so all list-related APIs should be nullable), so it might be better to just do it in pure Zig instead.
There was a problem hiding this comment.
Thanks for the hint, I think I understand it better now. I've kept the pure Zig version but added Null handling similar to the quit function by making the result of the findCustom optional and falling back to the first window when no active window is found, is this better or do you want me to change it?
src/input/Binding.zig
Outdated
| @@ -884,6 +847,11 @@ pub const Action = union(enum) { | |||
| right, | |||
| }; | |||
|
|
|||
| pub const WindowDirection = enum { | |||
There was a problem hiding this comment.
Is there a reason this is named differently here? Why not keep it GotoWindow as well?
There was a problem hiding this comment.
My bad, overlooked this from an earlier version. Changed it to GotoWindow now
src/input/command.zig
Outdated
| .description = "Focus the previous window, if any.", | ||
| }, | ||
| .{ | ||
| .action = .{ .goto_window = .previous }, |
| move_tab, | ||
| goto_tab, | ||
| goto_split, | ||
| goto_window, |
There was a problem hiding this comment.
This needs to be added to the corresponding enum in ghostty.h as well.
src/Surface.zig
Outdated
| .{ .surface = self }, | ||
| .goto_window, | ||
| switch (direction) { | ||
| inline else => |tag| @field( |
There was a problem hiding this comment.
The inline else seems like overkill here since there are only two values and it's unlikely to ever change.
There was a problem hiding this comment.
Replaced this with directly handling next/previous cases
|
@brtmax there are some conflicts now |
|
Sorry, I forgot the upstream changes |
Nothing to be sorry about, it happens all the time. Thanks for your contribution, looks good to me, but let’s wait for the gtk or terminal team to approve this. |
src/apprt/gtk/class/application.zig
Outdated
| const gtk_window: *gtk.Window = @ptrCast(@alignCast(target_node.f_data orelse return false)); | ||
| gtk.Window.present(gtk_window); | ||
|
|
||
| const ghostty_window: *Window = @ptrCast(@alignCast(gtk_window)); |
There was a problem hiding this comment.
Don't do this for GTK wrapper types. Use a type-safe alternative like gobject.ext.as when upcasting to a parent type (and it's comptime known that this is safe), and gobject.ext.cast when downcasting to a child type.
There was a problem hiding this comment.
I tried to apply it, is this what you had in mind? Also thanks for the feedback as well
src/apprt/gtk/class/application.zig
Outdated
| _ = value.init(gobject.ext.typeFor(?*Surface)); | ||
| ghostty_window.as(gobject.Object).getProperty("active-surface", &value); | ||
|
|
||
| const surface: ?*Surface = @ptrCast(@alignCast(value.getObject())); |
src/apprt/gtk/class/application.zig
Outdated
| var value = std.mem.zeroes(gobject.Value); | ||
| defer value.unset(); | ||
| _ = value.init(gobject.ext.typeFor(?*Surface)); | ||
| ghostty_window.as(gobject.Object).getProperty("active-surface", &value); |
There was a problem hiding this comment.
I think using gobject.Object.get, even though it's only supposed to be used in C, might make things a bit cleaner here.
var surface: ?*gobject.Object = null;
ghostty_window.as(gobject.Object).get("active-surface", &surface, @as(?*anyopaque, null));
src/Surface.zig
Outdated
| .next => apprt.action.GotoWindow.next, | ||
| .previous => apprt.action.GotoWindow.previous, |
There was a problem hiding this comment.
Decl literals should just work here, no?
| .next => apprt.action.GotoWindow.next, | |
| .previous => apprt.action.GotoWindow.previous, | |
| .next => .next, | |
| .previous => .previous, |
There was a problem hiding this comment.
Thanks, yes it works! I noticed that I got the ordering inconsistent in this part, so I committed it again with correct ordering.
4 participants
Related to #8387
This implements cycling through windows (next/previous) by using the list gtk provides, as mentioned in the discussion in #8387 . I tried to follow the same pattern as 'goto_tab' and 'goto_split'. It presents the next target window and then transfers the focus to its active surface.
Sorry, this took some time. Also, this is a GTK implementation only, I currently don't have access to Mac hardware. Hope this is enough to start.
AI Disclosure:
I used deepwiki.com to familiarize myself with the repository and look for patterns to reuse.