Skip to content

gtk: implement global shortcuts #7083

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

Merged
merged 2 commits into from
May 19, 2025
Merged

Conversation

pluiedev
Copy link
Member

It's been a lot of D-Bus related pain and suffering, but here it is.

I'm not sure about how well this is integrated inside App, but I'm fairly
proud of the standalone logic.

@pluiedev pluiedev requested a review from a team as a code owner April 14, 2025 08:46
@tristan957
Copy link
Member

I wonder what the diff would look like if we used libportal

@pluiedev
Copy link
Member Author

pluiedev commented Apr 14, 2025

Well... libportal doesn't implement this yet, and it doesn't seem like anyone else is interested. (There's an open PR from July 2024, but nobody's left a single comment yet: flatpak/libportal#153) Would make my day a whole lot easier if it were the case to be honest

It also would've been a lot easier if they made the API extensible so that you could write your own portal extensions via libportal, but sadly we can't have nice things

@pluiedev pluiedev force-pushed the push-ozotwnwrtutq branch 3 times, most recently from 2479f30 to 620e718 Compare April 14, 2025 16:03
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work. That fact it's so encapsulated is really well done. I agree with @tristan957 on the test request for the one function, then I just hit the "red" button to consider the ArrayHashMap change.

var trigger_buf: [256]u8 = undefined;

self.map.clearRetainingCapacity();
var it = self.app.config.keybind.set.bindings.iterator();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: create better API for this, I don't think the apprt's should be doing this level of introspection. Its totally fine for this PR, but I'll get around to a better API soon, because its in the hot path of me implementing global shortcuts on macOS that don't require special permissions (that they do today).

@pluiedev pluiedev force-pushed the push-ozotwnwrtutq branch from 620e718 to 6f1521b Compare April 22, 2025 03:49
@pluiedev pluiedev requested a review from mitchellh April 22, 2025 03:49
@00-kat 00-kat added the os/linux label May 9, 2025
@pluiedev pluiedev force-pushed the push-ozotwnwrtutq branch from 6f1521b to 646dbf8 Compare May 15, 2025 15:21
Copy link
Member

@jcollie jcollie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@jcollie jcollie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pluiedev
Copy link
Member Author

Gonna delay merging this until GNOME 48 support hits NixOS unstable/25.05 (PR, tracker) since I wanna see how well this works on GNOME first

@jcollie
Copy link
Member

jcollie commented May 17, 2025

Trying out now with Gnome 48 and I'm getting the following crash:

debug(gtk): windowing protocol=wayland
debug(gtk): cgroup isolation disabled config=config.Config.LinuxCgroup.single-instance
Segmentation fault at address 0x7f3994500008
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/heap/arena_allocator.zig:185:77: 0x314ee6d in alloc (ghostty)
            const cur_alloc_buf = @as([*]u8, @ptrCast(cur_node))[0..cur_node.data];
                                                                            ^
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/mem/Allocator.zig:129:26: 0x315a1da in allocBytesWithAlignment__anon_15860 (ghostty)
    return a.vtable.alloc(a.ptr, len, alignment, ret_addr);
                         ^
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/mem/Allocator.zig:260:40: 0x314a412 in allocWithSizeAndAlignment__anon_13957 (ghostty)
    return self.allocBytesWithAlignment(alignment, byte_count, return_address);
                                       ^
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/mem/Allocator.zig:254:75: 0x3130b58 in alloc__anon_11105 (ghostty)
    const ptr: [*]align(a) T = @ptrCast(try self.allocWithSizeAndAlignment(@sizeOf(T), a, n, return_address));
                                                                          ^
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/mem.zig:3222:36: 0x33ad9fb in joinMaybeZ (ghostty)
    const buf = try allocator.alloc(u8, total_len);
                                   ^
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/mem.zig:3208:31: 0x32fda62 in joinZ (ghostty)
    const out = try joinMaybeZ(allocator, separator, slices, true);
                              ^
/home/jeff/dev/ghostty/src/apprt/gtk/GlobalShortcuts.zig:400:42: 0x32fd8b1 in getRequestPath (ghostty)
    const object_path = try std.mem.joinZ(self.arena.allocator(), "/", &.{
                                         ^
/home/jeff/dev/ghostty/src/apprt/gtk/GlobalShortcuts.zig:348:49: 0x32fdd6a in request__anon_158218 (ghostty)
    const request_path = try self.getRequestPath(request_token);
                                                ^
/home/jeff/dev/ghostty/src/apprt/gtk/GlobalShortcuts.zig:114:21: 0x32fea12 in refreshSession (ghostty)
    try self.request(.create_session);
                    ^
/home/jeff/dev/ghostty/src/apprt/gtk/App.zig:1022:33: 0x32ffea7 in syncConfigChanges (ghostty)
        shortcuts.refreshSession(self) catch |err| {
                                ^
/home/jeff/dev/ghostty/src/apprt/gtk/App.zig:1317:27: 0x3322b9c in run (ghostty)
    self.syncConfigChanges(null) catch |err| {
                          ^
/home/jeff/dev/ghostty/src/main_ghostty.zig:114:24: 0x3323a88 in main (ghostty)
    try app_runtime.run();
                       ^
/nix/store/hmmxm8rfbmdvcd45xi7h42zsyi4crbrb-zig-0.14.0/lib/zig/std/start.zig:656:37: 0x33245b7 in main (ghostty)
            const result = root.main() catch |err| {
                                    ^
???:?:?: 0x7f39a7e2a47d in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7f39a7e2a47d` was not available, trace may be incomplete

@pluiedev
Copy link
Member Author

Hmmm.. I'm running into the same error. Lemme see here

@jcollie
Copy link
Member

jcollie commented May 17, 2025

This seems to fix it:

diff --git a/src/apprt/gtk/GlobalShortcuts.zig b/src/apprt/gtk/GlobalShortcuts.zig
index 09d487686..c4ea67df5 100644
--- a/src/apprt/gtk/GlobalShortcuts.zig
+++ b/src/apprt/gtk/GlobalShortcuts.zig
@@ -83,7 +83,6 @@ pub fn refreshSession(self: *GlobalShortcuts, app: *App) !void {
     // Ensure we have a valid reference to the app
     // (it was left uninitialized in `init`)
     self.app = app;
-    self.deinit();

     // Update map
     var trigger_buf: [256]u8 = undefined;

pluiedev added 2 commits May 18, 2025 22:40
It's been a lot of D-Bus related pain and suffering, but here it is.

I'm not sure about how well this is integrated inside App, but I'm fairly
proud of the standalone logic.
Compiling this list of known supported and unsupported platforms has been
amazingly painful. Never change, Linux desktop.
@pluiedev pluiedev force-pushed the push-ozotwnwrtutq branch from 646dbf8 to 6827dc0 Compare May 18, 2025 20:40
@pluiedev
Copy link
Member Author

pluiedev commented May 18, 2025

Works in my VM testing on both GNOME 48 and Plasma 6.3, NixOS unstable (25.11).

@jcollie Can you see if it works on your end?

@jcollie
Copy link
Member

jcollie commented May 18, 2025

Works in my VM testing on both GNOME 48 and Plasma 6.3, NixOS unstable (25.11).

@jcollie Can you see if it works on your end?

YESSS!!!! working for me now!

@pluiedev
Copy link
Member Author

Let's do this

@pluiedev pluiedev merged commit 60d8c42 into ghostty-org:main May 19, 2025
37 checks passed
@github-actions github-actions bot added this to the 1.2.0 milestone May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants