From 7c893881c349203fbc0e987e55988eaf887848a9 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Fri, 17 May 2024 17:13:43 -0500 Subject: [PATCH] Address review comments 1. Switch to using Wyhash instead of a cryptographic hash. 2. Move global variables to App struct. --- src/App.zig | 6 ++++++ src/Surface.zig | 33 +++++++++++++++++++-------------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/App.zig b/src/App.zig index 41a7887b2..314d0b25b 100644 --- a/src/App.zig +++ b/src/App.zig @@ -45,6 +45,12 @@ quit: bool, /// same font configuration. font_grid_set: font.SharedGridSet, +// Used to rate limit desktop notifications. Some platforms (notably macOS) will +// run out of resources if desktop notifications are sent too fast and the OS +// will kill Ghostty. +last_notification_time: ?std.time.Instant = null, +last_notification_digest: u64 = 0, + /// Initialize the main app instance. This creates the main window, sets /// up the renderer state, compiles the shaders, etc. This is the primary /// "startup" logic. diff --git a/src/Surface.zig b/src/Surface.zig index 8ab678744..28e702d7a 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -3486,32 +3486,37 @@ fn completeClipboardReadOSC52( self.io_thread.wakeup.notify() catch {}; } -const hash_algorithm = std.crypto.hash.sha2.Sha224; -var last_notification_time: ?std.time.Instant = null; -var last_notification_digest = [_]u8{0} ** hash_algorithm.digest_length; +// Wyhash is used to hash the contents of the desktop notification to limit how +// fast identical notifications can be sent sequentially. +const hash_algorithm = std.hash.Wyhash; +// This seed for Wyhash was literally chosen at random. The actual seed (AFAIK) +// shouldn't matter as long as it stays constant. +const hash_seed = 0xb8179c65b93cc558; fn showDesktopNotification(self: *Surface, title: [:0]const u8, body: [:0]const u8) !void { if (@hasDecl(apprt.Surface, "showDesktopNotification")) { const now = try std.time.Instant.now(); - var new_notification_digest: [hash_algorithm.digest_length]u8 = undefined; - - if (last_notification_time) |last| { + // Set a limit of one desktop notification per second so that the OS + // doesn't kill us when we run out of resources. + if (self.app.last_notification_time) |last| { if (now.since(last) < 1 * std.time.ns_per_s) { log.warn("rate limiting desktop notifications", .{}); return; } } - { - var hash = hash_algorithm.init(.{}); + const new_notification_digest = d: { + var hash = hash_algorithm.init(hash_seed); hash.update(title); hash.update(body); - hash.final(&new_notification_digest); - } + break :d hash.final(); + }; - if (last_notification_time) |last| { - if (std.mem.eql(u8, &last_notification_digest, &new_notification_digest)) { + // Set a limit of one notification per five seconds for desktop + // notifications with identical content. + if (self.app.last_notification_time) |last| { + if (self.app.last_notification_digest == new_notification_digest) { if (now.since(last) < 5 * std.time.ns_per_s) { log.warn("suppressing identical desktop notification", .{}); return; @@ -3519,8 +3524,8 @@ fn showDesktopNotification(self: *Surface, title: [:0]const u8, body: [:0]const } } - last_notification_time = now; - @memcpy(&last_notification_digest, &new_notification_digest); + self.app.last_notification_time = now; + self.app.last_notification_digest = new_notification_digest; try self.rt_surface.showDesktopNotification(title, body); } else log.warn("runtime doesn't support desktop notifications", .{});