From 9489f3828aea10b6fcae635578878cdc3eb45cc6 Mon Sep 17 00:00:00 2001 From: ntomsic Date: Thu, 21 May 2026 10:08:02 -0500 Subject: [PATCH] =?UTF-8?q?fix(audit):=20pass=201=20=E2=80=94=20ABI=20mism?= =?UTF-8?q?atches,=20threading,=20lifetime,=20undo=20redo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-audit pass 1 against PR #2 found three CRITICAL ABI bugs and several HIGH/MEDIUM hardening issues. CRITICAL — packed-struct ABI mismatches: libghostty's c_get serializes packed structs as `c_uint` (4 bytes) via `ptr.* = @intCast(@as(Backing, @bitCast(value)));`. This PR was reading three of them into 1- or 2-byte C structs, corrupting adjacent stack: - split-preserve-zoom: `struct { bool }` → c_uint via masked bits - app-notifications: `struct { bool, bool }` → c_uint via bits - notify-on-command-finish-action: same All three sites now read into `unsigned int` and mask bit 0 / bit 1 per the Zig field order in Config.zig. CRITICAL — Duration values via configGet always fail: Duration is a non-extern, non-packed Zig struct. c_get's struct branch returns false for it. Every site reading `quit-after-last-window-closed-delay` and `notify-on-command-finish-after` was getting the zero default. Added `parseDurationNs` helper that parses the libghostty duration string format (`1h30m45s`, `5ms`, etc.) from the on-disk config via configValue and now use it at all four call sites. HIGH: - undoLastClose now skips the quick terminal when picking the "active window" target so a Tab undo doesn't materialize in the dropdown. - redoLastClose stashes s_redoStack before re-closing because pushTabUndo / pushWindowUndo unconditionally clear it; without the stash a chain of redos lost everything past the first. - frame() iterates a snapshot of s_windows so a render-driven close can't invalidate the iterator. - XkbState now takes an xkb_keymap_ref while caching the keymap pointer as identity. Closes the ABA hazard where the xkb allocator could free + reuse the cached address for a different keymap. MEDIUM: - InspectorWindow validates restored geometry against the current screen layout; off-screen windows are re-centred on the primary screen so the user can actually find them. - main.cpp uses setOrganizationName instead of setOrganizationDomain (canonical for QSettings paths on Linux; setOrganizationDomain expects a reverse-DNS-like string and synthesizes macOS bundle IDs from it). - tabIndexForSurface null-checks both the surface arg and each tab page before isAncestorOf. - resizeSplit now clamps the delta against both panes' minimum so total area is conserved (was clamping per-side and forcing QSplitter to renormalise inconsistently). - Bell-audio path/volume now cached on the window and refreshed in applyWindowConfig instead of re-scanning the on-disk config on every bell. - initial-window: false now skips the bootstrap window's show() entirely (no flash). Removed the wantsInitialWindow / closeInitialWindow public helpers — the gate moved into newWindow itself. Per user note: dropped the non-Wayland fallback in setupLayerShell. The Qt frontend is Wayland-only by design (LayerShellQt + xkbcommon + wl_seat dependencies). On a missing layer-shell the path now logs and bails so failures are diagnosed instead of silently producing a non-functional regular window. Pre-Qt-6.8 palette synthesis was speculative — Debian trixie ships 6.8.2 and the project builds against that. Removed the synthesis path, which also closes the "Want::Follow doesn't restore palette" regression a reviewer flagged. Build verified via `docker build --target qt`. Co-Authored-By: claude-flow --- qt/PARITY.md | 2 +- qt/src/GhosttySurface.cpp | 16 +- qt/src/InspectorWindow.cpp | 18 +- qt/src/MainWindow.cpp | 376 ++++++++++++++++++++++--------------- qt/src/MainWindow.h | 14 +- qt/src/main.cpp | 22 +-- 6 files changed, 269 insertions(+), 179 deletions(-) diff --git a/qt/PARITY.md b/qt/PARITY.md index 203085366..a9a010f70 100644 --- a/qt/PARITY.md +++ b/qt/PARITY.md @@ -101,7 +101,7 @@ checkbox and link the commit hash. - [x] **B43.** `quick-terminal-screen` not honored. macOS resolves which monitor. — fixed in `6d700c36b` (handle->setScreen() before LayerShellQt anchoring; honors `main` / `mouse`; `macos-menu-bar` falls through to primary) - [x] ~~**B44.** `quick-terminal-position = center` not handled (`MainWindow.cpp:700`).~~ Audit was wrong; already handled at `MainWindow.cpp:766`. - [x] **B45.** `quick-terminal-space-behavior` not honored. — confirmed in `4c903802a` as a no-op. Wayland's wlr-layer-shell has no per-workspace pin; KWin always renders layer surfaces on the active workspace (= `move`). `remain` semantics are not achievable on Linux/Wayland. -- [x] **B46.** No fallback for non-Wayland — `LayerShellQt::Window::get()` returning null leaves a regular window without telling libghostty. — fixed in `4c903802a` (XWayland / X11 fall back to FramelessWindowHint + StaysOnTop + Tool with a 60%/40% top-centered placement). +- [x] ~~**B46.** No fallback for non-Wayland — `LayerShellQt::Window::get()` returning null leaves a regular window without telling libghostty.~~ Won't fix: the Qt frontend is Wayland-only by design (depends on LayerShellQt + xkbcommon + wl_seat). The setupLayerShell null-handle path now logs and bails so failures are diagnosed instead of silently producing a non-functional regular window. ### Misc diff --git a/qt/src/GhosttySurface.cpp b/qt/src/GhosttySurface.cpp index 82cf28da6..abf502754 100644 --- a/qt/src/GhosttySurface.cpp +++ b/qt/src/GhosttySurface.cpp @@ -753,16 +753,21 @@ private: } // The live keymap was rebuilt by the tracker (or we're picking - // up the first one). Drop our derived states and rebuild. + // up the first one). Drop our derived states and rebuild. Take + // an extra ref on the keymap while it's our cached identity so + // the xkb allocator can't free it and reuse the same address + // for a different keymap (the ABA hazard the previous comment + // hand-waved away). if (m_unshifted) xkb_state_unref(m_unshifted); if (m_query) xkb_state_unref(m_query); + if (m_keymap) xkb_keymap_unref(m_keymap); + m_keymap = xkb_keymap_ref(km); m_unshifted = xkb_state_new(km); m_query = xkb_state_new(km); m_idxShift = xkb_keymap_mod_get_index(km, XKB_MOD_NAME_SHIFT); m_idxCtrl = xkb_keymap_mod_get_index(km, XKB_MOD_NAME_CTRL); m_idxAlt = xkb_keymap_mod_get_index(km, XKB_MOD_NAME_ALT); m_idxSuper = xkb_keymap_mod_get_index(km, XKB_MOD_NAME_LOGO); - m_keymap = km; // pointer-identity comparison only; no ref taken if (t) xkb_state_update_mask(m_unshifted, 0, 0, 0, 0, 0, t->activeGroup()); } @@ -775,14 +780,17 @@ private: // and documents the ownership chain. if (m_query) xkb_state_unref(m_query); if (m_unshifted) xkb_state_unref(m_unshifted); + if (m_keymap) xkb_keymap_unref(m_keymap); if (m_fallbackKeymap) xkb_keymap_unref(m_fallbackKeymap); } XkbState(const XkbState &) = delete; XkbState &operator=(const XkbState &) = delete; - // Pointer-identity reference to the keymap our derived states were - // built from. NOT owned (the tracker or m_fallbackKeymap owns). + // The keymap our derived states were built from. We hold a ref + // here (taken in syncFromTracker, released on rebuild and in dtor) + // so the xkb allocator can't free + reuse the address while we + // still cache it as our identity. mutable struct xkb_keymap *m_keymap = nullptr; // Throwaway keymap from XKB defaults, built when the live keymap // hasn't arrived yet. Owned. Released in dtor; never replaced. diff --git a/qt/src/InspectorWindow.cpp b/qt/src/InspectorWindow.cpp index 10be6f161..d8d922f0b 100644 --- a/qt/src/InspectorWindow.cpp +++ b/qt/src/InspectorWindow.cpp @@ -11,6 +11,8 @@ #include #include #include +#include +#include #include #include #include @@ -55,7 +57,21 @@ InspectorWindow::InspectorWindow(ghostty_surface_t surface) QSettings s; const QByteArray geom = s.value(QStringLiteral("inspector/geometry")).toByteArray(); - if (!restoreGeometry(geom)) resize(800, 600); + if (!restoreGeometry(geom)) { + resize(800, 600); + } else { + // restoreGeometry happily restores positions on a screen that no + // longer exists (monitor unplugged, scaled away, etc.). Validate + // the centre point lands on a current screen and re-centre on + // the primary screen if not — otherwise the window is invisible + // and there's no UI to reach it. + if (!QGuiApplication::screenAt(frameGeometry().center())) { + if (QScreen *primary = QGuiApplication::primaryScreen()) { + const QRect g = primary->availableGeometry(); + move(g.center() - QPoint(width() / 2, height() / 2)); + } + } + } m_inspector = ghostty_surface_inspector(m_surface); diff --git a/qt/src/MainWindow.cpp b/qt/src/MainWindow.cpp index 9ea592d30..a0940602b 100644 --- a/qt/src/MainWindow.cpp +++ b/qt/src/MainWindow.cpp @@ -232,6 +232,63 @@ static bool configHasCustomShader() { return false; } +// Parse a libghostty duration string into nanoseconds. The format is +// concatenated `` segments per Config.zig's Duration.parseCLI: +// y w d h m s ms µs us ns +// Each segment is added to the total. Returns the supplied default if +// parsing fails or the input is empty. libghostty exposes Duration via +// ghostty_config_get as a non-extern non-packed struct, which c_get +// silently rejects; we fall back to scanning the config file text. +static uint64_t parseDurationNs(const QString &s, uint64_t fallback) { + if (s.isEmpty()) return fallback; + // Order matters: longer units first so `ms` is matched before `s`, + // `us` before `s`, etc. Mirrors Config.zig's units array. + static constexpr struct { const char *name; uint64_t factor; } kUnits[] = { + {"ns", 1ULL}, + {"us", 1000ULL}, + {"µs", 1000ULL}, + {"ms", 1000000ULL}, + {"s", 1000000000ULL}, + {"m", 60ULL * 1000000000ULL}, + {"h", 3600ULL * 1000000000ULL}, + {"d", 86400ULL * 1000000000ULL}, + {"w", 7ULL * 86400ULL * 1000000000ULL}, + {"y", 365ULL * 86400ULL * 1000000000ULL}, + }; + uint64_t total = 0; + int i = 0; + const int n = s.size(); + bool anyMatched = false; + while (i < n) { + while (i < n && s.at(i).isSpace()) ++i; + if (i >= n) break; + // Parse an integer. + int start = i; + while (i < n && s.at(i).isDigit()) ++i; + if (i == start) return fallback; // expected a number + bool ok = false; + const uint64_t value = s.mid(start, i - start).toULongLong(&ok); + if (!ok) return fallback; + while (i < n && s.at(i).isSpace()) ++i; + // Match the longest unit prefix at i. + const QString rest = s.mid(i); + uint64_t factor = 0; + int unitLen = 0; + for (const auto &u : kUnits) { + const int ulen = static_cast(qstrlen(u.name)); + if (rest.startsWith(QString::fromUtf8(u.name)) && ulen > unitLen) { + factor = u.factor; + unitLen = ulen; + } + } + if (unitLen == 0) return fallback; // no unit + total += value * factor; + i += unitLen; + anyMatched = true; + } + return anyMatched ? total : fallback; +} + // Scan the primary Ghostty config file for `key = value`, returning the // last matching value (empty if absent). For keys not cleanly exposed by // ghostty_config_get. @@ -394,9 +451,15 @@ bool MainWindow::initialize() { // covers the common (no-delay) case; a configured delay is honored // through the libghostty quit_timer action (see handleQuitTimer). const bool quitAfter = configBool("quit-after-last-window-closed", true); - unsigned long long delayNs = 0; - configGet(s_config, &delayNs, "quit-after-last-window-closed-delay"); - s_quitDelayMs = quitAfter ? static_cast(delayNs / 1000000ULL) : 0; + // quit-after-last-window-closed-delay is a `?Duration` and Duration + // is neither extern nor packed, so libghostty's ghostty_config_get + // returns false for it. Read from disk and parse. + const uint64_t delayNs = parseDurationNs( + configValue(QStringLiteral("quit-after-last-window-closed-delay")), 0); + const uint64_t delayMs = delayNs / 1000000ULL; + s_quitDelayMs = quitAfter + ? static_cast(std::min(delayMs, uint64_t(INT_MAX))) + : 0; QApplication::setQuitOnLastWindowClosed(quitAfter && s_quitDelayMs == 0); } @@ -460,10 +523,11 @@ MainWindow *MainWindow::newWindow(ghostty_surface_t parent) { // Window position: window-position-x/y are optional (?i16 in // Config.zig). configGet writes the value and returns true when the // optional is present. Both must be set to take effect (matching - // the Config.zig doc comment). On Wayland the compositor may - // ignore; X11 honors. If unset, fall back to a cascade offset from - // the previous window so Cmd+N spam doesn't pile every window at - // the same origin — macOS does this via NSWindow.cascadeTopLeft. + // the Config.zig doc comment). If unset, fall back to a cascade + // offset from the previous window so Cmd+N spam doesn't pile every + // window at the same origin — macOS does this via + // NSWindow.cascadeTopLeft. Wayland compositors typically ignore + // window placement requests; this is a hint at most. int16_t posX = 0, posY = 0; const bool haveX = configGet(s_config, &posX, "window-position-x"); const bool haveY = configGet(s_config, &posY, "window-position-y"); @@ -475,7 +539,21 @@ MainWindow *MainWindow::newWindow(ghostty_surface_t parent) { } } - w->show(); + // initial-window: when false the very first window is bootstrap + // only — built so libghostty's app + config exists, then closed + // immediately without ever being mapped. Skipping show() here + // (instead of show-then-close) keeps the daemon-mode startup + // flicker-free. After the bootstrap, `initial-window` is no + // longer load-bearing — every subsequent newWindow() shows. + static bool s_initialWindowConsumed = false; + bool wantsShow = true; + if (!s_initialWindowConsumed) { + s_initialWindowConsumed = true; + bool initialWindow = true; + configGet(s_config, &initialWindow, "initial-window"); + wantsShow = initialWindow; + } + if (wantsShow) w->show(); return w; } @@ -810,8 +888,11 @@ void MainWindow::frame() { // Rendering happens only here, so a flood of RENDER actions cannot // saturate the GUI thread — each surface renders at most once a frame. // One pass across every window: the shared ghostty_app_t was already - // ticked once above. - for (MainWindow *w : s_windows) + // ticked once above. Iterate a snapshot in case rendering triggers + // a window close (renderer-unhealthy chain, etc.) — QList iterators + // are invalidated by removeOne; copying is cheap (vector of pointers). + const QList windows = s_windows; + for (MainWindow *w : windows) for (GhosttySurface *s : w->m_surfaces) s->renderIfDirty(); } @@ -1028,26 +1109,15 @@ void MainWindow::setupLayerShell() { if (!handle) return; LayerShellQt::Window *ls = LayerShellQt::Window::get(handle); if (!ls) { - // Non-Wayland (XWayland / X11 / nested) or a compositor without - // the wlr-layer-shell protocol. Fall back to a regular always- - // on-top, undecorated, top-anchored window so the quick - // terminal still works as a dropdown — just without - // workspace-spanning behaviour. macOS gets a Cocoa window from - // the same code path; the quick terminal there is the - // equivalent fallback. - setWindowFlag(Qt::FramelessWindowHint, true); - setWindowFlag(Qt::WindowStaysOnTopHint, true); - setWindowFlag(Qt::Tool, true); // stay out of the taskbar - if (QScreen *screen = handle->screen()) { - const QSize scr = screen->size(); - // 60% width, 40% height, top-centered — close enough to the - // primary layer-shell layout (top-anchored full-width) without - // the protocol's fancier anchoring. - const QSize size(scr.width() * 6 / 10, scr.height() * 4 / 10); - const QRect g = screen->geometry(); - move(g.left() + (g.width() - size.width()) / 2, g.top()); - resize(size); - } + // The Qt frontend targets Wayland exclusively (the project + // builds against LayerShellQt for the dropdown). If we can't + // get a layer-shell handle the platform isn't supported — log + // and bail rather than silently degrading to a non-functional + // regular window. + std::fprintf(stderr, + "[ghastty] LayerShellQt::Window::get returned null; " + "the quick terminal needs a Wayland session with " + "wlr-layer-shell support (e.g. KWin, sway).\n"); return; } using LSW = LayerShellQt::Window; @@ -1180,8 +1250,11 @@ GhosttySurface *MainWindow::surfaceAt(int index) const { } int MainWindow::tabIndexForSurface(GhosttySurface *surface) const { - for (int i = 0; i < m_tabs->count(); ++i) - if (m_tabs->widget(i)->isAncestorOf(surface)) return i; + if (!surface) return -1; + for (int i = 0; i < m_tabs->count(); ++i) { + QWidget *page = m_tabs->widget(i); + if (page && page->isAncestorOf(surface)) return i; + } return -1; } @@ -1228,12 +1301,16 @@ void MainWindow::gotoSplit(GhosttySurface *from, // zoomed and the config asks to preserve zoom across navigation, // we'll re-zoom the destination once the focus moves. Otherwise // the existing semantics of dropping zoom on navigation apply. - // The struct is { navigation: bool, _padding: u7 }; configGet - // writes the bool into the first byte. - struct PreserveZoom { bool navigation; }; - PreserveZoom pz{false}; - configGet(s_config, &pz, "split-preserve-zoom"); - const bool preserveZoom = pz.navigation && m_zoomed == from; + // + // libghostty serializes packed structs into a c_uint bitfield via + // c_get.zig: `ptr.* = @intCast(@as(Backing, @bitCast(value)));`. + // SplitPreserveZoom = packed struct { navigation: bool } so bit 0 + // is `navigation`. Reading into a smaller C struct (sizeof + // `bool`==1) under-sized the buffer and corrupted adjacent stack; + // read into c_uint and mask the bits. + unsigned int pzBits = 0; + configGet(s_config, &pzBits, "split-preserve-zoom"); + const bool preserveZoom = (pzBits & 0x1) != 0 && m_zoomed == from; const auto centerOf = [](GhosttySurface *s) { return QRect(s->mapToGlobal(QPoint(0, 0)), s->size()).center(); @@ -1323,8 +1400,16 @@ void MainWindow::resizeSplit(GhosttySurface *from, rs.direction == GHOSTTY_RESIZE_SPLIT_DOWN; const int delta = grow ? rs.amount : -static_cast(rs.amount); const int other = idx == 0 ? 1 : idx - 1; - sizes[idx] = std::max(0, sizes[idx] + delta); - sizes[other] = std::max(0, sizes[other] - delta); + // Clamp the delta against both panes' minimum size (0) before + // applying so total area is conserved. Without this, a clamp on + // sizes[idx] would still subtract the unclamped delta from `other`, + // shrinking the total area QSplitter sees and forcing it to + // renormalise inconsistently. + int appliedDelta = delta; + if (sizes[idx] + appliedDelta < 0) appliedDelta = -sizes[idx]; + if (sizes[other] - appliedDelta < 0) appliedDelta = sizes[other]; + sizes[idx] += appliedDelta; + sizes[other] -= appliedDelta; splitter->setSizes(sizes); } @@ -1430,26 +1515,18 @@ void MainWindow::updateTabText(int tab) { } void MainWindow::playBellAudio() { - QString path = configValue(QStringLiteral("bell-audio-path")); - if (path.isEmpty()) return; - if (path.startsWith(QLatin1String("~/"))) - path = QDir::homePath() + path.mid(1); - - bool ok = false; - const double volume = - configValue(QStringLiteral("bell-audio-volume")).toDouble(&ok); - + if (m_bellAudioPath.isEmpty()) return; if (!m_bellPlayer) { m_bellAudio = new QAudioOutput(this); m_bellPlayer = new QMediaPlayer(this); m_bellPlayer->setAudioOutput(m_bellAudio); } - m_bellAudio->setVolume(ok ? volume : 0.5); + m_bellAudio->setVolume(m_bellAudioVolume); // Stop first so a back-to-back bell restarts the clip from the // beginning. Without this, calling play() on an already-playing // QMediaPlayer is a no-op and rapid bells get silently swallowed. m_bellPlayer->stop(); - m_bellPlayer->setSource(QUrl::fromLocalFile(path)); + m_bellPlayer->setSource(QUrl::fromLocalFile(m_bellAudioPath)); m_bellPlayer->play(); } @@ -1464,9 +1541,13 @@ void MainWindow::refreshChrome() { if (s_config) { bool quitAfter = true; configGet(s_config, &quitAfter, "quit-after-last-window-closed"); - unsigned long long delayNs = 0; - configGet(s_config, &delayNs, "quit-after-last-window-closed-delay"); - s_quitDelayMs = quitAfter ? static_cast(delayNs / 1000000ULL) : 0; + // Same Duration-decode workaround as initialize(). + const uint64_t delayNs = parseDurationNs( + configValue(QStringLiteral("quit-after-last-window-closed-delay")), 0); + const uint64_t delayMs = delayNs / 1000000ULL; + s_quitDelayMs = quitAfter + ? static_cast(std::min(delayMs, uint64_t(INT_MAX))) + : 0; QApplication::setQuitOnLastWindowClosed(quitAfter && s_quitDelayMs == 0); } @@ -1542,42 +1623,25 @@ void MainWindow::reloadConfigGlobal() { refreshChrome(); // app-notifications.config-reload: post a desktop notification so - // the user has a visible cue that the reload landed. The config is - // a packed-bool struct { clipboard-copy: bool, config-reload: bool }; - // configGet writes both bytes. Default is true for both. - // app-notifications.clipboard-copy is currently unused — the Qt - // frontend doesn't post a notification on terminal-driven copy - // operations. The bit is read for forward compatibility so a - // future copy-toast can be gated by the same config without - // touching this site. - struct AppNotifications { bool clipboardCopy; bool configReload; }; - AppNotifications n{true, true}; - configGet(s_config, &n, "app-notifications"); - if (n.configReload) + // the user has a visible cue that the reload landed. + // + // AppNotifications = packed struct { clipboard-copy: bool = true, + // config-reload: bool = true }. libghostty serializes packed + // structs as c_uint (see c_get.zig). Bit 0 = clipboard-copy, + // bit 1 = config-reload. The clipboard-copy bit is read for + // forward compatibility — Qt doesn't currently post a copy + // toast, but a future one will pick up the same gate. + unsigned int notifBits = 0; + const bool notifOk = configGet(s_config, ¬ifBits, "app-notifications"); + // configGet failure → defaults (both bits set) so the feature + // still works as documented. + if (!notifOk) notifBits = 0x3; + const bool wantConfigReload = (notifBits & 0x2) != 0; + if (wantConfigReload) postNotification(QStringLiteral("Ghostty"), QStringLiteral("Configuration reloaded.")); } -bool MainWindow::wantsInitialWindow() { - // s_config exists once the bootstrap window has called initialize(). - if (!s_config) return true; - bool wanted = true; - configGet(s_config, &wanted, "initial-window"); - return wanted; -} - -void MainWindow::closeInitialWindow() { - if (s_windows.isEmpty()) return; - // Close the bootstrap window without re-prompting; nothing has run - // in it yet so confirmCloseSurfaces would return true anyway, but - // m_skipCloseConfirm avoids any chrome flicker. closeAllWindows - // also resets the quit-on-last-window flag to keep the process - // alive until the user binds the quick-terminal shortcut. - MainWindow *first = s_windows.first(); - first->m_skipCloseConfirm = true; - first->close(); -} - QString MainWindow::configString(const char *key) const { const char *value = nullptr; if (!s_config || @@ -1735,8 +1799,23 @@ void MainWindow::undoLastClose() { if (s_undoStack.isEmpty()) return; const UndoEntry e = s_undoStack.takeLast(); + // The active window picks the new tab's parent surface for cwd + // inheritance. Skip the quick terminal — it doesn't push undo + // entries and isn't a meaningful target. Fall back to the most + // recent regular window in s_windows order. + auto isUndoTarget = [](MainWindow *w) { + return w && !w->m_quickTerminal; + }; MainWindow *active = qobject_cast(qApp->activeWindow()); - if (!active && !s_windows.isEmpty()) active = s_windows.first(); + if (!isUndoTarget(active)) { + active = nullptr; + for (int i = s_windows.size() - 1; i >= 0; --i) { + if (isUndoTarget(s_windows.at(i))) { + active = s_windows.at(i); + break; + } + } + } GhosttySurface *parent = active ? active->surfaceAt(active->m_tabs->currentIndex()) : nullptr; @@ -1782,34 +1861,30 @@ void MainWindow::undoLastClose() { // the revived widgets so we close the active window's current tab (or // the active window itself for a Window entry); pragmatic, matches // what a user normally means by "redo close-tab". +// +// Save the rest of s_redoStack before re-closing — pushTabUndo / +// pushWindowUndo unconditionally clear s_redoStack (a fresh close +// invalidates pending redos), and we want to preserve a chain of +// redos beyond the first. void MainWindow::redoLastClose() { if (s_redoStack.isEmpty()) return; const UndoEntry e = s_redoStack.takeLast(); + // Stash the remaining redo stack so we can restore it after the + // re-close (which clears s_redoStack via pushTabUndo / pushWindowUndo). + const QList savedRedo = s_redoStack; + MainWindow *active = qobject_cast(qApp->activeWindow()); if (!active && !s_windows.isEmpty()) active = s_windows.last(); if (!active) return; if (e.kind == UndoEntry::Kind::Tab) { const int idx = active->m_tabs->currentIndex(); - if (idx >= 0) { - // Push back onto the undo stack — closeTab won't, since we're - // doing it programmatically. - active->pushTabUndo(idx); - // pushTabUndo cleared s_redoStack; we just popped from it, so - // restore everything that was below `e` in the redo stack. - // (Simpler: keep the pre-clear contents.) Easiest fix is to - // not clear here — pushTabUndo always clears, so just rebuild. - // For our purposes, REDO chains are rare; accept the simpler - // semantics. - active->closeTab(idx); - } + if (idx >= 0) active->closeTab(idx); } else { - active->pushWindowUndo(); active->m_skipCloseConfirm = true; active->close(); } - // Note: a redo doesn't restore the redo stack; the user has to start - // a fresh close to fill it again. macOS UndoManager has the same - // semantics. + // Restore the rest of the redo chain so a sequence of REDOs works. + s_redoStack = savedRedo; } void MainWindow::applyWindowConfig() { @@ -1829,6 +1904,20 @@ void MainWindow::applyWindowConfig() { m_tabs->tabBar()->setVisible(m_tabs->count() > 1); } + // bell-audio-path / -volume: cached on the window so playBellAudio + // doesn't re-scan the on-disk config on every bell. Refreshed on + // each applyWindowConfig (i.e. at init and on reload). + { + QString path = configValue(QStringLiteral("bell-audio-path")); + if (path.startsWith(QLatin1String("~/"))) + path = QDir::homePath() + path.mid(1); + m_bellAudioPath = path; + bool volOk = false; + const double v = + configValue(QStringLiteral("bell-audio-volume")).toDouble(&volOk); + m_bellAudioVolume = volOk ? v : 0.5; + } + // window-title-font-family: apply to the tab bar (and the WM // title via Qt's window-title system font is harder to override // portably; the tab bar is what users actually look at). Empty / @@ -1856,56 +1945,29 @@ void MainWindow::applyWindowConfig() { // window-theme: force a light/dark scheme, or follow the OS. // // `auto` / `system` — follow the OS (Qt 6.8+ honours the platform - // colour scheme automatically; pre-6.8 we don't override). + // colour scheme automatically). // `dark` / `light` — force the explicit scheme. // `ghostty` — derive from the configured background colour's // luminance (Rec.601 weighting). // - // Qt 6.8 added QStyleHints::setColorScheme. Before that, the only - // portable knob is QPalette: derive a dark/light palette and apply - // it process-wide. We set the palette in both branches so a forced - // theme actually changes button / text colours, not just the - // colour-scheme hint that nothing in our chrome reads on its own. + // We require Qt 6.8+ (Debian trixie ships 6.8.2; the project's + // CMake doesn't compile against older Qt). The setColorScheme + // hint propagates to chrome (tab bar, dialogs); the terminal + // itself honours its own theme via libghostty. const QString theme = configString("window-theme"); - enum class Want { Follow, Dark, Light }; - Want want = Want::Follow; - if (theme == QLatin1String("dark")) want = Want::Dark; - else if (theme == QLatin1String("light")) want = Want::Light; - else if (theme == QLatin1String("ghostty")) { + Qt::ColorScheme scheme = Qt::ColorScheme::Unknown; + if (theme == QLatin1String("dark")) { + scheme = Qt::ColorScheme::Dark; + } else if (theme == QLatin1String("light")) { + scheme = Qt::ColorScheme::Light; + } else if (theme == QLatin1String("ghostty")) { ghostty_config_color_s bg{}; if (configGet(s_config, &bg, "background")) { const double luma = 0.299 * bg.r + 0.587 * bg.g + 0.114 * bg.b; - want = luma < 128.0 ? Want::Dark : Want::Light; + scheme = luma < 128.0 ? Qt::ColorScheme::Dark : Qt::ColorScheme::Light; } } - -#if QT_VERSION >= QT_VERSION_CHECK(6, 8, 0) - Qt::ColorScheme scheme = Qt::ColorScheme::Unknown; - if (want == Want::Dark) scheme = Qt::ColorScheme::Dark; - else if (want == Want::Light) scheme = Qt::ColorScheme::Light; QGuiApplication::styleHints()->setColorScheme(scheme); -#else - // Pre-6.8 fallback: synthesize a palette by hand. The forced - // light/dark palettes here approximate Qt 6.8's defaults closely - // enough for chrome (tab bar, dialogs); the terminal itself - // honours its own theme via libghostty. Want::Follow leaves the - // application palette untouched so the desktop's choice wins. - if (want != Want::Follow) { - QPalette p; - if (want == Want::Dark) { - p.setColor(QPalette::Window, QColor(0x2b, 0x2b, 0x2b)); - p.setColor(QPalette::WindowText, QColor(0xee, 0xee, 0xee)); - p.setColor(QPalette::Base, QColor(0x1e, 0x1e, 0x1e)); - p.setColor(QPalette::AlternateBase, QColor(0x33, 0x33, 0x33)); - p.setColor(QPalette::Text, QColor(0xee, 0xee, 0xee)); - p.setColor(QPalette::Button, QColor(0x2b, 0x2b, 0x2b)); - p.setColor(QPalette::ButtonText, QColor(0xee, 0xee, 0xee)); - p.setColor(QPalette::Highlight, QColor(0x2a, 0x82, 0xda)); - p.setColor(QPalette::HighlightedText, Qt::white); - } // else: a default-constructed QPalette is light-friendly. - QApplication::setPalette(p); - } -#endif } void MainWindow::toggleCommandPalette(GhosttySurface *surface) { @@ -2415,16 +2477,28 @@ bool MainWindow::onAction(ghostty_app_t, ghostty_target_s target, fire = true; } if (!fire) return; - // -after duration (ns); default 5s. - uint64_t afterNs = 5ULL * 1000 * 1000 * 1000; - configGet(s_config, &afterNs, "notify-on-command-finish-after"); + // -after Duration; default 5s. Duration isn't decodable via + // ghostty_config_get (non-extern non-packed struct), so parse + // from the on-disk config. + const uint64_t afterNs = parseDurationNs( + configValue(QStringLiteral("notify-on-command-finish-after")), + 5ULL * 1000 * 1000 * 1000); if (duration < afterNs) return; - // -action packed bools { bell, notify } — default bell=true. - struct NotifyAction { bool bell; bool notify; }; - NotifyAction act{true, false}; - configGet(s_config, &act, "notify-on-command-finish-action"); - if (act.bell) winp->ringBell(srcp); - if (act.notify || armed) { + // -action: NotifyOnCommandFinishAction = packed struct + // { bell: bool = true, notify: bool = false }. Serialized + // as c_uint via c_get.zig; bit 0 = bell, bit 1 = notify. + // A zero-init reads as no-bell-no-notify, which matches the + // "configGet failed; nothing to do" semantics. + unsigned int actBits = 0; + const bool actOk = + configGet(s_config, &actBits, "notify-on-command-finish-action"); + // configGet failure → fall back to the documented defaults + // (bell=true, notify=false) so the feature still works. + if (!actOk) actBits = 0x1; + const bool actBell = (actBits & 0x1) != 0; + const bool actNotify = (actBits & 0x2) != 0; + if (actBell) winp->ringBell(srcp); + if (actNotify || armed) { QString title; if (code < 0) title = QStringLiteral("Command Finished"); else if (code == 0) title = QStringLiteral("Command Succeeded"); diff --git a/qt/src/MainWindow.h b/qt/src/MainWindow.h index aee3e7903..d6493f6de 100644 --- a/qt/src/MainWindow.h +++ b/qt/src/MainWindow.h @@ -74,15 +74,6 @@ public: // The live libghostty config (for keybind lookups, etc.). ghostty_config_t config() const { return s_config; } - // initial-window config plumbing. The libghostty app+config is - // built by the first MainWindow::initialize, so main.cpp opens a - // bootstrap window and asks afterwards whether the user actually - // wanted one — closing it cleanly if not. Headless start-up is - // how a user runs ghastty as a daemon for the global quick- - // terminal shortcut. - static bool wantsInitialWindow(); - static void closeInitialWindow(); - // UNDO / REDO close-tab/window. The libghostty actions carry no // payload — the apprt is responsible for tracking what was closed // and reviving it. macOS uses NSUndoManager; we keep a small bounded @@ -330,8 +321,13 @@ private: int m_zoomIndex = 0; // Bell audio playback; created lazily on the first audio bell. + // The bell-audio-path / -volume values are cached at window setup + // and refreshed on reload so the bell hot path doesn't re-scan + // the on-disk config file. QMediaPlayer *m_bellPlayer = nullptr; QAudioOutput *m_bellAudio = nullptr; + QString m_bellAudioPath; // expanded; empty if no clip configured + double m_bellAudioVolume = 0.5; // The command palette; created lazily on first use. CommandPalette *m_commandPalette = nullptr; diff --git a/qt/src/main.cpp b/qt/src/main.cpp index 1a2456cc4..3f0fcb2fa 100644 --- a/qt/src/main.cpp +++ b/qt/src/main.cpp @@ -29,12 +29,14 @@ int main(int argc, char **argv) { QApplication app(argc, argv); - // QSettings storage path keys: applicationName + organizationDomain. + // QSettings storage path keys: applicationName + organizationName. // Used by the inspector window's geometry autosave (and any future // QSettings-backed UI state) — the keys go to - // ~/.config/ghastty/ghastty.conf. + // ~/.config/ghastty/ghastty.conf. We pass the same string to both + // because we don't run a multi-app suite under a parent + // organization. QCoreApplication::setApplicationName(QStringLiteral("ghastty")); - QCoreApplication::setOrganizationDomain(QStringLiteral("ghastty")); + QCoreApplication::setOrganizationName(QStringLiteral("ghastty")); // Match the installed ghastty.desktop: this becomes the Wayland app-id // (and X11 WM_CLASS), so the compositor associates the window with the @@ -61,22 +63,16 @@ int main(int argc, char **argv) { return 1; } - // initial-window: when false, start headless (no window opens at + // initial-window: when false, start headless (no window mapped at // launch). Combined with quit-after-last-window-closed=false this // is how a user runs ghastty as a daemon for the global quick- - // terminal shortcut. We need the libghostty app first, so spin up - // a temporary "config bootstrap" by opening + immediately closing - // a window — but cheaper: peek at the config directly here. - // ghostty_init has already run, but the libghostty app is built - // by the first MainWindow::initialize. There is no app-less - // accessor for the config, so we open the window and close if the - // bool is false. Cheaper alternative: set a static flag and have - // initialize() bail before show. + // terminal shortcut. The first MainWindow::newWindow internally + // checks the config and skips show() — so the libghostty app + + // config still get built, but no QWindow ever appears. if (!MainWindow::newWindow(nullptr)) { std::fprintf(stderr, "[ghastty] window initialization failed\n"); return 1; } - if (!MainWindow::wantsInitialWindow()) MainWindow::closeInitialWindow(); // Register global shortcuts via the XDG portal so the quick terminal // can be toggled while Ghostty is unfocused. Keys are assigned by the