diff --git a/plans/reports/brainstormer-260518-0917-offscreen-bubble-recovery.md b/plans/reports/brainstormer-260518-0917-offscreen-bubble-recovery.md new file mode 100644 index 0000000..047c1be --- /dev/null +++ b/plans/reports/brainstormer-260518-0917-offscreen-bubble-recovery.md @@ -0,0 +1,80 @@ +# Brainstorm: Off-Screen Bubble Recovery + +## 1. Recovery strategies ranked + +### RECOMMEND — Layered: validate on load + clamp on create + +**A. Validate position in `settings::load`** (primary defense) +- After deserialize, walk `bubble_positions`. For each `Some((x,y))`, build a probe rect `(x, y, x+min_w, y+min_h)` and check via `MonitorFromRect(... MONITOR_DEFAULTTONULL)`. If null → set to `None`. +- One pass, ~15 lines, runs before any window code sees the value. +- Pro: KISS, no race with `ShowWindow`, fixes related bugs (hand-edited JSON, dpi-changed coords). + +**B. Clamp on create** (defense-in-depth, kept as proposed) +- After `CreateWindowExW`, before `ShowWindow`, call `clamp_into_work_area(hwnd)`. +- Catches monitor unplug **between** `load()` and `create()` (rare but possible: laptop closed mid-startup). +- Cost: one extra call, idempotent. + +Both together are the right answer. Neither alone covers all cases. + +### CONSIDER — Visual cue + +**C. Tray balloon "Widget repositioned to primary monitor"** +- Only when validator actually relocated. Uses existing `Shell_NotifyIconW NIF_INFO`. ~20 lines. +- Risk: balloon spam on dock/undock cycles. Fire only when *saved* position was killed. + +### AVOID + +**D. Topology fingerprint** — overkill, doesn't preserve intent better than (A). +**E. Per-monitor relative pinning** — future feature, not a fix. YAGNI. + +## 2. Trade-off matrix + +| Approach | Preserves intent on replug | Surprise on cold start | LOC | Risk | +|----------|---------------------------|------------------------|-----|------| +| A (validate-on-load) | No — wipes saved coord | Low | ~15 | None | +| B (clamp-on-create) | Partial — moves to nearest edge | Low | ~3 | None | +| A+B | No | Low | ~18 | None | +| A+B+C | Same + explains itself | Very low | ~38 | Balloon fatigue | +| D (topology hash) | Yes if same monitor before next launch | Medium | ~80 | Maintenance | +| E (relative pin) | Yes | Medium | ~150 | Premature | + +## 3. Edge cases proposed fix misses + +1. **Saved-pos monitor asleep / no input** — `MonitorFromRect` still returns handle; A+B no-op. Correct. +2. **DPI change while app closed** — px coords technically valid by topology; A passes, B no-op. Acceptable. +3. **Negative-coord monitors (secondary left of primary)** — validator MUST use `MONITOR_DEFAULTTONULL`, not `DEFAULTTONEAREST` (would silently snap valid secondary coord to primary). +4. **Dual bubbles overlap after relocate** — both clamped to bottom-right of primary → stacked. `default_position` staggers Codex; clamp doesn't. Minor. +5. **User drags to secondary, unplugs, restarts** — A+B: bubble at primary default; saved pos destroyed. No recovery on replug. Acceptable for v1. +6. **Dock-daily multi-monitor user** — every undock wipes pos; every dock back gives default. Annoying. Case where E would win. Punt unless reported. + +## 4. Logging strategy (minimal) + +On the visibility-affecting path only: + +- `info`: `bubble create model={} pos=({},{}) size={}x{} dpi={}` — one line per bubble at create. +- `warn`: `bubble position ({},{}) outside all monitors, resetting to default` — fires in validator. **This is the line that would have solved this bug in 5 seconds.** +- `warn`: `clamp_into_work_area moved bubble from ({},{}) to ({},{})` — fires on create-time clamp. +- `debug`: monitor enumeration on startup. + +Skip: per-render logs, drag logs, timer ticks. + +## 5. "Reset position" discoverability — secondary + +Menu item exists but buried. If A+B work, this path is unreachable. Don't add a "Reset position" balloon prompt — confirmation fatigue. Just fix silently and the §4 warn + §C balloon explain it once. + +## Recommended action + +1. Add `BubblePositions::validate(&mut self)` called from `settings::load`. Use `MonitorFromRect(... MONITOR_DEFAULTTONULL)` with `(x, y, x+MIN_BUBBLE_SIZE, y+MIN_BUBBLE_SIZE)`. Set to `None` on miss. Log `warn`. +2. Call `clamp_into_work_area(hwnd)` in `bubble::create` between `CreateWindowExW` and `ShowWindow`. Log `warn` on movement. +3. Add the 4 log lines from §4. +4. Single tray balloon "Widget repositioned: previous monitor not connected" once per launch when validator killed any saved position. +5. Defer monitor-index pinning (E) and topology hash (D). + +Total: ~40 lines, one new function, two log statements, one balloon call. + +## Unresolved questions + +- Balloon (item 4): opt-in or always-on? Default always-on. +- `MIN_BUBBLE_SIZE` as probe rect, or account for current `bubble_size_logical`? Min safer. +- Re-attempt last-known coord on replug? Probably no — YAGNI. +- Codex/Claude stagger preservation on auto-relocate? Currently they'd stack. Worth fixing in same patch? diff --git a/plans/reports/code-reviewer-260518-0917-bubble-offscreen-clamp-fix.md b/plans/reports/code-reviewer-260518-0917-bubble-offscreen-clamp-fix.md new file mode 100644 index 0000000..d95fa2d --- /dev/null +++ b/plans/reports/code-reviewer-260518-0917-bubble-offscreen-clamp-fix.md @@ -0,0 +1,60 @@ +# Code Review: Bubble Off-Screen Clamp Fix + +**Scope:** Proposed bug-fix for v0.1.7 "widget enabled but not shown" — saved positions on disconnected monitor. +**Files:** `src/bubble.rs` (create, clamp_into_work_area, set_user_visible, default_position), `src/app.rs` (spawn_bubble, toggle_widget_visibility, reset_positions). + +## Overall Assessment + +**Fix is correct and minimal. Ship it with two small refinements.** Root-cause matches code (verified: `bubble.rs:143-160` passes saved `position` straight into `CreateWindowExW`; `clamp_into_work_area` at `:770` only wired into `WM_SETTINGCHANGE` at `:486`). Approach is the right shape: clamp post-create, pre-show. + +## Critical Issues + +None. + +## High Priority + +1. **Call order — clamp must run BEFORE `render(hwnd)` at `bubble.rs:220`, not just before `ShowWindow` at `:222`.** `render` calls `GetWindowRect` (`:1108`) for the `UpdateLayeredWindow` destination point. If clamp runs after `render`, the first frame paints at the off-screen coords; second paint only happens on next update_data tick. Move `clamp_into_work_area(hwnd)` to between line `:218` (state insert) and `:220` (render). + +## Medium Priority + +2. **`MonitorFromWindow` on a not-yet-shown off-screen window — verified safe.** Win32 sets the window rect immediately at `CreateWindowExW` return (visibility is irrelevant to `GetWindowRect`). With `MONITOR_DEFAULTTONEAREST` and a window whose entire rect lies on a disconnected monitor, the OS computes intersection with each *currently attached* monitor's rect; none intersect → falls back to nearest by Euclidean distance → returns the primary on a single-monitor setup. Saved `[2407,1282]` on a 1920-wide primary → nearest = primary → clamp pulls to `(1920-w, …)`. Correct. + +3. **Multi-monitor edge case is preserved.** If the saved position is on a still-connected secondary, `MonitorFromWindow` returns that secondary monitor and clamps within its work area — no unwanted pull to primary. Good. + +4. **Partial off-screen.** `clamp_into_work_area` only adjusts when fully outside (clamps each axis independently to `[wa.left, wa.right-w]`). A window whose top-left is on-screen but bottom-right spills off → it pulls the whole window inside. Behaviour is fine; matches `snap_to_edge` (`:644-645`). + +5. **DPI mismatch (saved from 4K → 1080p primary):** the saved coords are physical pixels but the new bubble's `width_px/height_px` are recomputed against the *current* primary DPI (`:140-142`). Clamp uses the new size against the new monitor's work area — correct. No DPI bug. + +6. **`default_position` case:** no-op (already inside work area). Safe. + +## Low Priority + +7. **Log levels are appropriate.** `info!` in `create` (fires once per bubble creation), `set_user_visible` (fires only on user-toggle — verified at `app.rs:1152` only called from `toggle_widget_visibility`), and `toggle_widget_visibility` (one event per click). None on the render hot path. Approved. + +8. **Alternative call site (clamp in `app::spawn_bubble`):** Less attractive. `spawn_bubble` doesn't own the HWND lifecycle and would need a fresh `GetWindowRect` round-trip. Keeping the clamp inside `bubble::create` keeps the bubble module the sole owner of window geometry and means future call sites (e.g. tests, a hypothetical re-create-on-DPI-change) also benefit for free. The "bubble module stays position-agnostic" argument is weak — it already calls `default_position`, `snap_to_edge`, and `clamp_into_work_area`. Position-aware is the status quo. + +## Side Effects + +- No callers of `bubble::create` assert the returned HWND is at the exact requested coords. `app::spawn_bubble` (`:277`) ignores position post-create; `reset_positions` (`:1156`) destroys + recreates. Safe. +- `position(hwnd)` (`:322`) reads live `GetWindowRect`, so any subsequent `on_bubble_moved` save reflects the clamped coords — this self-heals the persisted bad value on first drag. + +## Positive Observations + +- Clamp helper already exists and is correct (`:770-809`). +- Fix is one line + three log statements; minimal blast radius. +- Persisted-corruption auto-heal via first interaction is a nice property. + +## Recommended Actions + +1. **MUST:** Place `clamp_into_work_area(hwnd)` between `lock_bubbles().insert(...)` (`:218`) and `render(hwnd)` (`:220`) — not after `render`. +2. **SHOULD:** Add an `info!` in `clamp_into_work_area` that fires only when `nx != r.left || ny != r.top` (i.e. the actual reposition path). Free diagnostic for future "bubble moved itself" reports. +3. **CONSIDER:** Persist the clamped position immediately after `create` so `settings.json` is self-healed on next launch, not only after a drag. Trade-off: writes settings on every startup; current behaviour writes only on user action. Probably YAGNI — drift gets repaired on first interaction. + +## Unresolved Questions + +- Should we also persist the corrected position eagerly (action 3)? Default to no per YAGNI; flag for user. +- Does Windows ever defer `CreateWindowExW` window-rect commit until `ShowWindow`? Per MSDN and verified by existing `snap_to_edge` using the same pattern in `WM_EXITSIZEMOVE`, no — rect is committed synchronously. + +**Status:** DONE_WITH_CONCERNS +**Summary:** Fix is correct and small. One ordering bug: clamp must precede `render`, not just `ShowWindow`, otherwise the first paint targets the off-screen coords. +**Concerns:** Action 1 (clamp before render) is a real correctness issue — the proposal as written ("after CreateWindowExW succeeds and before ShowWindow") technically permits ordering after `render`, which would defeat the fix until the next data update.