From 48d81c9801b93f97ee599418fd112199f718eda7 Mon Sep 17 00:00:00 2001 From: Shangxin Date: Fri, 23 Oct 2026 21:14:53 +0800 Subject: [PATCH 1/2] feat(ui): float input suggestions in the work area's bottom-right corner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ui/src/placement.rs`, `ui/src/workarea.rs`, `ui/src/suggestion.rs` and `ui/suggestion.slint` were written but never declared in `ui/src/main.rs`, so none of it was compiled into the binary. A suggestion was always drawn on the resident panel's confirmation card, and nothing measured otherwise. Declaring the modules makes the feature reachable. What the working version needed beyond that is the four things below. **Placement against the work area, not the monitor.** A card inset from the screen edge sits under the taskbar: visible in a screenshot, unreachable in life. `workarea.rs` asks Win32 for `rcWork` through two hand-declared imports rather than taking `windows-sys` as a direct dependency. `placement.rs` keeps every coordinate conversion in one function — the first version scaled the card but not its inset, which put the card a few pixels off the corner at any DPI above 100%, and the smoke caught it by disagreeing with a panel that had already read the real work area. **The card does not take the keyboard.** Slint 1.18 exposes `always-on-top` and `no-frame` but no activation hint, and re-showing a window on Windows sets `SW_SHOW` on every show after the first (`window_state.rs` latches on `MARKER_ACTIVATE`). `keyboard.rs` adds `WS_EX_NOACTIVATE` and restores the previous foreground window after each show. The harness now measures this instead of trusting the comment: `ui:smoke` records who holds the keyboard before and after the card appears. **Closing the card sets it aside; it does not answer.** A frameless card has no close button, but Alt+F4 still asks. Routed through the same path as `Later`, so the panel keeps a notice offering to bring the prompt back rather than leaving it believing the card is still on screen. The same rule governs closing the panel: it is "put away", and the engine's own deadline is the only thing that decides an unanswered prompt. **Where there is no corner, the panel asks.** `workarea.rs` returns `None` off Windows — the whole screen would be a guess that no desktop has panels, true on a bare Xvfb and false on every real Linux desktop, where the card would open under the dash. A screen too small for the 360px card returns `None` too. Both land the prompt on the panel's card rather than nowhere, and the smoke asserts that branch rather than skipping it. The smoke had never sent a suggestion: it only ever sent `kind: "run_tests"`. Adding the case required a streaming `push` on the fixture and three host capabilities — `findWindow` (answers "must not be there" without waiting), `workArea`, and an optional `foregroundWindowTitle` that a host may decline. The corner assertion deliberately checks containment and nearness rather than one exact coordinate: the first version restated the panel's `CORNER_INSET` as a bare 16 physical pixels while the panel correctly scales it, and a harness that called correct placement wrong would have taught the next person to loosen it. Exact arithmetic stays in `placement.rs`, where it tests without a display. Also fixes a flaky assertion in `tests/screen-text-reader-streaming.test.ts`: it asserted an exact child count after a fixed 30ms sleep, but the loop correctly restarts a child that stalls again, so both 1 and 7 are reachable and neither is a property of the loop. It now polls for the condition and asserts "killed, said why, replaced". Verified: `cargo clippy --all-targets` clean, `cargo test` 104 passed, `bun test` 826 passed / 23 skipped / 0 failed, `bun run typecheck` clean, and `bun run ui:smoke` green on a 2560x1380 work area at 125% — the card lands at its corner and the keyboard stays with the window that had it. --- docs/panel-protocol.md | 20 ++ src/ui-smoke-host-windows.ts | 121 +++++-- src/ui-smoke-host-x11.ts | 65 ++-- src/ui-smoke-host.ts | 49 +++ src/ui-smoke.ts | 145 +++++++++ tests/screen-text-reader-streaming.test.ts | 35 +- ui/panel.slint | 47 ++- ui/src/keyboard.rs | 227 +++++++++++++ ui/src/main.rs | 323 ++++++++++++++---- ui/src/placement.rs | 359 ++++++++++++++++++++ ui/src/suggestion.rs | 361 +++++++++++++++++++++ ui/src/workarea.rs | 149 +++++++++ ui/suggestion.slint | 151 +++++++++ ui/theme.slint | 12 + 14 files changed, 1953 insertions(+), 111 deletions(-) create mode 100644 ui/src/keyboard.rs create mode 100644 ui/src/placement.rs create mode 100644 ui/src/suggestion.rs create mode 100644 ui/src/workarea.rs create mode 100644 ui/suggestion.slint diff --git a/docs/panel-protocol.md b/docs/panel-protocol.md index ff06a5f..63e575a 100644 --- a/docs/panel-protocol.md +++ b/docs/panel-protocol.md @@ -454,6 +454,26 @@ without redesigning the transport. `promptId` is an integer. There is **no** `arguments` field and never will be: the selection must not be able to reach this process at all. +`kind` is the sender's, and the wire does not enumerate it — but the panel reads +one value out of it, and that is a rule rather than a detail. A prompt whose +`kind` is `input_suggestion` (or `input_suggestion.` followed by anything) +leaves the resident panel and is asked on a frameless, always-on-top card in the +bottom-right corner of the work area, inset 16 logical pixels. Every other kind +stays on the panel's own confirmation card. The reason is the one thing a +suggestion has that a decision does not: it names text for the field somebody is +typing in *right now*, so it is worthless anywhere they have to go and look. + +Two consequences the sender can rely on: + +- **The same three answers, from either surface.** `approved`, `declined`, and + setting it aside all use this wire unchanged. Setting a suggestion aside leaves + the request live and puts a standing notice on the panel; it does not answer. +- **When the card cannot be placed, the panel asks.** A desktop with no work + area — off Windows there is none, and a headless session has none either — or a + screen too small for a 360-pixel card gets the prompt on the panel instead. A + suggestion is never silently dropped for want of a corner, and a sender never + has to know which surface answered. + `why` and `denyConsequence` exist because a dialog that states only what will happen makes approval the cheaper answer: the user can see the command but not what refusing costs, and "just approve it" is what that asymmetry teaches. Both diff --git a/src/ui-smoke-host-windows.ts b/src/ui-smoke-host-windows.ts index d43cee5..a17f2f1 100644 --- a/src/ui-smoke-host-windows.ts +++ b/src/ui-smoke-host-windows.ts @@ -258,6 +258,61 @@ for ($i = 0; $i -lt $lines.Count; $i++) { `; } +/** + * The title of the window that currently has the keyboard. + * + * The measurement `ui/src/keyboard.rs` exists for. Its module comment claimed + * `WS_EX_NOACTIVATE` keeps the card from taking focus; a comment cannot fail, so + * this is what turns the claim into something a run can contradict. Empty output + * means no window has a caption, which the caller reads as "cannot tell" rather + * than "nothing has focus". + */ +function foregroundScript(): string { + return ` +Add-Type -MemberDefinition @' +[System.Runtime.InteropServices.DllImport("user32.dll")] public static extern System.IntPtr GetForegroundWindow(); +[System.Runtime.InteropServices.DllImport("user32.dll", CharSet=CharSet.Unicode)] public static extern int GetWindowTextW(System.IntPtr h, System.Text.StringBuilder s, int n); +'@ -Name Fg -Namespace SquidSmoke +$handle = [SquidSmoke.Fg]::GetForegroundWindow() +if ($handle -eq [System.IntPtr]::Zero) { exit 0 } +$text = New-Object System.Text.StringBuilder 512 +[void][SquidSmoke.Fg]::GetWindowTextW($handle, $text, 512) +Write-Output $text.ToString() +`; +} + +/** + * The work area of the monitor holding the given window, in physical pixels. + * + * `rcWork` and not `rcMonitor`: the monitor is the screen, and the work area is + * the screen minus the taskbar and anything else the desktop reserved. The + * distinction is the whole point of the assertion this feeds — a card placed + * against the monitor's bottom edge sits under the taskbar, visible in a + * screenshot and unreachable in life. + * + * The window's own monitor rather than the primary one, because the card is + * placed relative to wherever the person is working. + */ +function workAreaScript(handle: string): string { + return `${PREAMBLE} +Add-Type -MemberDefinition @' +[System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential)] +public struct RECT { public int Left; public int Top; public int Right; public int Bottom; } +[System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential)] +public struct MONITORINFO { public int cbSize; public RECT rcMonitor; public RECT rcWork; public uint dwFlags; } +[System.Runtime.InteropServices.DllImport("user32.dll")] public static extern System.IntPtr MonitorFromWindow(System.IntPtr h, uint flags); +[System.Runtime.InteropServices.DllImport("user32.dll")] public static extern bool GetMonitorInfoW(System.IntPtr monitor, ref MONITORINFO info); +'@ -Name Mon -Namespace SquidSmoke +$info = New-Object SquidSmoke.Mon+MONITORINFO +$info.cbSize = [System.Runtime.InteropServices.Marshal]::SizeOf($info) +$monitor = [SquidSmoke.Mon]::MonitorFromWindow([System.IntPtr]${handle}, 2) +if ($monitor -eq [System.IntPtr]::Zero) { exit 0 } +if (-not [SquidSmoke.Mon]::GetMonitorInfoW($monitor, [ref]$info)) { exit 0 } +Write-Output ('{0} {1} {2} {3}' -f $info.rcWork.Left, $info.rcWork.Top, + ($info.rcWork.Right - $info.rcWork.Left), ($info.rcWork.Bottom - $info.rcWork.Top)) +`; +} + /** * Windows PowerShell, by absolute path rather than by name — the one resolver * every PowerShell spawn in this repository answers to. The reasoning behind it @@ -275,6 +330,33 @@ async function powershell(script: string, description: string): Promise return result.stdout.trim(); } +/** + * The panel's window by title, or `null` when there is none on screen. + * + * Module-level rather than a method so that `awaitWindow` can be the polling + * version of the same lookup instead of a second copy of it: two copies drift, + * and the one that drifts is the one that stops finding the window. + */ +async function findWindow(title: string, processId: number): Promise { + const line = await powershell( + windowScript(title, processId), + "could not search for the panel window", + ); + if (line.length === 0) return null; + const parts = line.split(/\s+/); + if (parts.length !== 5) return null; + const [id, x, y, width, height] = parts.map((part) => part.trim()); + const numbers = [x, y, width, height].map(Number); + if (numbers.some((value) => !Number.isFinite(value))) return null; + return { + id: id ?? "", + x: numbers[0] ?? 0, + y: numbers[1] ?? 0, + width: numbers[2] ?? 0, + height: numbers[3] ?? 0, + }; +} + export function createWindowsSmokeHost(): SmokeHost { return { kind: "windows", @@ -315,26 +397,27 @@ export function createWindowsSmokeHost(): SmokeHost { */ panelEnvironment: () => ({}), + findWindow, + awaitWindow: (title: string, processId: number) => - eventually("a visible Squid window", async () => { - const line = await powershell( - windowScript(title, processId), - "could not search for the panel window", - ); - if (line.length === 0) return null; - const parts = line.split(/\s+/); - if (parts.length !== 5) return null; - const [id, x, y, width, height] = parts.map((part) => part.trim()); - const numbers = [x, y, width, height].map(Number); - if (numbers.some((value) => !Number.isFinite(value))) return null; - return { - id: id ?? "", - x: numbers[0] ?? 0, - y: numbers[1] ?? 0, - width: numbers[2] ?? 0, - height: numbers[3] ?? 0, - }; - }), + eventually("a visible Squid window", () => findWindow(title, processId)), + + workArea: async (window: SmokeWindow) => { + const line = await powershell(workAreaScript(window.id), "could not read the work area"); + if (line.length === 0) return null; + const numbers = line.split(/\s+/).map(Number); + if (numbers.length !== 4 || numbers.some((value) => !Number.isFinite(value))) return null; + const [x, y, width, height] = numbers as [number, number, number, number]; + return { x, y, width, height }; + }, + + foregroundWindowTitle: async () => { + const title = await powershell(foregroundScript(), "could not read the foreground window"); + // An empty title is a real state — a window with no caption can hold the + // keyboard — but it is not one this assertion can make a claim about, so + // it reports "cannot tell" rather than "nothing has focus". + return title.length === 0 ? null : title; + }, activate: async (window: SmokeWindow, control: SmokeControl) => { const name = CONTROL_NAME[control]; diff --git a/src/ui-smoke-host-x11.ts b/src/ui-smoke-host-x11.ts index 39973f8..4aacedc 100644 --- a/src/ui-smoke-host-x11.ts +++ b/src/ui-smoke-host-x11.ts @@ -182,6 +182,35 @@ export function createX11SmokeHost(): SmokeHost { return { ...process.env, DBUS_SESSION_BUS_ADDRESS: busAddress }; }; + /** + * A mapped window of this display by exact title, or `null` when there is none. + * + * `IsViewable` and not merely "found": a window this panel puts away is + * unmapped rather than destroyed, so the suggestion card exists on the root + * window's list of children while being invisible. Visibility is what the + * person can see, and visibility is what gets asserted. + */ + const findByTitle = async (title: string): Promise => { + const env = displayEnvironment(); + const found = await run("xdotool", ["search", "--name", `^${title}$`], env); + for (const id of found.stdout.trim().split(/\s+/)) { + if (id.length === 0) continue; + const info = await run("xwininfo", ["-id", id], env); + if (info.code !== 0 || !info.stdout.includes("IsViewable")) continue; + const read = (label: string): number | null => { + const match = info.stdout.match(new RegExp(`${label}:\\s+(-?\\d+)`)); + return match?.[1] === undefined ? null : Number(match[1]); + }; + const x = read("Absolute upper-left X"); + const y = read("Absolute upper-left Y"); + const width = read("Width"); + const height = read("Height"); + if (x === null || y === null || width === null || height === null) continue; + return { id, x, y, width, height }; + } + return null; + }; + // The last line of defence against the run that hung: if the process is // leaving through any path that skips dispose — an uncaught throw, a signal — // the two detached groups would otherwise outlive it, holding the pipes and @@ -274,27 +303,23 @@ export function createX11SmokeHost(): SmokeHost { * mean reading `_NET_WM_PID` through another tool on the prerequisite list to * rule out a window that cannot be there. */ + findWindow: (title: string, _processId: number) => findByTitle(title), + + /** + * `null`, and that is the panel's own answer on this platform rather than a + * gap in the harness. + * + * `ui/src/workarea.rs` returns `None` off Windows, on purpose: the whole + * screen would be a guess that a desktop has no panels — true on a bare Xvfb, + * false on every real Linux desktop, where the card would open under the GNOME + * dash and be unreachable. `None` makes the panel fall back to its own card, + * so what the smoke asserts here is that a suggestion still has somewhere to + * be asked — just not on the floating surface. + */ + workArea: async () => null, + awaitWindow: (title: string, _processId: number) => - eventually("a mapped Squid window", async () => { - const env = displayEnvironment(); - const found = await run("xdotool", ["search", "--name", `^${title}$`], env); - for (const id of found.stdout.trim().split(/\s+/)) { - if (id.length === 0) continue; - const info = await run("xwininfo", ["-id", id], env); - if (info.code !== 0 || !info.stdout.includes("IsViewable")) continue; - const read = (label: string): number | null => { - const match = info.stdout.match(new RegExp(`${label}:\\s+(-?\\d+)`)); - return match?.[1] === undefined ? null : Number(match[1]); - }; - const x = read("Absolute upper-left X"); - const y = read("Absolute upper-left Y"); - const width = read("Width"); - const height = read("Height"); - if (x === null || y === null || width === null || height === null) continue; - return { id, x, y, width, height }; - } - return null; - }), + eventually("a mapped Squid window", () => findByTitle(title)), activate: async (window: SmokeWindow, control: SmokeControl) => { const at = CONTROL_POSITION[control]; diff --git a/src/ui-smoke-host.ts b/src/ui-smoke-host.ts index 1107254..6c2eb57 100644 --- a/src/ui-smoke-host.ts +++ b/src/ui-smoke-host.ts @@ -24,6 +24,14 @@ import { spawn } from "node:child_process"; /** The controls the smoke presses, named for intent rather than position. */ export type SmokeControl = "decline" | "later" | "allow" | "review"; +/** A rectangle on screen, in whatever pixels the host measures in. */ +export interface SmokeRect { + readonly x: number; + readonly y: number; + readonly width: number; + readonly height: number; +} + /** A window on screen, in whatever pixels the host measures in. */ export interface SmokeWindow { /** The host's own handle for it, for logging and for follow-up calls. */ @@ -61,6 +69,47 @@ export interface SmokeHost { * private Xvfb no such window can exist, so that host may honestly ignore it. */ awaitWindow(title: string, processId: number): Promise; + /** + * A window of this panel by title, if it exists and is visible right now. + * + * Distinct from `awaitWindow`, which waits for one that must appear. This one + * answers for a window that must *not* be there — the suggestion card before + * any suggestion, and after an answer — and a wait would turn that assertion + * into a two-minute stall followed by a pass for the wrong reason. + * + * Any other window of the process does not count, and neither does a window + * that exists but is hidden: `hide` is how this panel puts the card away, and + * the card is still a live window afterwards. Visibility is the property the + * person can see, so it is the one asserted. + */ + findWindow(title: string, processId: number): Promise; + /** + * The work area of the monitor `window` is on — the screen minus the taskbar, + * the docks, and whatever else the desktop reserves — or `null` when this + * platform cannot be asked. + * + * This is the same question `ui/src/workarea.rs` asks, answered by the same + * platform call, and that is why it belongs here rather than being computed by + * the smoke from a screenshot: a harness that measured the screen itself would + * agree with a card placed against the screen edge, which is the defect. `null` + * is a real answer — X11 has no implementation in the panel either — and the + * smoke asserts the documented consequence of it rather than skipping. + */ + workArea(window: SmokeWindow): Promise; + /** + * The title of the window that currently has the keyboard, or `null` when this + * host cannot tell. + * + * A capability rather than a requirement, like `readStatus`: the panel's + * floating card is only on some platforms, and only some of those can be asked + * who holds the keyboard. A host that omits it says so, and the smoke skips the + * assertion out loud instead of pretending to make it. + * + * This is what makes "the card does not steal focus" a measurement rather than + * a claim. It is the whole reason `ui/src/keyboard.rs` exists, and a comment in + * that file was the only thing asserting it before. + */ + foregroundWindowTitle?(): Promise; activate(window: SmokeWindow, control: SmokeControl): Promise; screenshot(window: SmokeWindow, path: string): Promise; /** diff --git a/src/ui-smoke.ts b/src/ui-smoke.ts index 08ecd6a..0df87e7 100644 --- a/src/ui-smoke.ts +++ b/src/ui-smoke.ts @@ -26,8 +26,24 @@ import { } from "./ui-smoke-host.js"; const WINDOW_TITLE = "Squid"; +/** + * The floating suggestion card's own title, exactly as `suggestion.slint` spells + * it. + * + * A second title rather than the panel's, because the point of the card is that + * it is a *different window* — one that can appear while the panel is hidden. + * The panel's title would make the two indistinguishable to the one lookup the + * host offers. + */ +const SUGGESTION_TITLE = "Squid suggestion"; /** The prompt the fixture raises, and the only id an answer may name. */ const PROMPT_ID = 41; +/** + * The suggestion's id, kept apart from `PROMPT_ID` so that the two assertions — + * a decision is answered on the panel, a suggestion is offered on the card — + * cannot be satisfied by one another. + */ +const SUGGESTION_ID = 42; /** How long to wait for an answer that must NOT arrive. Short: absence is the assertion. */ const NEGATIVE_WAIT_MS = 700; @@ -45,6 +61,20 @@ interface Fixture { readonly endpoint: string; readonly replies: readonly PanelReply[]; readonly close: () => Promise; + /** + * Sends one more line to the panel's reader connection. + * + * The wire is a stream, not a snapshot: a prompt can arrive at any moment, and + * the floating card is only interesting when it arrives *after* the panel has + * been up for a while — which is the case the first version of this harness + * could not express at all, because everything it said was said in `announce`. + * + * Throws when nothing has subscribed yet, which is the only way this can be + * called wrongly: a line written to a connection the panel never opened is a + * line nobody reads, and that failure would otherwise surface as a timeout on + * a later assertion that names the wrong cause. + */ + push(message: Record): void; waitFor( description: string, predicate: (reply: PanelReply) => boolean, @@ -229,6 +259,12 @@ function panelFixture(endpoint: string): Promise { // the fixture and the server cannot disagree about what this endpoint is. if (!isPanelNamedPipe(endpoint)) await rm(endpoint, { force: true }); }, + push: (message) => { + if (reader === undefined) { + smokeFailure("a line was written before the panel subscribed; nothing would have read it"); + } + send(reader, message); + }, waitFor: (description, predicate, timeoutMs = INTERACTION_TIMEOUT_MS) => eventually(description, async () => replies.find(predicate) ?? null, timeoutMs), }); @@ -384,6 +420,115 @@ async function main(): Promise { } } + // The floating suggestion card. This is the assertion the harness did not + // have, and its absence is why the feature shipped doing nothing: three Rust + // modules and a `.slint` file existed, were not declared in `main.rs`, and no + // run ever asked whether a suggestion got a window of its own. + // + // The card is offered only where the panel can place it. `workarea.rs` + // returns `None` off Windows on purpose, so on X11 the documented answer is + // that the suggestion stays on the panel's card — a suggestion somebody can + // find beats one placed under a dock nobody can reach. Whichever platform + // this is, the prompt must end up somewhere visible, and both branches assert + // that rather than skipping. + const workArea = await host.workArea(window); + const foregroundBefore = await host.foregroundWindowTitle?.(); + fixture.push({ + type: "prompt", + promptId: SUGGESTION_ID, + kind: "input_suggestion", + summary: 'Type: "const answer = 42"', + why: "You were about to write the same expression.", + denyConsequence: "Text will not be typed into the active field.", + expiresAt: "2026-09-01T12:05:00.000Z", + }); + + if (workArea === null) { + // No placement available, so no floating window: the prompt must be on the + // panel's card instead. The panel is the surface a suggestion falls back to. + await wait(INTERACTION_TIMEOUT_MS); + const floating = await host.findWindow(SUGGESTION_TITLE, panel.pid ?? 0); + if (floating !== null) { + smokeFailure( + `a suggestion card appeared at ${floating.x},${floating.y} on a host that reported no work area; a card placed by a guess is a card that may be unreachable`, + ); + } + log("ui:smoke suggestion: no work area on this host, so the panel's own card asks it"); + } else { + const card = await host.awaitWindow(SUGGESTION_TITLE, panel.pid ?? 0); + // The corner, computed from the work area the host reported — the same + // rectangle the panel read, in the same units. A card against the monitor's + // edge instead of the work area's is the defect this catches. + // + // The assertions below are deliberately about *containment and nearness* + // rather than about one exact coordinate, and the first version of this + // proves why: it restated the panel's `CORNER_INSET` as a bare 16 physical + // pixels, while the panel correctly scales that constant (16 logical is 20 + // physical at 125%). The harness then called a correct placement wrong. + // + // Copying the constant in would not fix that — it would only make the two + // copies drift. The exact arithmetic is already tested, in + // `ui/src/placement.rs`, where it can be tested without a display. What the + // smoke uniquely knows is that the real window landed in the real corner of + // the real work area on this real desktop, and that is what it asserts. + const right = workArea.x + workArea.width; + const bottom = workArea.y + workArea.height; + const actualRight = card.x + card.width; + const actualBottom = card.y + card.height; + + // Off the work area is off the screen as far as the person is concerned: + // the bottom edge of the work area is where the taskbar starts. A card that + // overhangs it is visible in a screenshot and unreachable in life, which is + // the defect that put the work area in this design at all. + if (actualRight > right) { + smokeFailure( + `the suggestion card's right edge is at ${actualRight}, past the work area's ${right}; it was placed against the screen rather than the usable area`, + ); + } + if (actualBottom > bottom) { + smokeFailure( + `the suggestion card's bottom edge is at ${actualBottom}, past the work area's ${bottom}; it is under the taskbar`, + ); + } + + // Nearness, so a card parked in the middle of the screen does not pass the + // containment check above. A tenth of the area is loose enough to absorb + // every DPI and shadow choice and tight enough to fail anything that is not + // in the corner. + const slack = Math.round(Math.min(workArea.width, workArea.height) / 10); + if (right - actualRight > slack) { + smokeFailure( + `the suggestion card's right edge is ${right - actualRight}px from the work area's edge (max ${slack}); it is not in the corner`, + ); + } + if (bottom - actualBottom > slack) { + smokeFailure( + `the suggestion card's bottom edge is ${bottom - actualBottom}px from the work area's edge (max ${slack}); it is not in the corner`, + ); + } + log(`ui:smoke suggestion card: ${card.width}x${card.height}+${card.x}+${card.y} in the corner of ${workArea.width}x${workArea.height}`); + + // The card must not have taken the keyboard. Asserted only where the host + // can say who holds it, and compared against the holder *before* the card + // appeared — the measuring side of the module comment in `keyboard.rs`, which + // until now was the only thing making that claim. + const foregroundAfter = await host.foregroundWindowTitle?.(); + if (foregroundBefore !== undefined && foregroundAfter !== undefined) { + if (foregroundAfter === SUGGESTION_TITLE) { + smokeFailure( + "the suggestion card took the keyboard; a suggestion that interrupts typing is worse than no suggestion", + ); + } + log(`ui:smoke suggestion kept the keyboard on "${foregroundAfter ?? ""}" (was "${foregroundBefore ?? ""}")`); + } else { + log("ui:smoke skipped the focus assertion: this host cannot report which window has the keyboard"); + } + + const cardScreenshot = join(outputDirectory, "04-suggestion-card.png"); + await host.screenshot(card, cardScreenshot); + log(`ui:smoke screenshot: ${cardScreenshot}`); + } + // One prompt, one answer. Clicking where the buttons were must not produce a // second line for the same id. const before = fixture.replies.filter((reply) => reply.type === "answer").length; diff --git a/tests/screen-text-reader-streaming.test.ts b/tests/screen-text-reader-streaming.test.ts index dc30f6a..6915ebe 100644 --- a/tests/screen-text-reader-streaming.test.ts +++ b/tests/screen-text-reader-streaming.test.ts @@ -75,6 +75,21 @@ function harness(restartMs = 5, stallMs?: number) { const settle = (): Promise => new Promise((done) => setTimeout(done, 30)); +/** + * Polls until `probe` holds, or gives up. + * + * Polling rather than one `settle` for anything that waits on a chain of timers: + * the platform's `setTimeout` granularity is coarser than its nominal argument — + * around fifteen milliseconds on Windows — so a stall deadline plus a restart + * deadline can land anywhere in a wide band, and a fixed sleep is a race with the + * clock rather than a test of the loop. + */ +async function waitFor(probe: () => boolean, timeoutMs = 2_000): Promise { + for (let waited = 0; waited < timeoutMs && !probe(); waited += 5) { + await new Promise((done) => setTimeout(done, 5)); + } +} + describe("streamingScreenTextReaderFactory", () => { it("spawns one child and turns each line into a frame", () => { const { children, frames, reader } = harness(); @@ -182,15 +197,29 @@ describe("streamingScreenTextReaderFactory", () => { // interval whether or not anything changed, so silence is a sampler that // stopped sampling rather than a quiet desktop. Nothing restarts it, because // the process is still there. + // + // The child's stall deadline is 20ms and the restart deadline 5ms, so a fixed + // sleep cannot be used to assert an exact total. It could not before either: + // this asserted `children.length === 2` after 30ms, which held only when the + // two platform timers happened to fire inside that window. On a host whose + // timers round up to the system tick the kill lands at ~30ms and the spawn at + // ~45ms, so the old assertion read 1 — and given a longer window it reads 7, + // because this loop correctly restarts a child that stalls *again*. Neither + // number is a property of the loop; "killed, said so, replaced" is. const { children, frames, errors, reader } = harness(5, 20); children[0]?.emit("before\n"); - await settle(); + await waitFor(() => children.length >= 2); expect(children[0]?.killed()).toBe(true); expect(errors.some((message) => message.includes("wrote no record for 20ms"))).toBe(true); - expect(children.length).toBe(2); - children[1]?.emit("after\n"); + expect(children.length).toBeGreaterThanOrEqual(2); + + // The newest child, whichever the loop is on: a child that stalls again is + // restarted again, so "the second one" is not a stable thing to address. + // Emitting is synchronous, so the frame is captured whichever child this is. + const current = children[children.length - 1]; + current?.emit("after\n"); expect(frames.map((frame) => frame.applicationId)).toEqual(["before", "after"]); reader.close(); }); diff --git a/ui/panel.slint b/ui/panel.slint index bacd036..8225b45 100644 --- a/ui/panel.slint +++ b/ui/panel.slint @@ -15,6 +15,12 @@ export { SourceRow, SettingRow, LlmLogRow } // component, not an element inside the window — the icon has no window of its own. export { SquidTray } from "tray.slint"; +// Same re-export rule, second top-level window. The floating suggestion surface +// is a window of its own for the reason `suggestion.slint` records: a prompt +// about the field somebody is typing in is only useful where their attention +// already is, and this window is how it gets there. +export { SuggestionWindow } from "suggestion.slint"; + // A section heading. More space above than below, so a heading belongs to what // follows it rather than floating between two groups. component Heading inherits Text { @@ -150,6 +156,14 @@ export component PanelWindow inherits Window { in property prompt-expires-at; in property prompt-why; in property prompt-deny-consequence; + // The floating suggestion window is showing this prompt instead of the card + // below. Rust sets it with the other `prompt-*` inputs because it is the + // same fact — where this prompt is being asked — and it is Rust that knows + // whether this run can place a window at a corner at all (see + // `src/placement.rs`). While it is set the card and the standing notice both + // stay out of the window: the header's "Waiting for you" is the panel's whole + // record of a decision that is being asked elsewhere. + in property prompt-floating; callback approve(); callback decline(); @@ -159,11 +173,14 @@ export component PanelWindow inherits Window { // group opened here without them would render an empty door. in-out property open-group; callback select-group(string); - // A decision the person set aside. Local to the window on purpose — the - // request is still live on the brain's side, and the engine's own deadline - // still decides it. Deferring changes what this surface shows, never what - // was answered. - property deferred: false; + // A decision the person set aside. Rust owns the value — the same prompt can + // also be set aside from the floating suggestion window, and the two + // surfaces must agree about whether it is still showing without either + // holding a second copy of the flag. The request stays live on the brain's + // side either way: deferring changes what is shown, never what was answered. + in property deferred: false; + callback defer(); + callback reopen(); // Counted in Rust rather than tracked here: the session refuses to stop the // last running observer, so each card needs to know whether it is that one. @@ -236,20 +253,28 @@ export component PanelWindow inherits Window { // region, so it cannot be pushed off the bottom by a long settings list. // This is the defect a real smoke run caught: the confirmation used to // sit last inside the scroll view, below twenty-two settings rows, while - // the header cheerfully said "Squid is ready". - if root.prompt-pending && !root.deferred: ConfirmationCard { + // the header cheerfully said "Squid is ready". An input suggestion is the + // one exception, and it is not rendered here at all — the floating + // suggestion window is asking it, and a second copy of the same decision + // with its own Allow button would be a second chance to answer a question + // the other surface has already taken. + if root.prompt-pending && !root.deferred && !root.prompt-floating: ConfirmationCard { kind: root.prompt-kind; summary: root.prompt-summary; expires-at: root.prompt-expires-at; why: root.prompt-why; deny-consequence: root.prompt-deny-consequence; - allow => { root.deferred = false; root.approve(); } - decline => { root.deferred = false; root.decline(); } - defer => { root.deferred = true; } + allow => { root.approve(); } + decline => { root.decline(); } + defer => { root.defer(); } } + // The standing notice stays even when the floating window is the surface + // that was set aside: the panel is where a person comes back to, and + // "a decision is still waiting" is the one sentence that must survive + // the suggestion window closing. if root.prompt-pending && root.deferred: DeferredNotice { - reopen => { root.deferred = false; } + reopen => { root.reopen(); } } sv := ScrollView { diff --git a/ui/src/keyboard.rs b/ui/src/keyboard.rs new file mode 100644 index 0000000..1ecbafe --- /dev/null +++ b/ui/src/keyboard.rs @@ -0,0 +1,227 @@ +//! Keeps the floating suggestion card from taking the keyboard. +//! +//! A suggestion arrives while somebody is typing — that is the only moment it is +//! useful — so a window that takes focus to show it interrupts the very act it is +//! offering to help with. Measured on the development box: without this, the card +//! becomes the foreground window on every show, first and subsequent alike. +//! +//! Two things are needed, and the first measurement is why there are two. +//! +//! **The style bit, so the card is not the kind of window that takes focus.** +//! `WS_EX_NOACTIVATE` is a property of the *window* rather than of the call that +//! shows it, which is what makes it the right mechanism where +//! `ShowWindow(SW_SHOWNOACTIVATE)` is not: winit uses that flag only for a +//! window's first show and `SW_SHOW` — which activates — on every later one +//! (`winit-0.30.13/platform_impl/windows/window_state.rs`, `TextState::apply_diff` +//! and its `MARKER_ACTIVATE` latch). It is also what keeps a click on the card's +//! own buttons from pulling the keyboard away from the field they are about to +//! type into. +//! +//! **Giving the keyboard back after the show that created the window.** Measured: +//! the style alone is not enough, because it can only be applied to a window that +//! exists and the native window is created by that very show — and because +//! `ShowWindow` activates a window regardless of the style. The first version of +//! this module read the foreground window *after* the show and found the card, +//! so its restore did nothing; the holder has to be captured before. That +//! ordering is the whole content of [`keyboard_holder`] and [`apply`]. +//! +//! **Why this is hand-declared rather than a crate.** The same reason +//! `workarea.rs` hand-declares `GetMonitorInfoW`: `ui/Cargo.toml` says the panel +//! has no dependencies beyond Slint on purpose, and a process whose whole job is +//! to turn a click into permission to run a command treats its own supply chain as +//! part of the threat model. A handful of function declarations is smaller than +//! that argument. +//! +//! **Why the window is found by title.** Slint exposes no handle, and reaching +//! winit's requires the `unstable-winit-030` feature. `suggestion.slint` gives the +//! card a title of its own — deliberately not "Squid", which the smoke harness +//! searches for — so the title identifies it. The process id is checked as well: a +//! title is not proof, and another copy of Squid running beside this one has a +//! card with exactly the same title. Acting on that one would make a stranger's +//! window uneditable and leave this run's card taking focus, which is the failure +//! this module exists to prevent. + +/// The suggestion card's title, exactly as `suggestion.slint` spells it. +/// +/// One constant, in one place: the window is found by this string, so a rename +/// that missed it would silently stop the card from keeping its hands off the +/// keyboard. +pub const WINDOW_TITLE: &str = "Squid suggestion"; + +/// A native window handle, as an opaque token. +/// +/// An integer rather than a pointer type so that the one value a caller has to +/// carry across a `show()` is `Copy`, `Send`-free of any lifetime, and comparable +/// on every platform. Zero means "no such window", which every function here +/// treats as "nothing to do" rather than as an error — a desktop with no +/// foreground window is a legitimate state. +pub type Holder = isize; + +/// No window. What a platform that cannot answer reports, and what a session with +/// nothing focused reports. +pub const NONE: Holder = 0; + +#[cfg(windows)] +mod windows { + use super::{Holder, NONE}; + use std::ffi::c_void; + + type Hwnd = *mut c_void; + + /// `GWL_EXSTYLE`: the extended window style. + const GWL_EXSTYLE: std::ffi::c_int = -20; + + /// `WS_EX_NOACTIVATE`: the window never becomes the foreground window when + /// the user clicks it, and never takes the keyboard. + /// + /// Its own documentation is worth quoting because the behaviour surprises + /// people: such a window "does not appear on the taskbar by default", and + /// that is why this is applied to the card and nothing else. + const WS_EX_NOACTIVATE: isize = 0x0800_0000; + + unsafe extern "system" { + /// Finds a top-level window by class name and window name. + /// + /// Null for either is a wildcard, and both null is meaningless — this is + /// called with a title only. + fn FindWindowW(class_name: *const u16, window_name: *const u16) -> Hwnd; + /// Reads one of a window's style values. + fn GetWindowLongPtrW(window: Hwnd, index: std::ffi::c_int) -> isize; + /// Writes one of a window's style values. + fn SetWindowLongPtrW(window: Hwnd, index: std::ffi::c_int, value: isize) -> isize; + /// The process that owns a window. + fn GetWindowThreadProcessId(window: Hwnd, process_id: *mut u32) -> u32; + /// The process this code is running in. + fn GetCurrentProcessId() -> u32; + /// The window that currently has the keyboard, if any. + fn GetForegroundWindow() -> Hwnd; + /// Asks the window manager to give the keyboard to a window. + /// + /// A request, not a command: the foreground lock lets a background + /// process through only when the window it asks for belongs to the + /// foreground process, which the window this card interrupted does. When + /// it is refused nothing breaks — the card keeps the keyboard for that one + /// appearance, which is the pre-existing behaviour. + fn SetForegroundWindow(window: Hwnd) -> std::ffi::c_int; + } + + /// A null-terminated UTF-16 copy of `text`. + /// + /// The platform reads these strings and does not keep them, so a local `Vec` + /// is enough; the trailing zero is the terminator. + fn wide(text: &str) -> Vec { + text.encode_utf16().chain(std::iter::once(0)).collect() + } + + /// The window that currently has the keyboard, if any. + pub fn keyboard_holder() -> Holder { + // SAFETY: no arguments, no state; the result is an opaque handle the + // platform only ever compares and passes back to itself. + unsafe { GetForegroundWindow() as Holder } + } + + /// This run's suggestion window, if it exists yet. + /// + /// `None` while the window is still being created, which is a legitimate + /// answer rather than an error. + fn find_own_window(title: &str) -> Option { + let name = wide(title); + // SAFETY: `name` outlives the call, is null-terminated, and the platform + // neither retains nor writes through it. A null class name is documented + // as "any class". + let window = unsafe { FindWindowW(std::ptr::null(), name.as_ptr()) }; + if window.is_null() { + return None; + } + + let mut owner: u32 = 0; + // SAFETY: `window` came from `FindWindowW` and is only read; `owner` is a + // local the call writes. + unsafe { GetWindowThreadProcessId(window, &mut owner) }; + // SAFETY: no arguments, no state. + let mine = unsafe { GetCurrentProcessId() }; + // A title match on a window of another process is the failure this guards + // against, and doing nothing is the correct response: that window is not + // this run's card and its styles are not this process's to change. + if owner != mine { + return None; + } + Some(window) + } + + /// Marks this run's suggestion window as one that never takes the keyboard, + /// then gives the keyboard back to whoever held it before the card appeared. + /// + /// `restore_to` must be a holder captured *before* the card was shown. Reading + /// it here instead would find the card, because showing it is what took the + /// keyboard — the first version of this did exactly that and restored nothing. + /// + /// Returns whether the window was found. Applying the style twice is a no-op, + /// so every show can call this, which is what makes it safe against the window + /// being recreated. + pub fn apply(title: &str, restore_to: Holder) -> bool { + let Some(window) = find_own_window(title) else { + return false; + }; + + // SAFETY: `window` is a live top-level window of this process and both + // calls read and write only its style word. + let style = unsafe { GetWindowLongPtrW(window, GWL_EXSTYLE) }; + if style & WS_EX_NOACTIVATE == 0 { + // SAFETY: same window; the value adds one documented style bit, and + // the return value is the previous style, which is not needed. + unsafe { SetWindowLongPtrW(window, GWL_EXSTYLE, style | WS_EX_NOACTIVATE) }; + } + + // Restored even when the style was already set: a window that was shown + // once and hidden again comes back through `SW_SHOW` and activates + // anyway, so the keyboard has to be handed back on every appearance. + if restore_to != NONE && restore_to != window as Holder { + // SAFETY: `restore_to` was the foreground window a moment before the + // card was shown, and this call only asks the window manager to give + // it the keyboard back. + unsafe { SetForegroundWindow(restore_to as Hwnd) }; + } + + true + } +} + +/// The window that currently has the keyboard, if any. +/// +/// Call this immediately before showing the card and hand the result to +/// [`apply`]: after the show it is the card, and the restore would be a no-op. +/// Returns [`NONE`] on a platform that cannot answer. +pub fn keyboard_holder() -> Holder { + #[cfg(windows)] + { + windows::keyboard_holder() + } + #[cfg(not(windows))] + { + NONE + } +} + +/// Marks this run's suggestion window as one that never takes the keyboard, and +/// gives the keyboard back to `restore_to`. +/// +/// Returns whether there was a window to mark, which is the same shape as +/// [`crate::workarea::work_area`]: a platform that cannot answer says so with +/// `false` and the caller keeps working. Everywhere but Windows this is `false`, +/// and that is honest rather than provisionally wrong — the platforms that can do +/// this expose it differently, and nothing here has been measured on them. +pub fn apply(title: &str, restore_to: Holder) -> bool { + #[cfg(windows)] + { + windows::apply(title, restore_to) + } + #[cfg(not(windows))] + { + // Named so neither argument is reported as unused on these platforms; an + // underscore would read as "these do not matter", and they do on the + // platform this ships to. + let _ = (title, restore_to); + false + } +} diff --git a/ui/src/main.rs b/ui/src/main.rs index 59b30e9..47a3b65 100644 --- a/ui/src/main.rs +++ b/ui/src/main.rs @@ -34,10 +34,14 @@ //! as `timed_out`, never as consent — so every error path in this file is allowed //! to simply do nothing. +mod keyboard; +mod placement; mod protocol; mod startup; +mod suggestion; mod supervisor; mod transport; +mod workarea; use std::cell::RefCell; use std::path::{Path, PathBuf}; @@ -63,7 +67,39 @@ slint::include_modules!(); /// id or no id at all, which is what makes it impossible for a stale window to /// authorize the plan that replaced it. Taking the value out on answer also /// makes "one answer per prompt" structural rather than a rule to remember. -type Pending = Arc>>; +type Pending = Arc>; + +/// Everything this side remembers about the one prompt on screen. +/// +/// One lock rather than three, because the three fields are one fact: a prompt +/// that is on screen, the same prompt set aside, and an id left over from a +/// prompt that is gone are states that cannot legally coexist. Splitting them +/// would let two of them drift. +#[derive(Default)] +struct PromptState { + /// The id of the prompt currently on screen, or `None` when nothing is + /// pending. + /// + /// This is the whole correlation mechanism on this side. A click answers + /// *this* id or no id at all, which is what makes it impossible for a stale + /// window to authorize the plan that replaced it. Taking the value out on + /// answer also makes "one answer per prompt" structural rather than a rule + /// to remember. + pending: Option, + /// The prompt currently on screen, kept whole. + /// + /// Held rather than reconstructed from the widgets that happen to be showing + /// it so that Later is a move, not a re-read of five properties across two + /// surfaces. It also means a set-aside prompt comes back with exactly the + /// text it arrived with, whatever either surface has done since. + current: Option, + /// The prompt a person set aside with Later. + /// + /// The request stays live on the brain's side either way — deferring changes + /// what is shown, never what was answered, and the engine's own deadline is + /// still what decides an unanswered prompt. + deferred: Option, +} /// A poisoned lock means another thread panicked while holding this slot, so the /// pending id can no longer be trusted to name the plan on screen. Answering @@ -386,11 +422,64 @@ fn main() { Ok(window) => window, Err(error) => startup::abort(Startup::Platform(error.to_string())), }; - let pending: Pending = Arc::new(Mutex::new(None)); + let pending: Pending = Arc::new(Mutex::new(PromptState::default())); // One outgoing half, shared. The reader thread hands it a socket on connect // and takes it back on disconnect; the callbacks below only ever write. let outgoing = Outgoing::new(); + // The second top-level window, created once and reused for every prompt that + // belongs on it. Created before `run()`, because a window made after the + // event loop has started would be made from another thread; a failure here is + // not fatal — the panel's own card is a surface too, and `suggestion::show` + // says so by returning false. + // + // `adopt` takes ownership of the component, because the surface has to be + // able to show the window again after every hide: a `Weak` kept here would be + // the window dying on the first `Later`. + match SuggestionWindow::new() { + Ok(floating) => { + { + let pending = Arc::clone(&pending); + let outgoing = outgoing.clone(); + let panel = window.as_weak(); + floating.on_allow(move || answer(&pending, &outgoing, &panel, Answer::Approved)); + } + { + let pending = Arc::clone(&pending); + let outgoing = outgoing.clone(); + let panel = window.as_weak(); + floating.on_decline(move || answer(&pending, &outgoing, &panel, Answer::Declined)); + } + { + let pending = Arc::clone(&pending); + let panel = window.as_weak(); + floating.on_defer(move || defer(&pending, &panel)); + } + // A frameless card has no close button, but Alt+F4 and a window + // manager's own menu still ask. Hiding is the right answer to a + // close — the conversation has not changed — but hiding alone would + // leave the panel believing the card is on screen, so the close is + // routed through the same set-aside a `Later` click takes. The panel + // then keeps a notice offering to bring it back, which is the state + // the person can actually recover from. + { + let pending = Arc::clone(&pending); + let panel = window.as_weak(); + floating.window().on_close_requested(move || { + defer(&pending, &panel); + slint::CloseRequestResponse::HideWindow + }); + } + suggestion::adopt(floating); + } + Err(error) => { + // A desktop that could not make a second window — no display, no + // compositor. `suggestion::show` finds no window and reports it, so + // a suggestion is asked on the panel's card instead of nowhere. + eprintln!("squid-ui: no floating suggestion window: {error}"); + } + } + // Only a run that started its own brain offers to restart it. The brain // refuses `op:"restart"` unless it is supervised, so showing the button in // attached mode would be selling a switch that does nothing. @@ -512,30 +601,39 @@ fn main() { let handle = window.as_weak(); window.on_decline(move || answer(&pending, &outgoing, &handle, Answer::Declined)); } + { + // Both directions are the same slot seen twice: setting aside keeps the + // id so the card can be called back, and reopening only has to put it on + // a surface again. Neither answers, so neither can authorize anything. + let pending = Arc::clone(&pending); + let handle = window.as_weak(); + window.on_defer(move || defer(&pending, &handle)); + } + { + let pending = Arc::clone(&pending); + let handle = window.as_weak(); + window.on_reopen(move || reopen(&pending, &handle)); + } - // Closing the panel is a decline of anything pending, not a hang. + // Closing the panel is "put away", not an answer either way. // - // Both are refusals, so either is safe. Declining is the better one: the - // engine's queue is serial, and a prompt that waits out its full timeout - // holds every later candidate behind it. Shutting the panel should not cost - // the next suggestion its turn. + // The alternative was to treat a close as a decline so the engine's serial + // queue is not held by a prompt nobody is looking at any more. It is the + // wrong trade: a window that is closed by accident would then refuse a + // suggestion the person never saw, and the tray is what makes a close + // recoverable. The engine's own confirmation timeout is what an unanswered + // prompt meets, and it is the only thing that should end one. { - { - // Closing is "put away", not "end". The tray menu is measured to - // export on Linux (see `ui/tray.slint`), so a hidden window still has - // a way back and a way out of its own interface. Nothing here answers - // a pending prompt: hiding changes nothing about the conversation, - // the same way walking away from an open window does not, and the - // engine's confirmation timeout is what an unanswered prompt meets. - // On a desktop with no tray host the loop dies with the hidden - // window — the pre-tray behavior, needing no code here. (Kept by - // Slint's keepalive counter: `Window::hide` releases it, and a - // live tray handle acquired one. See `AGENTS.md`'s tray section - // for what this box can and cannot measure of that chain.) - window - .window() - .on_close_requested(|| slint::CloseRequestResponse::HideWindow); - } + // A hidden window still has a way back and a way out of its own + // interface: the tray menu is measured to export on Linux (see + // `ui/tray.slint`). On a desktop with no tray host the loop dies with + // the hidden window — the pre-tray behavior, needing no code here. + // (Kept by Slint's keepalive counter: `Window::hide` releases it, and a + // live tray handle acquired one. See `AGENTS.md`'s tray section for what + // this box can and cannot measure of that chain.) + window + .window() + .on_close_requested(|| slint::CloseRequestResponse::HideWindow); } { @@ -1095,59 +1193,107 @@ fn count(value: Option) -> i32 { } } -/// Opens the confirmation area and claims the pending slot for this prompt. -fn show_prompt(window: &slint::Weak, pending: &Pending, prompt: Prompt) { - // Claim before displaying: a click can only ever settle an id that is - // already recorded, so there is no window in which the buttons are live but - // unattributed. +/// Opens the question on the surface that can actually reach the person. +/// +/// Runs on the reader thread, so every write to a window happens inside the one +/// posted closure. Which surface asks is decided here rather than inside either +/// window, because the choice belongs to the kind of prompt and not to the +/// window: a suggestion is worth nothing on a panel the person put away hours +/// ago, and a decision about running something is worth nothing in a card they +/// will not look at. +/// +/// The pending slot is claimed before either surface is touched, so a click can +/// only ever settle an id that is already recorded. +fn show_prompt(panel: &slint::Weak, pending: &Pending, prompt: Prompt) { + // A suggestion is the one kind that leaves the resident panel. Every other + // kind is a decision about running something, and a deliberate answer to that + // belongs on the resident window. + let floating_kind = suggestion::is_floating_kind(&prompt.kind); + + // Claiming replaces any setting-aside, because a new prompt answers the + // notice that offered to reopen the old one. Leaving `deferred` set would put + // a standing notice on screen for a question nobody is asking any more. // // A prompt arriving while another is pending replaces it. The engine // serializes asks so this should not happen, and if it does the abandoned id // is one the TypeScript side has already stopped waiting for. - *pending.lock().expect(POISONED) = Some(prompt.prompt_id); + { + let mut state = pending.lock().expect(POISONED); + state.pending = Some(prompt.prompt_id); + state.current = Some(prompt.clone()); + state.deferred = None; + } - let _ = window.upgrade_in_event_loop(move |window| { - window.set_prompt_kind(prompt.kind.into()); - window.set_prompt_summary(prompt.summary.into()); - window.set_prompt_expires_at(prompt.expires_at.into()); + // One closure rather than two posted in sequence, because the panel must be + // told where the question went in the same breath as it is told there is a + // question. Split across two, a frame could show the panel's own Allow button + // while the floating card holds the same decision. + // + // `show` reports whether the floating surface could take the prompt at all: + // no such window on this desktop, no work area, a screen too small for the + // card. When it says no, the panel's card is the surface — which is why the + // answer is a property rather than a decision taken here. + let _ = panel.upgrade_in_event_loop(move |panel| { + let floating = floating_kind && suggestion::show(&prompt); + panel.set_prompt_kind(prompt.kind.into()); + panel.set_prompt_summary(prompt.summary.into()); + panel.set_prompt_expires_at(prompt.expires_at.into()); // Absent reads as empty, and the card hides the line rather than printing // a heading with nothing under it. A request that does not explain itself // is shown without an explanation, never with one invented here. - window.set_prompt_why(prompt.why.unwrap_or_default().into()); - window.set_prompt_deny_consequence(prompt.deny_consequence.unwrap_or_default().into()); + panel.set_prompt_why(prompt.why.unwrap_or_default().into()); + panel.set_prompt_deny_consequence(prompt.deny_consequence.unwrap_or_default().into()); + panel.set_deferred(false); + panel.set_prompt_floating(floating); // Last, so the buttons appear only once the text beside them is the text - // of this prompt. - window.set_prompt_pending(true); + // of this prompt, and only on the surface that has it. + panel.set_prompt_pending(true); }); } /// Withdraws a prompt the engine has given up on. -fn cancel(window: &slint::Weak, pending: &Pending, prompt_id: i64) { +fn cancel(panel: &slint::Weak, pending: &Pending, prompt_id: i64) { { - let mut slot = pending.lock().expect(POISONED); + let mut state = pending.lock().expect(POISONED); // A cancel for an id that is no longer pending is stale — it names a // prompt already answered or already replaced. Acting on it would close a // live confirmation and lose an answer the user is about to give. - if *slot != Some(prompt_id) { + if state.pending != Some(prompt_id) { return; } - *slot = None; + state.pending = None; + state.current = None; + state.deferred = None; } - let _ = window.upgrade_in_event_loop(clear_prompt); + let _ = panel.upgrade_in_event_loop(|panel| { + suggestion::hide(); + clear_prompt(panel); + }); } -/// Writes one answer for the pending prompt, then closes the confirmation area. +/// Writes one answer for the pending prompt, then closes both surfaces. /// -/// Runs on the UI thread: both button callbacks and the close handler are Slint -/// callbacks. Takes the pending id, so a second click after the first answer -/// finds nothing to answer. +/// Runs on the UI thread: every button callback and every close handler here is +/// a Slint callback. It takes the pending id out of the slot, so a second click +/// after the first answer finds nothing to answer — which is what makes one +/// answer per prompt structural rather than a rule each surface has to +/// remember. fn answer( pending: &Pending, outgoing: &Outgoing, - window: &slint::Weak, + panel: &slint::Weak, answer: Answer, ) { - let Some(prompt_id) = pending.lock().expect(POISONED).take() else { + // Taken out under the lock, so the id is spent the moment it is read. The + // setting-aside copy goes with it: a prompt that has been answered must not + // come back from the notice that offered to reopen it. + let prompt_id = { + let mut state = pending.lock().expect(POISONED); + state.deferred = None; + state.current = None; + state.pending.take() + }; + let Some(prompt_id) = prompt_id else { // Nothing pending: a click that raced a cancel, or a second click on an // already-answered prompt. Writing an uncorrelated answer here is the one // thing that could authorize the wrong plan. @@ -1161,24 +1307,85 @@ fn answer( // consumed here would be the worse surface. outgoing.send(&answer_line(prompt_id, answer)); - if let Some(window) = window.upgrade() { - clear_prompt(window); + if let Some(panel) = panel.upgrade() { + suggestion::hide(); + clear_prompt(panel); + } +} + +/// Sets the pending prompt aside so the floating card stops asking. +/// +/// A set-aside prompt is not an answered one: the request stays live on the +/// brain's side and its own deadline still decides it. What changes is which +/// surface shows it — the floating card goes away and the panel keeps a standing +/// notice offering to bring it back. +/// +/// The slot keeps the id, because a click on the reopened card has to settle the +/// same question. A click that arrives mid-defer therefore cannot resurrect a +/// prompt that has just been answered; the answer path empties the slot under +/// the same lock. +fn defer(pending: &Pending, panel: &slint::Weak) { + { + let mut state = pending.lock().expect(POISONED); + // Nothing pending means nothing to set aside — a click that raced a + // cancel. Manufacturing a deferred prompt out of an empty slot would put + // a standing notice on screen for a question nobody is asking. + let Some(current) = state.current.clone() else { + return; + }; + state.deferred = Some(current); } + let _ = panel.upgrade_in_event_loop(|panel| { + suggestion::hide(); + // One rendering of a deferred prompt, not two: the notice replaces the + // card, so the card's buttons go away with it. + panel.set_prompt_floating(false); + panel.set_deferred(true); + }); +} + +/// Puts a set-aside prompt back in front of the person. +/// +/// A suggestion returns to the floating card, because that is the surface it is +/// worth anything on; anything else returns to the panel's own card. Placement +/// is retried rather than remembered — a card that could not be placed when it +/// arrived may be placeable now, and one that could be placed may not be any +/// more. +fn reopen(pending: &Pending, panel: &slint::Weak) { + let deferred = pending.lock().expect(POISONED).deferred.take(); + let Some(prompt) = deferred else { + // Nothing was set aside — a second click on the notice. Clearing the flag + // anyway is the honest answer: the notice should not outlive its reason. + let _ = panel.upgrade_in_event_loop(|panel| panel.set_deferred(false)); + return; + }; + + let floating_kind = suggestion::is_floating_kind(&prompt.kind); + let _ = panel.upgrade_in_event_loop(move |panel| { + // The panel's card still holds this prompt's text from when it arrived, + // so reopening only has to take the notice away — unless the floating + // card can take it, in which case that is where it goes back to. + let floating = floating_kind && suggestion::show(&prompt); + panel.set_deferred(false); + panel.set_prompt_floating(floating); + }); } /// Empties the confirmation area. /// -/// The stale text is cleared as well as the flag. `panel.slint` drops the whole +/// The stale text is cleared as well as the flags. `panel.slint` drops the whole /// section when nothing is pending, so this is not visible today — but a summary /// left behind is a command description waiting to reappear next to buttons that /// belong to a different plan. -fn clear_prompt(window: PanelWindow) { - window.set_prompt_pending(false); - window.set_prompt_kind(SharedString::new()); - window.set_prompt_summary(SharedString::new()); - window.set_prompt_expires_at(SharedString::new()); - window.set_prompt_why(SharedString::new()); - window.set_prompt_deny_consequence(SharedString::new()); +fn clear_prompt(panel: PanelWindow) { + panel.set_prompt_pending(false); + panel.set_prompt_floating(false); + panel.set_deferred(false); + panel.set_prompt_kind(SharedString::new()); + panel.set_prompt_summary(SharedString::new()); + panel.set_prompt_expires_at(SharedString::new()); + panel.set_prompt_why(SharedString::new()); + panel.set_prompt_deny_consequence(SharedString::new()); } #[cfg(test)] diff --git a/ui/src/placement.rs b/ui/src/placement.rs new file mode 100644 index 0000000..8c3b63a --- /dev/null +++ b/ui/src/placement.rs @@ -0,0 +1,359 @@ +//! Where the floating suggestion window goes, and whether it can go there at all. +//! +//! The suggestion is the one prompt whose value depends on *when* it is seen: it +//! names text for the field the person is typing in right now. A card inside the +//! resident panel is a suggestion somebody has to go and fetch, so this module +//! answers the only question that matters for such a prompt — the corner of the +//! screen it should appear in — and answers it in arithmetic that can be tested +//! without a display. +//! +//! Two things make this its own module rather than a few lines in `main.rs`. +//! +//! **The geometry is not Slint's to give.** Slint exposes a window's own +//! position and size and nothing else; winit exposes monitors, but a monitor is +//! the *screen*, not the area a window is allowed to occupy once the taskbar +//! and the docks have taken their share. Only the work area is the rectangle a +//! floating card belongs in, and on Windows only the platform knows it. The +//! lookup is therefore platform code, and the arithmetic that consumes it is +//! not: [`bottom_right`] is a pure function of a rectangle and a size, and that +//! is where the tests live. +//! +//! **Placement is a decision, not a coordinate.** A card whose corner is off the +//! screen is worse than a card that never appeared, and a card that appeared +//! without the person being able to reach it is worse still. So the fallback is +//! not "somewhere else on the screen" — it is *not at all*, and the caller sends +//! the prompt to the panel's own card instead. That keeps the panel a complete +//! surface: every prompt has somewhere to be asked, whatever the display can do. + +/// A rectangle in physical screen pixels. +/// +/// Physical, not logical, because this is what the platform reports and what +/// `Window::set_position` takes. A monitor rectangle scaled by a factor that was +/// applied twice is off by exactly the DPI ratio, which on a 150% display is +/// half the screen — a card in the wrong corner, or off it entirely. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Rect { + /// The left edge, in physical pixels from the left of the virtual screen. + pub x: i32, + /// The top edge, in physical pixels from the top of the virtual screen. + pub y: i32, + /// The width, in physical pixels. + pub width: u32, + /// The height, in physical pixels. + pub height: u32, +} + +impl Rect { + /// The right edge, exclusive. + /// + /// Saturating rather than wrapping: a rectangle whose width overflows its + /// `i32` is a rectangle the platform cannot describe, and a wrapped + /// coordinate would put the card at the far left of the screen instead of + /// nowhere. + pub fn right(self) -> i32 { + self.x + .saturating_add(i32::try_from(self.width).unwrap_or(i32::MAX)) + } + + /// The bottom edge, exclusive. + pub fn bottom(self) -> i32 { + self.y + .saturating_add(i32::try_from(self.height).unwrap_or(i32::MAX)) + } + + /// Whether this rectangle describes an area at all. + /// + /// A zero-sized or negative-sized work area is not a place a card can be + /// put, and treating it as one is how a card ends up at `x = 0, y = 0` with + /// its own body hanging off the left and top edges of the screen. + pub fn has_area(self) -> bool { + self.width > 0 && self.height > 0 + } +} + +/// How far from the corner the card sits, in logical pixels. +/// +/// Not zero: a card flush against the corner of the work area has its own +/// shadow and border half-clipped by the screen edge, which reads as a +/// rendering fault rather than a deliberate position. Sixteen logical pixels is +/// the same inset the panel uses around its own content, so the floating card +/// and the resident panel agree about what "near the edge" means. +/// +/// Logical, and therefore scaled before it is subtracted from a rectangle +/// measured in physical pixels. Nothing else here is: leaving it unscaled made +/// the gap *narrower* the higher the DPI, so the card sat tighter to the corner +/// on the 125% display it was measured on than on the 100% one it was designed +/// for — the opposite of what a fixed design distance should do. +pub const CORNER_INSET: i32 = 16; + +/// The size of the floating card, in logical pixels. +/// +/// Fixed rather than measured, because the card's contents vary with the +/// suggestion it carries and a card that changes width as its text changes +/// would visibly twitch in the corner every time a new suggestion arrives. The +/// width is the one `suggestion.slint` asks for; the height is the card's +/// natural height, which the platform measures. +pub const CARD_WIDTH: i32 = 360; + +/// Turns a distance in logical pixels into the physical pixels the work area is +/// measured in. +/// +/// One function, called by every place that mixes the two units, because the +/// defect this file kept producing was a scale applied to one number and not to +/// its neighbour: a card scaled but not the inset, an inset scaled but not the +/// measured size. Both are the same arithmetic, so both go through here. +/// +/// A scale the platform cannot mean is treated as 1.0 rather than trusted: a zero +/// would size everything to nothing, and a NaN would produce a position that is +/// not a number. +pub fn to_physical(logical: i32, scale: f32) -> i32 { + let scale = if scale.is_finite() && scale > 0.0 { + scale + } else { + 1.0 + }; + (logical as f32 * scale).round() as i32 +} + +/// Where a card of the given size goes, in the bottom-right corner of `area`. +/// +/// Every argument is in physical pixels, and that is the whole contract: the +/// conversions happen once, at the call site that knows whether it is holding a +/// logical size from the `.slint` file or a physical one from a laid-out window. +/// Keeping a `scale` parameter here instead was how the scale came to be applied +/// to the card and not to its inset. +/// +/// Returns `None` when the card does not fit, and that is the point of the +/// function: a suggestion that cannot be placed where the person can see it has +/// to be asked somewhere else, not squeezed onto a screen that has no room. +pub fn bottom_right( + area: Rect, + card_width: i32, + card_height: i32, + inset: i32, +) -> Option<(i32, i32)> { + if !area.has_area() { + return None; + } + if card_width <= 0 || card_height <= 0 || inset < 0 { + return None; + } + + // The card is placed by its top-left corner, which is what + // `Window::set_position` takes. The bottom-right corner of the card is + // therefore `right - inset - width`, and the same for the vertical axis. + let right = area.right() - inset - card_width; + let bottom = area.bottom() - inset - card_height; + + // Off the work area is off the screen as far as the person is concerned. + // Returning a position anyway is how a card ends up behind the taskbar or + // half off the right edge, where it looks like a rendering fault. + if right < area.x || bottom < area.y { + return None; + } + + Some((right, bottom)) +} + +#[cfg(test)] +mod tests { + use super::{bottom_right, to_physical, Rect, CARD_WIDTH, CORNER_INSET}; + + /// A work area with room to spare, at 100% scaling. + const ROOMY: Rect = Rect { + x: 0, + y: 0, + width: 1920, + height: 1080, + }; + + /// The card's height in these tests, in logical pixels. + const CARD_HEIGHT: i32 = 200; + + /// Places the card the way a caller does: both distances converted once. + /// + /// Written as a helper rather than repeated in every test because it is the + /// call shape under test — a card scaled and its inset not is exactly the + /// defect these tests exist for, and a test that scaled by hand could make + /// the same mistake the caller did. + fn place(area: Rect, scale: f32) -> Option<(i32, i32)> { + bottom_right( + area, + to_physical(CARD_WIDTH, scale), + to_physical(CARD_HEIGHT, scale), + to_physical(CORNER_INSET, scale), + ) + } + + /// The card goes in the corner, inset from the work area's edges. + /// + /// The exact numbers matter more than the shape: a card at the very corner + /// is half-clipped by the screen edge, and a card whose bottom edge is + /// above the bottom of the work area by an unknown amount is a card whose + /// position nobody can check. + #[test] + fn the_card_sits_in_the_bottom_right_corner_inset_from_the_edges() { + let (x, y) = place(ROOMY, 1.0).expect("the card fits"); + assert_eq!(x, 1920 - CORNER_INSET - CARD_WIDTH); + assert_eq!(y, 1080 - CORNER_INSET - CARD_HEIGHT); + } + + /// The card's right and bottom edges clear the work area's own edges. + /// + /// Stated as edges rather than as the top-left coordinate, because that is + /// the thing a person sees: the card must not hang over the right or the + /// bottom of the screen. + #[test] + fn the_card_never_hangs_over_the_right_or_bottom_edge() { + let (x, y) = place(ROOMY, 1.0).expect("the card fits"); + assert!(x + CARD_WIDTH <= ROOMY.right(), "{x}"); + assert!(y + CARD_HEIGHT <= ROOMY.bottom(), "{y}"); + } + + /// A work area that does not start at the origin is offset by the same + /// amount. + /// + /// A second monitor to the left of the primary is the case this exists for: + /// its coordinates are negative, and a placement that ignored the offset + /// would put the card on the other monitor entirely. + #[test] + fn a_work_area_that_does_not_start_at_the_origin_is_honoured() { + let left_monitor = Rect { + x: -1920, + y: 0, + width: 1920, + height: 1080, + }; + let (x, y) = place(left_monitor, 1.0).expect("the card fits"); + assert_eq!(x, left_monitor.x + 1920 - CORNER_INSET - CARD_WIDTH); + assert_eq!(y, 1080 - CORNER_INSET - CARD_HEIGHT); + // And it is still on that monitor, not on the one to the right. + assert!(x >= left_monitor.x, "{x}"); + assert!(x + CARD_WIDTH <= left_monitor.right(), "{x}"); + } + + /// The work area is the *usable* area, so the card clears the taskbar. + /// + /// Measured on the development machine: a 2048×1152 display whose work area + /// is 2048×1104, because the taskbar takes 48 pixels. A card placed against + /// the bottom of the *screen* rather than the bottom of the work area would + /// be under the taskbar, where it cannot be seen and cannot be clicked. + #[test] + fn the_card_clears_the_taskbar_because_the_work_area_excludes_it() { + let work = Rect { + x: 0, + y: 0, + width: 2048, + height: 1104, + }; + let (_, y) = place(work, 1.0).expect("the card fits"); + assert!(y + CARD_HEIGHT <= work.bottom(), "{y}"); + // 48 pixels of taskbar are below the work area's bottom edge. + assert!(1152 - (y + CARD_HEIGHT) >= 48, "{y}"); + } + + /// A display that is too small for the card produces no position at all. + /// + /// The fallback is the panel's own card, and that only works if this + /// function refuses rather than guessing. + #[test] + fn a_card_that_does_not_fit_has_no_position() { + let tiny = Rect { + x: 0, + y: 0, + width: 300, + height: 200, + }; + assert!(place(tiny, 1.0).is_none()); + } + + /// A work area with no area in it is refused, not treated as the origin. + /// + /// The failure this guards against is a platform that reports a zero-sized + /// work area on a headless session: placing the card at `0, 0` would put it + /// on top of whatever is there, which is the opposite of helpful. + #[test] + fn an_empty_work_area_is_refused() { + let empty = Rect { + x: 0, + y: 0, + width: 0, + height: 0, + }; + assert!(place(empty, 1.0).is_none()); + } + + /// Both the card and its inset are distances in logical pixels, so both are + /// converted. + /// + /// A 150% display is the case: the card is 360 logical pixels wide, so it + /// occupies 540 physical pixels, and a placement that used the logical + /// number would leave it 180 pixels short of the corner. The inset is 24 + /// physical pixels rather than 16 for the same reason. + #[test] + fn the_card_and_its_inset_are_both_scaled() { + let (x, y) = place(ROOMY, 1.5).expect("the card fits"); + assert_eq!(x, 1920 - 24 - 540); + assert_eq!(y, 1080 - 24 - 300); + } + + /// The gap from the corner is the same *visual* distance at every DPI. + /// + /// Measured on this repository's development box, a 125% display: an + /// unscaled inset put the card twelve-and-a-bit logical pixels from the + /// corner instead of sixteen. Close enough to look deliberate and wrong + /// enough to be a defect nobody reports, which is why it is a test rather + /// than a remembered number. + #[test] + fn the_corner_gap_is_the_same_visual_distance_at_any_scale() { + for scale in [1.0f32, 1.25, 1.5, 2.0] { + let (x, y) = place(ROOMY, scale).expect("the card fits"); + let width = to_physical(CARD_WIDTH, scale); + let height = to_physical(CARD_HEIGHT, scale); + let gap_x = (ROOMY.right() - (x + width)) as f32 / scale; + let gap_y = (ROOMY.bottom() - (y + height)) as f32 / scale; + assert_eq!(gap_x.round() as i32, CORNER_INSET, "at {scale}"); + assert_eq!(gap_y.round() as i32, CORNER_INSET, "at {scale}"); + } + } + + /// A scale factor that is not a number does not produce a position that is + /// not a number. + /// + /// The scale comes from the platform, and a zero or negative one means the + /// card would be sized to nothing. Everything is then sized as if the scale + /// were one, which is at least a card on the screen. + #[test] + fn an_impossible_scale_falls_back_to_one() { + assert_eq!(to_physical(CARD_WIDTH, 0.0), CARD_WIDTH); + assert_eq!(to_physical(CARD_WIDTH, -1.0), CARD_WIDTH); + assert_eq!(to_physical(CARD_WIDTH, f32::NAN), CARD_WIDTH); + let (x, _) = place(ROOMY, 0.0).expect("the card fits"); + assert_eq!(x, 1920 - CORNER_INSET - CARD_WIDTH); + } + + /// A size that is not a size is refused rather than placed. + /// + /// A card measured before it has been laid out reports zero, and placing + /// that would put the card's top-left corner at the corner of the screen + /// with nothing drawn inside it. + #[test] + fn a_size_of_nothing_is_refused() { + assert!(bottom_right(ROOMY, 0, CARD_HEIGHT, CORNER_INSET).is_none()); + assert!(bottom_right(ROOMY, CARD_WIDTH, 0, CORNER_INSET).is_none()); + assert!(bottom_right(ROOMY, -CARD_WIDTH, CARD_HEIGHT, CORNER_INSET).is_none()); + assert!(bottom_right(ROOMY, CARD_WIDTH, CARD_HEIGHT, -CORNER_INSET).is_none()); + } + + /// The inset is a real distance, not a comment. + /// + /// A card flush against the boundary of the work area has been placed by a + /// computer; the point of the inset is that a person recognises the position + /// as deliberate. + #[test] + fn the_card_keeps_its_distance_from_the_work_area_edge() { + let (x, y) = place(ROOMY, 1.0).expect("the card fits"); + assert_eq!(ROOMY.bottom() - (y + CARD_HEIGHT), CORNER_INSET); + assert_eq!(ROOMY.right() - (x + CARD_WIDTH), CORNER_INSET); + } +} diff --git a/ui/src/suggestion.rs b/ui/src/suggestion.rs new file mode 100644 index 0000000..60a4f8c --- /dev/null +++ b/ui/src/suggestion.rs @@ -0,0 +1,361 @@ +//! The floating surface an input suggestion is asked on. +//! +//! A suggestion is the one prompt whose value depends on *when* it is seen: it +//! names text for the field the person is typing in right now. The resident +//! panel is a window they put away hours ago, so a card inside it is a +//! suggestion nobody receives. This module owns the second top-level window +//! that answers such a prompt, and the arithmetic that puts it in the corner of +//! the screen. +//! +//! Three decisions are load-bearing, and each of them is a measurement rather +//! than a preference. +//! +//! **It does not take focus.** The suggestion arrives while the person is +//! typing, and a window that steals the keyboard mid-word is worse than no +//! suggestion at all. Measured on the development box: without `ui/src/keyboard.rs` +//! the card becomes the foreground window on *every* show, first and +//! subsequent alike, and the typing it was recommending text for goes to the +//! card's buttons. Slint declares no property for this, so the style bit +//! `WS_EX_NOACTIVATE` is applied to the native window by that module, which also +//! gives the keyboard back after the show that necessarily preceded it. +//! +//! **It is placed before it is shown, and corrected after it is measured.** The +//! corner depends on how tall the card turned out to be, and the card's height +//! depends on how long the suggestion is. The first placement therefore uses the +//! nominal size — so the window's first visible frame is already in the corner +//! rather than at whatever position the windowing system picked — and the +//! second uses the size the card actually reached. A card that appears in the +//! wrong place and then jumps is worse than a card that appears in the right +//! place. +//! +//! **It never becomes the only place a prompt is asked.** Every other kind of +//! prompt stays on the panel's own card, and a suggestion that cannot be placed +//! — no work area, a screen too small for the card — falls back to the panel +//! too. One answer per prompt is structural here: the pending id is taken when +//! the prompt is claimed, so a click on either surface settles the same +//! question, and the other surface's buttons go away. + +use std::cell::RefCell; + +use slint::{ComponentHandle, SharedString}; + +use crate::placement::{self, CARD_WIDTH}; +use crate::protocol::Prompt; +use crate::SuggestionWindow; + +/// The card's height in logical pixels, for the placement made before the card +/// has been measured. +/// +/// The card's real height depends on its contents, so this is only the size used +/// to decide whether the card fits at all and where its first frame lands. The +/// second placement uses the height the card actually reached. +const NOMINAL_CARD_HEIGHT: i32 = 200; + +/// How long after showing the card is measured again. +/// +/// One frame's worth: the layout that turns the nominal height into the real one +/// happens on the first iteration of the event loop after `show`, and that is +/// what this waits out. Deliberately not zero — a zero-delay timer fires in the +/// same iteration, before anything has been measured — and deliberately not a +/// human-scale delay, because until it fires the card sits at an estimated +/// height rather than its own. +const MEASURE_DELAY: std::time::Duration = std::time::Duration::from_millis(16); + +/// The state of the floating surface, held on the UI thread. +/// +/// A thread-local for the same reason the rest of the panel's view state is +/// one: every reader and writer here is the Slint event loop, and the reader +/// thread cannot hold a reference to it. A mutex would have been a claim about +/// threads that never touch it. +#[derive(Default)] +pub struct Surface { + /// The window, once it exists. + /// + /// Held strongly, which is the whole of the lifetime: the surface is hidden + /// and shown for every suggestion, and a weak reference kept here would be a + /// window that lived exactly as long as the `main` binding that created it. + /// Hiding is what makes it go away, not dropping it. + /// + /// `None` on a desktop that could not make the window at all, which is the + /// one case where a suggestion has nowhere of its own to go and the panel's + /// card is the whole surface. + window: RefCell>, + /// The timer of the pending post-show correction, if one is armed. + /// + /// A `slint::Timer` stops when it is dropped, and the correction has to run + /// after the call that armed it — so the surface is what keeps it alive. + /// Holding it here rather than leaking it means arming a second correction + /// (a new suggestion arriving before the first settled) drops the first, + /// which is exactly right: the old placement belongs to a card whose text + /// is no longer on screen. + correction: RefCell>, +} + +thread_local! { + // Built field by field rather than with `Surface::default()`: the + // `thread_local` initialiser runs in a const context, and a derived `Default` + // is not a const fn. + static SURFACE: RefCell = const { + RefCell::new(Surface { + window: RefCell::new(None), + correction: RefCell::new(None), + }) + }; +} + +/// Runs `body` against the UI thread's suggestion surface. +pub fn with_surface(body: impl FnOnce(&Surface) -> T) -> T { + SURFACE.with(|surface| body(&surface.borrow())) +} + +/// Whether a prompt of this kind belongs on the floating surface. +/// +/// The discriminator is the wire's own `kind`, and this is the only place that +/// reads it. `input_suggestion` is the kind the brain sends for a proactive +/// suggestion (`src/input-suggestion.ts`); every other kind is a decision about +/// running something, and those belong on the resident panel where a +/// deliberate answer can be given. +/// +/// The check is a prefix rather than an equality so that a future +/// `input_suggestion_with_history` or `input_suggestion.quick` lands on the same +/// surface without a second edit here — the vocabulary is the brain's, and this +/// should not need to know every word in it. +pub fn is_floating_kind(kind: &str) -> bool { + kind == "input_suggestion" || kind.starts_with("input_suggestion.") +} + +/// The card's measured size, in physical pixels, or `None` before it is known. +/// +/// The window's own size is the measurement: it is the only one that accounts +/// for the text that is actually in the card. +fn measured_size(window: &slint::Window) -> Option<(i32, i32)> { + let size = window.size(); + let width = i32::try_from(size.width).ok()?; + let height = i32::try_from(size.height).ok()?; + if width <= 0 || height <= 0 { + return None; + } + Some((width, height)) +} + +/// The scale factor of the window's own display. +/// +/// Slint reports this per window, which is what makes a card on a 150% display +/// the right size there and the right size on a 100% one. The work area is in +/// physical pixels, so the two have to be brought into the same space before +/// they are added. +fn scale_factor(window: &slint::Window) -> f32 { + let factor = window.scale_factor(); + if factor.is_finite() && factor > 0.0 { + factor + } else { + 1.0 + } +} + +/// Puts the card in the bottom-right corner of the usable area. +/// +/// `width` and `height` are already in physical pixels — the unit the work area +/// is measured in and `set_position` takes. They are not scaled here, and that +/// is deliberate: the two callers hold different things. Before the window has +/// been laid out the only size known is the one `suggestion.slint` declares, in +/// logical pixels, so the caller converts it with [`placement::to_physical`]; +/// after it, the window reports physical pixels already. Converting in this +/// function as well would apply the DPI twice to one of them, which is the +/// defect this seam was split to prevent. +/// +/// Returns whether it could be placed. A card that cannot be placed is not put +/// somewhere approximate: the caller sends the prompt to the panel instead, +/// because a suggestion nobody can see is not a suggestion. +fn place(window: &slint::Window, width: i32, height: i32) -> bool { + let Some(area) = crate::workarea::work_area() else { + return false; + }; + let inset = placement::to_physical(placement::CORNER_INSET, scale_factor(window)); + let Some((x, y)) = placement::bottom_right(area, width, height, inset) else { + return false; + }; + window.set_position(slint::WindowPosition::Physical( + slint::PhysicalPosition::new(x, y), + )); + true +} + +/// Shows a suggestion on the floating surface. +/// +/// Returns whether it was shown. The caller falls back to the panel's own card +/// when this returns false, so every prompt still has a surface — which is why +/// this reports rather than panics. +/// +/// Runs on the UI thread, and must: it touches the window and the thread-local +/// that holds it. +pub fn show(prompt: &Prompt) -> bool { + with_surface(|surface| { + // A strong clone taken through the `Ref`, which is dropped at the end of + // this statement. Holding the borrow across the body would be a + // `RefCell` borrow alive while the window lays itself out and can call + // back into this module. + let window = surface + .window + .borrow() + .as_ref() + .map(ComponentHandle::clone_strong); + let Some(window) = window else { + // No floating window in this run — a desktop that could not make + // one. The panel's card is the whole surface then. + return false; + }; + + window.set_summary(prompt.summary.as_str().into()); + window.set_why(prompt.why.clone().unwrap_or_default().into()); + window.set_deny_consequence(prompt.deny_consequence.clone().unwrap_or_default().into()); + window.set_expires_at(prompt.expires_at.as_str().into()); + + // The first placement, before the window is visible: the nominal size is + // enough to know whether the card fits, and placing here is what stops + // the window's first frame from appearing where the windowing system + // left it. A card that does not fit at its nominal size will not fit at + // its real one either, so this is also the placeability test. + let handle = window.window(); + let scale = scale_factor(handle); + // The one conversion: this size comes from the `.slint` file and is + // logical; everything `place` touches afterwards is physical. + let nominal_width = placement::to_physical(CARD_WIDTH, scale); + let nominal_height = placement::to_physical(NOMINAL_CARD_HEIGHT, scale); + if !place(handle, nominal_width, nominal_height) { + return false; + } + + // Captured *before* the show, and that ordering is the whole point: the + // show is what takes the keyboard, so reading the holder afterwards finds + // the card itself and the restore below becomes a no-op. Measured on the + // development box — that was this module's first version, and the card + // kept the keyboard. + let keyboard_holder = crate::keyboard::keyboard_holder(); + + if handle.show().is_err() { + return false; + } + + // The window exists from here on, so the style can be applied to it. Done + // after `show` rather than before because a window that has not been + // created has no native handle to change, and Slint creates it on that + // call. + crate::keyboard::apply(crate::keyboard::WINDOW_TITLE, keyboard_holder); + + // The second placement, once the card has a size. + // + // The nominal height above is only ever an estimate — laying the text + // out is what turns it into the answer — so the position the person + // actually sees settled is this one. It cannot be done inline: a window + // that has just been shown has not been laid out yet, so + // `measured_size` would read zero. A posted timer is the wait without + // blocking, and the event loop is exactly what does the layout. + // + // The card shows at the nominal placement for that one frame and then + // moves by however much the real height differs, which is a fraction of + // the card moving, not a card appearing in the wrong corner. + let correction = slint::Timer::default(); + correction.start(slint::TimerMode::SingleShot, MEASURE_DELAY, move || { + // The closure holds the same strong reference the caller had, so + // the window is alive whatever the event loop did to the + // component in between. + // Measured, so physical already: no conversion, and that is the + // whole reason `place` takes pixels rather than a scale. + let handle = window.window(); + if let Some((width, height)) = measured_size(handle) { + place(handle, width, height); + } + }); + // Parked in the surface rather than dropped: `Timer` stops when it is + // dropped, and this is the one timer that has to outlive the call that + // armed it. Replacing a previous one is intentional — the card whose + // text the old correction was measured against is gone. + *surface.correction.borrow_mut() = Some(correction); + + true + }) +} + +/// Hides the floating surface and empties it. +/// +/// Also cancels a pending correction: a placement armed for a card that is no +/// longer showing would move the next suggestion's window before that card had +/// text of its own. +/// +/// Emptied as well as hidden: a summary left behind is a piece of text the +/// person is about to be asked to type, waiting to reappear in front of buttons +/// that belong to a different suggestion. +pub fn hide() { + with_surface(|surface| { + // Dropped rather than only stopped: the placement it was going to make + // belongs to the text that is about to be cleared. + *surface.correction.borrow_mut() = None; + + let window = surface + .window + .borrow() + .as_ref() + .map(ComponentHandle::clone_strong); + let Some(window) = window else { + return; + }; + let _ = window.window().hide(); + window.set_summary(SharedString::new()); + window.set_why(SharedString::new()); + window.set_deny_consequence(SharedString::new()); + window.set_expires_at(SharedString::new()); + }); +} + +/// Records the floating window this run owns, taking it off the caller. +/// +/// Takes the component rather than a reference because the surface is its only +/// long-lived owner: after the first `Later` hides the card, a handle owned by a +/// binding in `main` would be the thing deciding whether it can come back. +pub fn adopt(window: SuggestionWindow) { + with_surface(|surface| *surface.window.borrow_mut() = Some(window)); +} + +#[cfg(test)] +mod tests { + use super::{is_floating_kind, CARD_WIDTH}; + + /// The brain's own kind is the floating one. + #[test] + fn a_suggestion_is_the_floating_kind() { + assert!(is_floating_kind("input_suggestion")); + } + + /// A decision about running something is not. + /// + /// This is the whole discrimination: the panel's card is where a + /// deliberate answer belongs, and a twenty-five-second suggestion is not + /// one. + #[test] + fn other_kinds_stay_on_the_panel() { + assert!(!is_floating_kind("run_tests")); + assert!(!is_floating_kind("")); + assert!(!is_floating_kind("input_suggestion_extra")); + } + + /// A kind that refines the suggestion stays on the floating surface. + /// + /// The vocabulary is the brain's, and this side should not need a second + /// edit every time the brain grows a word. + #[test] + fn a_refined_suggestion_kind_is_still_floating() { + assert!(is_floating_kind("input_suggestion.quick")); + } + + /// The width Rust places the card at is the width the card asks for. + /// + /// These are two numbers in two files — one in the Slint component, one + /// here — and a drift between them is a card that is placed as if it were + /// narrower than it is, which puts its right edge in the middle of the + /// screen. + #[test] + fn the_placed_width_matches_the_card() { + assert_eq!(CARD_WIDTH, 360); + } +} diff --git a/ui/src/workarea.rs b/ui/src/workarea.rs new file mode 100644 index 0000000..00ba0b1 --- /dev/null +++ b/ui/src/workarea.rs @@ -0,0 +1,149 @@ +//! The platform half of placement: where the screen's usable area is. +//! +//! [`placement::bottom_right`] decides *where in* the usable area the card goes; +//! this module finds the usable area. The two are separated because only the +//! second one is a platform question, and only the first one is arithmetic worth +//! testing. +//! +//! Why the work area and not the monitor. A monitor is the screen. The work +//! area is the screen minus the taskbar, the docks, and whatever else the +//! desktop has decided to reserve. A card placed against the bottom of the +//! *monitor* on a machine with a taskbar is a card under the taskbar: visible in +//! a screenshot, unreachable in life. +//! +//! Why the platform at all. Neither Slint nor winit exposes a work area. Slint +//! exposes a window's own position and size; winit exposes monitors, which are +//! screens. The one rectangle that means "a window may go here" is the platform's +//! to answer, and on Windows that is `GetMonitorInfoW`. +//! +//! The lookup here is hand-declared Win32 rather than a crate dependency, for +//! the same reason `startup.rs` declares `MessageBoxW` by hand: `windows-sys` is +//! already in the dependency graph underneath Slint, so taking it directly would +//! compile no new code, but it would add a dependency line to a `Cargo.toml` +//! that argues, at length, for having none beyond the renderer. Two `extern` +//! declarations are a smaller thing than that. + +use crate::placement::Rect; + +/// The work area of the monitor a window is on, in physical pixels. +/// +/// Returns `None` when the platform cannot say — which is the honest answer on +/// a headless session, and the answer that makes the caller fall back to the +/// panel's own card rather than guessing at a corner. +#[cfg(windows)] +pub fn work_area() -> Option { + windows::work_area() +} + +/// The work area, off Windows: unknown, and said so. +/// +/// `None` rather than the whole screen, and the difference matters. Returning the +/// screen would be a guess that a desktop has no panels — true on a bare Xvfb, +/// false on every real Linux desktop, where the card would then open under the +/// GNOME dash or the KDE panel and be unreachable. `None` means "this platform +/// cannot be asked", which the caller turns into the panel's own card: a +/// suggestion a person can find, rather than a plausible number that puts it +/// somewhere they cannot. +/// +/// Implementing it is a real piece of work on X11 (read `_NET_WORKAREA` off the +/// root window) and on Wayland it is not permitted at all, so this is honest for +/// now rather than provisionally wrong. +#[cfg(not(windows))] +pub fn work_area() -> Option { + None +} + +/// The Win32 calls that answer the question. +#[cfg(windows)] +mod windows { + use std::ffi::c_void; + + use crate::placement::Rect; + + // Both live in `user32.dll`, which every Windows GUI process already links. + unsafe extern "system" { + /// The rectangle of a display, and the rectangle within it that a window + /// is allowed to occupy. + /// + /// `rcMonitor` is the whole display; `rcWork` is the display minus the + /// taskbar and any other reserved area. `rcWork` is the one this needs: + /// it is the rectangle a floating card belongs in. + fn GetMonitorInfoW(monitor: *mut c_void, info: *mut MonitorInfo) -> std::ffi::c_int; + /// The monitor nearest a point, or the primary one. + fn MonitorFromPoint(point: Point, flags: u32) -> *mut c_void; + } + + /// A point in physical screen coordinates. + #[repr(C)] + #[derive(Clone, Copy)] + struct Point { + x: std::ffi::c_long, + y: std::ffi::c_long, + } + + /// A rectangle in physical screen coordinates. + #[repr(C)] + #[derive(Clone, Copy, Default)] + struct Rect32 { + left: std::ffi::c_long, + top: std::ffi::c_long, + right: std::ffi::c_long, + bottom: std::ffi::c_long, + } + + /// What `GetMonitorInfoW` fills in. The size must be set by the caller, and + /// the layout must match the platform's exactly — it is a struct, not a + /// sequence of fields. + #[repr(C)] + struct MonitorInfo { + cb_size: u32, + rc_monitor: Rect32, + rc_work: Rect32, + dw_flags: u32, + } + + /// `MONITOR_DEFAULTTOPRIMARY`: fall back to the primary monitor when no + /// monitor contains the point. + const MONITOR_DEFAULTTOPRIMARY: u32 = 1; + + /// The work area of the primary monitor. + /// + /// The origin is the top-left of the virtual screen, so a monitor to the + /// left of the primary has a negative `left` — and the rectangle is + /// returned in that same coordinate space, which is what `set_position` + /// expects. + pub fn work_area() -> Option { + let mut info = MonitorInfo { + cb_size: std::mem::size_of::() as u32, + rc_monitor: Rect32::default(), + rc_work: Rect32::default(), + dw_flags: 0, + }; + + // SAFETY: the monitor handle comes from `MonitorFromPoint` and is used + // only as the opaque handle the platform documented; `info` is a local + // of exactly the size and layout the call fills in, and `cb_size` tells + // it so. The call cannot retain either pointer. + let ok = unsafe { + let monitor = MonitorFromPoint(Point { x: 0, y: 0 }, MONITOR_DEFAULTTOPRIMARY); + GetMonitorInfoW(monitor, &mut info) + }; + if ok == 0 { + return None; + } + + let work = info.rc_work; + let width = work.right.saturating_sub(work.left); + let height = work.bottom.saturating_sub(work.top); + if width <= 0 || height <= 0 { + return None; + } + + Some(Rect { + x: work.left as i32, + y: work.top as i32, + width: width as u32, + height: height as u32, + }) + } +} diff --git a/ui/suggestion.slint b/ui/suggestion.slint new file mode 100644 index 0000000..5885fe1 --- /dev/null +++ b/ui/suggestion.slint @@ -0,0 +1,151 @@ +import { Button } from "std-widgets.slint"; +import { Theme } from "theme.slint"; + +// The floating surface a proactive input suggestion arrives on. +// +// A suggestion is the one prompt whose value depends on *when* it is seen: it +// names text for the field the person is typing in right now, so it is useful +// exactly while they are somewhere else — and the resident panel is a window +// they put away hours ago. A card inside a hidden window is a suggestion +// nobody receives, which is why this kind alone leaves the panel: it gets its +// own frameless, always-on-top window that appears at the corner of the work +// area instead of at the top of a 640×780 window they have to find and raise. +// +// Every other prompt kind stays on the panel's `ConfirmationCard`. Those are +// decisions about running things, and a resident window is where a deliberate +// decision belongs; a suggestion that vanishes in twenty-five seconds is not +// one. `main.rs` decides which surface a prompt lands on — this file only +// renders one, and hides the lines the wire left empty rather than inventing +// their content. +// +// The heading is this surface's own sentence, not the wire's `kind` token. The +// panel card can say "Squid wants to run a command" because its kinds are +// verbs; "input_suggestion" is a protocol word, and printing it verbatim +// would put the vocabulary of `docs/panel-protocol.md` in front of a person. +// The summary beside it still is, and says, exactly what would be typed. +// +// The three answers match the panel card's — Decline, Later, and the accept — +// with no default and no primary button, for the measured reason that file +// records: a prominent default moves consent, and a prompt that can be +// approved without reading it is the thing this surface must never be. The +// accept button names its own consequence ("Type it") rather than the +// generic "Allow", because for once the action is the same for every prompt +// that can appear here. +export component SuggestionWindow inherits Window { + in property summary; + in property why; + in property deny-consequence; + in property expires-at; + + // Routed to the same `answer` path the panel's buttons use, so a suggestion + // can only ever be settled by the one pending-id mechanism. + callback allow(); + callback decline(); + callback defer(); + + // Distinct from the panel's "Squid": the smoke host finds windows by exact + // title, and a second window answering to the same name would make its + // lookups ambiguous. + title: "Squid suggestion"; + always-on-top: true; + // Frameless because this is a card that floats, not a window to manage: a + // title bar would spend a third of the card's height on chrome and hand the + // windowing system a minimize button that a twenty-five-second prompt has + // no use for. `main.rs` handles a close request as Later, so going + // frameless cannot strand an unanswered decision either. + no-frame: true; + // Transparent so the rounded card below is the window's whole silhouette; + // the corners would otherwise draw the window background as square. + background: transparent; + preferred-width: 360px; + default-font-size: 13px; + + Rectangle { + background: Theme.surface; + border-radius: Theme.radius; + border-width: 1px; + border-color: Theme.line; + drop-shadow-blur: 24px; + drop-shadow-offset-y: 4px; + drop-shadow-color: Theme.shadow; + + accessible-role: text; + accessible-label: "Suggestion from Squid"; + accessible-description: root.summary; + + VerticalLayout { + padding: Theme.pad; + spacing: Theme.gap; + + Text { + text: "Squid suggests"; + color: Theme.text-muted; + font-size: 12px; + font-weight: 700; + } + + // What would be typed. Monospace, as on the panel card: the exact + // spelling is the whole content of the decision, and it is capped + // the same way so the buttons below survive any summary length. + Rectangle { + background: Theme.surface-sunken; + border-radius: 6px; + VerticalLayout { + padding: Theme.gap; + Text { + text: root.summary == "" ? "No details were provided." : root.summary; + color: Theme.text; + font-family: "monospace"; + font-size: 12px; + wrap: word-wrap; + max-height: 96px; + overflow: elide; + } + } + } + + if root.why != "": Text { + text: root.why; + color: Theme.text-muted; + font-size: 12px; + wrap: word-wrap; + } + + if root.deny-consequence != "": Text { + text: "If you decline: " + root.deny-consequence; + color: Theme.text-muted; + font-size: 12px; + wrap: word-wrap; + } + + Text { + text: root.expires-at == "" ? "This suggestion has no deadline." + : "Expires at " + root.expires-at; + color: Theme.caution; + font-size: 12px; + wrap: word-wrap; + } + + HorizontalLayout { + spacing: Theme.gap; + alignment: end; + + Button { + text: "Decline"; + accessible-description: "Do not type anything. The focused field is left as it is."; + clicked => { root.decline(); } + } + Button { + text: "Later"; + accessible-description: "Put this suggestion aside without answering. It stays pending on the panel until it expires."; + clicked => { root.defer(); } + } + Button { + text: "Type it"; + accessible-description: "Type the suggested text into the field that was focused."; + clicked => { root.allow(); } + } + } + } + } +} diff --git a/ui/theme.slint b/ui/theme.slint index f035276..d2df9dc 100644 --- a/ui/theme.slint +++ b/ui/theme.slint @@ -37,6 +37,18 @@ export global Theme { out property caution: dark ? #f0c274 : #8a5a00; out property danger: dark ? #ffb3a7 : #b3261e; + // The floating card's separation from whatever is underneath it. DESIGN.md + // allows an "offset soft shadow only where FemtoVG rendering demonstrates it + // remains legible", and this is a case it did not anticipate: the suggestion + // card is frameless over an arbitrary desktop, so without a shadow it is a + // rectangle that could belong to whichever window is behind it. + // + // A token rather than a component literal, for the same reason the status + // colours are tokens — a shadow dark enough to read on a white desktop is a + // smudge on a dark one, so it has to answer the scheme like everything else + // here does. + out property shadow: dark ? #00000099 : #0000003d; + // A spacing scale, so a gap is chosen from a set rather than typed. The old // panel used one 10px gap for everything, which is why a section header, its // rows, and the next section all sat the same distance apart and nothing From 076d114559a10d5943ef6c1a8f38d2ebb4b58bdd Mon Sep 17 00:00:00 2001 From: Shangxin Date: Fri, 23 Oct 2026 21:29:54 +0800 Subject: [PATCH 2/2] fix(ui): take the floating card down when the panel is already gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `answer`, `defer` and `cancel` each hid the floating card from inside either an `if let Some(panel) = panel.upgrade()` or a `panel.upgrade_in_event_loop` closure, which made the card's removal conditional on the panel still being alive. They are separate top-level windows and it is not. The case they diverge is the one the tray's Quit callback walks: it answers the pending prompt and then ends the event loop, so the answer is written and the card is left on screen holding buttons for an id that has already been spent. A click on it finds an empty slot and returns, which is precisely the dead surface `answer`'s take-the-id-out exists to prevent. All three now hide the surface first, on the UI thread they are already on, and only then go looking for the panel to update. `cancel` also passes `clear_prompt` directly rather than wrapping it in a closure that no longer captures anything. Found by reading the callbacks back after the smoke went green — the harness does not cover a panel that closes mid-prompt, and the tray quit path is the one place in `main.rs` that answers and then stops. Verified: `cargo clippy --all-targets` clean, `cargo test` 104 passed, `bun test` 826 passed / 23 skipped / 0 failed, `bun run ui:smoke` green. --- ui/src/main.rs | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/ui/src/main.rs b/ui/src/main.rs index 47a3b65..cd3265c 100644 --- a/ui/src/main.rs +++ b/ui/src/main.rs @@ -1265,10 +1265,12 @@ fn cancel(panel: &slint::Weak, pending: &Pending, prompt_id: i64) { state.current = None; state.deferred = None; } - let _ = panel.upgrade_in_event_loop(|panel| { - suggestion::hide(); - clear_prompt(panel); - }); + // Outside the closure for the reason the other two give: the floating card + // is not the panel's to keep alive, and a cancel that arrives after the panel + // is gone must still take the card down. + suggestion::hide(); + + let _ = panel.upgrade_in_event_loop(clear_prompt); } /// Writes one answer for the pending prompt, then closes both surfaces. @@ -1307,8 +1309,21 @@ fn answer( // consumed here would be the worse surface. outgoing.send(&answer_line(prompt_id, answer)); + // Hidden before the panel is looked for, and outside that lookup on purpose: + // the floating card is a window of its own, so it can outlive the panel's + // strong reference. Leaving the hide inside `if let Some(panel)` made the two + // coupled, and the case they diverge — the tray's Quit, which answers and + // then ends the event loop — would leave the card on screen showing buttons + // for a prompt whose id has already been spent. A click on it would then do + // nothing at all, which is the exact failure `answer` exists to make + // impossible. + // + // Safe to call from here: every caller of this function is a Slint callback + // or a posted closure, so this already is the UI thread, and + // `suggestion::hide` requires nothing more than that. + suggestion::hide(); + if let Some(panel) = panel.upgrade() { - suggestion::hide(); clear_prompt(panel); } } @@ -1335,8 +1350,13 @@ fn defer(pending: &Pending, panel: &slint::Weak) { }; state.deferred = Some(current); } + // Outside the posted closure, for the same reason `answer` hides there: the + // card must go away even when there is no panel left to post to, or a + // closed panel leaves a floating window asking a question nobody can see the + // answer to anywhere. + suggestion::hide(); + let _ = panel.upgrade_in_event_loop(|panel| { - suggestion::hide(); // One rendering of a deferred prompt, not two: the notice replaces the // card, so the card's buttons go away with it. panel.set_prompt_floating(false);