Review everything shipped this weekend, and fix what the reviewers caught

Claude-Session: https://claude.ai/code/session_01Ms2FbjQy31TVf3CEvQhGM8
2026-08-24 13:29:42 -04:00
parent ffce48964e
commit 07db1068f1
42 changed files with 959 additions and 219 deletions
@@ -129,16 +129,34 @@ PanelWindow {
        onTriggered: root.revealed = false
    }

-    // Other modules (the bar, the capture overlay) read this. It is one
-    // shared flag but there is one Dock per monitor, so only the instance on
-    // the currently-focused monitor is allowed to write it -- otherwise
-    // whichever instance last changed reveal state would stomp the others,
-    // and a reader would see an arbitrary monitor's value. This scopes the
-    // flag to mean "is the dock revealed on the monitor the user is on",
-    // which is what a capture overlay or the bar actually care about.
-    // (A true per-monitor flag would need ShellState.dockRevealed itself to
-    // become keyed by screen, which is out of scope here -- see the report.)
-    readonly property bool isFocusedMonitorInstance: root.monitor === null || root.monitor === Hyprland.focusedMonitor
+    // ShellState.dockRevealed is one shared flag and there is one Dock per
+    // monitor, so exactly one instance may write it -- otherwise whichever
+    // instance last changed reveal state stomps the others and a reader gets an
+    // arbitrary monitor's answer. Scoped this way the flag means "is the dock
+    // revealed on the monitor the user is on", which is the only question a
+    // reader outside the dock can sensibly ask of it. (A genuinely per-monitor
+    // answer would need the property itself keyed by screen.)
+    //
+    // Two things disqualify an instance. One is not being on the focused
+    // monitor, which is the whole point. The other is not being on screen at
+    // all: a Dock whose screen is not in `dockScreens` is invisible, and an
+    // invisible dock reporting itself as revealed is a lie a reader acts on. An
+    // instance whose monitor could not be resolved -- created standalone, or
+    // asked before Hyprland has reported the screen -- knows nothing about
+    // which monitor the user is on, so it writes only when there is no focused
+    // monitor to be wrong about rather than stomping the instance that knows.
+    //
+    // Nothing reads the flag today: the bar and the capture overlay it was
+    // written for both stopped. It is written correctly rather than left
+    // half-wrong, and when ShellState is next opened the property, this
+    // arbitration and _syncShellState should go together.
+    readonly property bool isFocusedMonitorInstance: {
+        if (!root.onThisScreen)
+            return false;
+        if (root.monitor !== null)
+            return root.monitor === Hyprland.focusedMonitor;
+        return Hyprland.focusedMonitor === null;
+    }

    onRevealedChanged: root._syncShellState()
    onIsFocusedMonitorInstanceChanged: root._syncShellState()
@@ -183,6 +201,33 @@ PanelWindow {
        Item {
            id: maskItem

+            // Where the body comes to rest once the slide finishes, mirroring
+            // DockBody's own x/y bindings in their revealed case.
+            //
+            // The revealed region is built from these rather than from the
+            // body's live position. The body slides in over a couple of hundred
+            // milliseconds, and a region that follows it in is a region that is
+            // a few pixels tall on the frame the dock is summoned -- the exact
+            // frame the pointer that summoned it needs to be inside, and the
+            // exact frame it drops the hover and sends the dock back. Following
+            // the animation also means an input-region commit to the compositor
+            // on every one of those frames, for a rectangle that is only right
+            // on the last of them.
+            //
+            // Clamped rather than assigned outright so the mask still tracks
+            // the body if anything else ever moves it: the reveal only ever
+            // approaches these values from outside the screen edge.
+            readonly property real settledX: {
+                if (!root.vertical)
+                    return body.x;
+                return root.position === "left"
+                    ? Math.max(body.x, root.tooltipSpace)
+                    : Math.min(body.x, surface.width - body.width - root.tooltipSpace);
+            }
+            readonly property real settledY: root.vertical
+                ? body.y
+                : Math.min(body.y, root.tooltipSpace)
+
            x: {
                if (!root.revealed)
                    return root.position === "right" ? surface.width - root.revealStripHeight : 0;
@@ -194,25 +239,27 @@ PanelWindow {
                // to its own edge, which is x 0 on the left and the body on
                // the right.
                if (!root.vertical)
-                    return body.x;
-                return root.position === "right" ? body.x : 0;
+                    return maskItem.settledX;
+                return root.position === "right" ? maskItem.settledX : 0;
            }
            y: {
                if (!root.revealed)
                    return root.vertical ? 0 : surface.height - root.revealStripHeight;
-                return body.y;
+                return maskItem.settledY;
            }
            width: {
                if (!root.revealed)
                    return root.vertical ? root.revealStripHeight : surface.width;
                return root.vertical
-                    ? (root.position === "right" ? surface.width - body.x : body.x + body.width)
+                    ? (root.position === "right"
+                        ? surface.width - maskItem.settledX
+                        : maskItem.settledX + body.width)
                    : body.width;
            }
            height: {
                if (!root.revealed)
                    return root.vertical ? surface.height : root.revealStripHeight;
-                return root.vertical ? body.height : surface.height - body.y;
+                return root.vertical ? body.height : surface.height - maskItem.settledY;
            }
        }

@@ -226,7 +273,7 @@ PanelWindow {
                body.dismissPreview();
                dockContextMenu.anchorItem = anchorItem;
                dockContextMenu.app = app;
-                dockContextMenu.visible = true;
+                dockContextMenu.requested = true;
            }

            vertical: root.vertical
@@ -284,6 +331,33 @@ PanelWindow {
        id: dockContextMenu
    }

+    // The safety net for a latched interaction flag.
+    //
+    // Every term of `interactionHeld` is cleared by an event that can go
+    // missing: a drag by its release, which a stolen grab eats; a preview by
+    // the pointer leaving, which a destroyed anchor never reports; a menu by a
+    // row being chosen, which a menu whose anchor died is never offered. Any
+    // one of them left set holds the dock revealed with the pointer nowhere
+    // near it, and there is no gesture that gets it back -- the flag is stuck,
+    // so the dock is out until the shell restarts.
+    //
+    // Eight seconds of held-but-untouched is not a gesture. The pointer being
+    // on the previews or on the menu counts as touched, because both are their
+    // own surfaces and leaving the dock is how you reach them.
+    Timer {
+        id: interactionWatchdog
+
+        interval: 8000
+        running: root.interactionHeld && !pointer.hovered
+            && !dockPreviews.hovered && !dockContextMenu.hovered
+
+        onTriggered: {
+            body.cancelDrag();
+            body.dismissPreview();
+            dockContextMenu.requested = false;
+        }
+    }
+
    // Its own surface rather than something drawn inside the dock: the dock's
    // input mask is a thin strip when hidden and the bar's own rectangle when
    // shown, and widening it to cover a preview would hand the dock every
@@ -248,13 +248,34 @@ Rectangle {
    // are their own surface: leaving the icon to reach them would otherwise
    // close the thing being reached for.
    property Item previewAnchor: null
-    property var previewApp: null
+
+    // Looked up in the live model rather than snapshotted when the dwell fires.
+    // `items` is rebuilt into fresh objects whenever any window opens or
+    // closes, so a snapshot keeps the window list the app had when the pointer
+    // stopped moving: a window closed while its preview is up stays in the
+    // strip, and its ScreencopyView goes on holding a handle to a surface that
+    // no longer exists. Reading it back out of `items` means the strip empties
+    // itself, and an app whose last window closed drops the preview entirely.
+    readonly property var previewApp: {
+        const anchor = root.previewAnchor;
+        if (!anchor || !anchor.app)
+            return null;
+        const id = anchor.app.appId;
+        for (let i = 0; i < root.items.length; i++) {
+            if (root.items[i].appId === id)
+                return root.items[i];
+        }
+        return null;
+    }

    // Written by the Dock from the preview popup's own hover.
    property bool previewHovered: false

    onHoveredItemChanged: root.reconsiderPreview()
    onPreviewHoveredChanged: root.reconsiderPreview()
+    // A window opening or closing changes what the strip should be showing --
+    // including, when it was the last one, whether there should be a strip.
+    onItemsChanged: root.reconsiderPreview()

    function reconsiderPreview(): void {
        const item = root.hoveredItem;
@@ -277,7 +298,6 @@ Rectangle {
        previewDwell.stop();
        previewGrace.stop();
        root.previewAnchor = null;
-        root.previewApp = null;
        root.previewHovered = false;
    }

@@ -289,7 +309,6 @@ Rectangle {
            if (!item || !item.app || !item.app.windows || item.app.windows.length === 0)
                return;
            root.previewAnchor = item;
-            root.previewApp = item.app;
        }
    }

@@ -375,6 +394,13 @@ Rectangle {
                onDragMoved: travel => root.moveDrag(travel)
                onDragEnded: root.endDrag()
                onDragCancelled: root.cancelDrag()
+
+                // Keying the model keeps a delegate alive through a rebuild,
+                // but not through the app's last window closing while it is
+                // being dragged: that row is gone, and the release it owed is
+                // gone with it. Without this the dock keeps dragIndex forever,
+                // which reads as a dock that will not hide again.
+                Component.onDestruction: if (root.dragIndex === dockItem.index) root.cancelDrag()
            }
        }
    }
@@ -32,9 +32,24 @@ PopupWindow {
    implicitWidth: Math.min(360, Math.max(menu.implicitWidth + Theme.popoverPadding * 2, 240))
    implicitHeight: menu.implicitHeight + Theme.popoverPadding * 2
    color: "transparent"
-    visible: false
    grabFocus: true

+    // Whether the dock has asked for the menu. Visibility is that AND a live
+    // anchor, rather than the request alone: the dock icon a menu is anchored
+    // to is a delegate, and a delegate dies when its app's last window closes.
+    // An anchored PopupWindow whose anchor has gone stays mapped with a null
+    // anchor -- a menu hanging over the desktop, attached to nothing, holding
+    // the dock revealed behind it.
+    property bool requested: false
+    visible: root.requested && root.anchorItem !== null
+
+    onAnchorItemChanged: if (root.anchorItem === null) root.requested = false
+
+    // Read by the Dock's interaction watchdog. A menu with the pointer on it is
+    // a menu being read, not a stuck flag -- and the pointer being here means
+    // it is not on the dock, which is the only other thing the dock can see.
+    readonly property bool hovered: menuPointer.hovered
+
    // Long window titles are the one thing here that can be arbitrarily wide,
    // and a menu as wide as a browser tab's title is not a menu.
    function shortTitle(toplevel: var): string {
@@ -99,6 +114,10 @@ PopupWindow {
        border.width: 1
        border.color: Theme.alpha(Theme.fg, 0.08)

+        HoverHandler {
+            id: menuPointer
+        }
+
        PrismEdge {
            anchors.top: parent.top
            anchors.topMargin: 1
@@ -125,7 +144,7 @@ PopupWindow {
                    label: root.shortTitle(modelData)
                    onActivated: {
                        root.focusToplevel(modelData);
-                        root.visible = false;
+                        root.requested = false;
                    }
                }
            }
@@ -151,7 +170,7 @@ PopupWindow {
                    label: modelData.name
                    onActivated: {
                        modelData.execute();
-                        root.visible = false;
+                        root.requested = false;
                    }
                }
            }
@@ -175,7 +194,7 @@ PopupWindow {
                onActivated: {
                    if (root.entry)
                        root.entry.execute();
-                    root.visible = false;
+                    root.requested = false;
                }
            }

@@ -191,7 +210,7 @@ PopupWindow {
                        root.unpin();
                    else
                        root.pin();
-                    root.visible = false;
+                    root.requested = false;
                }
            }

@@ -201,7 +220,7 @@ PopupWindow {
                rowEnabled: root.windows.length > 0
                onActivated: {
                    root.quit();
-                    root.visible = false;
+                    root.requested = false;
                }
            }

@@ -218,7 +237,7 @@ PopupWindow {
                label: "Dock settings"
                onActivated: {
                    ShellState.openSettings("dock");
-                    root.visible = false;
+                    root.requested = false;
                }
            }
        }
@@ -187,6 +187,17 @@ Item {
        }

        onPressed: mev => {
+            // A press arriving while a drag is still marked active means the
+            // last one never got its release -- a grab stolen by a surface that
+            // opened over the dock, most often. Clearing the flag alone would
+            // leave DockBody still holding a dragIndex, and with it the
+            // interaction hold that keeps the dock revealed forever; the drag
+            // has to be cancelled through the same path a stolen grab uses.
+            if (mouse.dragActive) {
+                mouse.dragActive = false;
+                mouse.dragConsumed = false;
+                root.dragCancelled();
+            }
            mouse.pressX = mev.x;
            mouse.pressY = mev.y;
            mouse.dragActive = false;
@@ -167,7 +167,6 @@ Rectangle {
                    activeFocusOnTab: true
                    verticalAlignment: TextInput.AlignVCenter
                    maximumLength: 7
-                    text: String(root.swatchColor).toLowerCase()
                    color: root.invalid ? Theme.danger : Theme.fg
                    selectionColor: Theme.alpha(Theme.accent, 0.4)
                    selectedTextColor: Theme.fg
@@ -178,6 +177,14 @@ Rectangle {
                    Accessible.role: Accessible.EditableText
                    Accessible.name: root.role + " colour, hex"

+                    // Seeded once, then written only by the guarded handlers
+                    // below and by onSwatchColorChanged. A binding here would
+                    // be a second writer for the same text, silently dropped
+                    // the first time one of them assigned to it -- so which of
+                    // the two was in charge depended on what had happened
+                    // earlier in the session.
+                    Component.onCompleted: hexInput.text = String(root.swatchColor).toLowerCase()
+
                    onTextEdited: root.invalid = false
                    onAccepted: root.commit()

@@ -17,8 +17,15 @@ Item {
    implicitWidth: 40
    implicitHeight: 40

+    // A Canvas reads Theme inside onPaint, where nothing records a dependency
+    // on it, so a palette change would leave the ring drawn in the old warn
+    // tone until the next tick -- or for good, on a countdown sitting still.
+    // Naming the colour as a property gives the repaint something to watch.
+    readonly property color ringColor: Theme.warn
+
    onSecondsLeftChanged: ring.requestPaint()
    onTotalSecondsChanged: ring.requestPaint()
+    onRingColorChanged: ring.requestPaint()
    onVisibleChanged: if (root.visible) ring.requestPaint()

    Canvas {
@@ -12,6 +12,7 @@
 // mirroring, rather than drawing it wherever its stale coordinates point.

 import QtQuick
+import Quickshell
 import qs.config
 import qs.widgets
 import "../../services/DisplayLayout.js" as DisplayLayout
@@ -198,7 +199,20 @@ Item {
            }

            Repeater {
-                model: root.tiles
+                // Keyed on the output name rather than fed the array directly.
+                // Every drag step and every arrow key rewrites draftLayout,
+                // which rebuilds `tiles` into a fresh array of fresh objects --
+                // a new model, which resets the Repeater and destroys the tile
+                // being dragged on its first millimetre of travel. The drag
+                // ended there, silently, and nothing was ever applied; the
+                // arrow keys lost focus after a single step for the same
+                // reason. Keyed, the same rebuild is "this row moved", and the
+                // tile under the pointer keeps its handler and its focus.
+                model: ScriptModel {
+                    values: root.tiles
+                    objectProp: "name"
+                    comparisonMode: ObjectComparison.Structure
+                }

                Rectangle {
                    id: tile
@@ -91,7 +91,17 @@ Column {
    property int draggingIndex: -1
    property var workingOrder: []

-    readonly property var displayed: root.draggingIndex >= 0 ? root.workingOrder : root.pinned
+    // The order the strip is drawing, as rows the Repeater can identify. A
+    // plain array of ids is a NEW model every time the order changes, and
+    // reordering is the one thing this strip does: handing that to a Repeater
+    // resets it and destroys the delegate the pointer is holding, mid-press, on
+    // the first cell crossed. The gesture dies there and draggingIndex stays
+    // set, which latches the strip. Keyed on the pin id -- the only thing a pin
+    // is -- a reorder becomes a row move instead, and the delegate under the
+    // hand survives it. One-property objects because ScriptModel identifies a
+    // row by a property of it, which a bare string has none of.
+    readonly property var displayed: (root.draggingIndex >= 0 ? root.workingOrder : root.pinned)
+        .map(id => ({ id: id }))

    function beginDrag(index: int): void {
        root.workingOrder = root.pinned.slice();
@@ -119,12 +129,23 @@ Column {
        root.commit(next);
    }

+    // The drag let go of without a decision. Used when the cell being dragged
+    // is destroyed under the pointer -- an unpin from somewhere else, a
+    // settings file rewritten underneath -- where the working order is a
+    // rearrangement of a list that no longer exists, and writing it back would
+    // undo whatever really happened.
+    function cancelDrag(): void {
+        root.draggingIndex = -1;
+        root.workingOrder = [];
+    }
+
    // ── Keyboard ────────────────────────────────────────────────────────────
    //
-    // The Repeater's model is a plain array, so committing a move rebuilds
-    // every delegate and the focused one is destroyed mid-keystroke. The
-    // focused position is remembered here instead of in the delegate, and the
-    // cell that lands on it takes focus back as it is created.
+    // Which POSITION has the keyboard, remembered here rather than left to the
+    // delegate that happens to hold focus. Unpinning really does destroy the
+    // focused cell, and a keyed model still cannot keep focus on a row that is
+    // gone; remembering the index means the cell that lands on it takes focus
+    // back as it arrives, so Delete twice in a row deletes twice.

    property int keyboardIndex: -1

@@ -159,7 +180,13 @@ Column {
        spacing: root.cellSpacing

        Repeater {
-            model: root.displayed
+            // Keyed on the pin id: see `displayed`. This is the same reason
+            // DockBody keys the dock's own icons.
+            model: ScriptModel {
+                values: root.displayed
+                objectProp: "id"
+                comparisonMode: ObjectComparison.Structure
+            }

            delegate: Item {
                id: cell
@@ -167,9 +194,10 @@ Column {
                required property var modelData
                required property int index

+                readonly property string appId: cell.modelData ? cell.modelData.id : ""
                readonly property bool dragging: root.draggingIndex === cell.index
-                readonly property string appName: root.nameFor(cell.modelData)
-                readonly property string iconSource: root.iconFor(cell.modelData)
+                readonly property string appName: root.nameFor(cell.appId)
+                readonly property string iconSource: root.iconFor(cell.appId)

                width: root.cellSize
                height: root.cellSize
@@ -194,6 +222,12 @@ Column {
                onActiveFocusChanged: if (cell.activeFocus) root.keyboardIndex = cell.index
                Component.onCompleted: if (root.keyboardIndex === cell.index) cell.forceActiveFocus()

+                // A cell that goes away while it is the one being dragged takes
+                // the release with it -- there is no MouseArea left to report
+                // one -- and the strip would sit there with draggingIndex still
+                // set, refusing every later press.
+                Component.onDestruction: if (cell.dragging) root.cancelDrag()
+
                Connections {
                    target: root
                    function onKeyboardIndexChanged(): void {
@@ -1,5 +1,5 @@
 // A slider whose track carries a meaning, and whose value is committed once the
-// drag settles rather than on every pixel.
+// gesture is over rather than on every pixel.
 //
 // Two rows on the Displays page need something SliderRow cannot give them:
 //
@@ -8,7 +8,14 @@
 //     like, and this is the one slider whose numbers most people cannot picture.
 //   * The SDR trims, which write through the display transaction. A transaction
 //     per pixel of drag would arm a fifteen-second countdown dozens of times;
-//     the value is applied once, when the pointer stops moving.
+//     the value is applied once, when the button comes up.
+//
+// On release, not on an idle timer. Holding a slider still for a third of a
+// second is not the end of a gesture, and the commit an idle timer made
+// mid-hold armed a display transaction that turned this row's `enabled` false
+// with the button still down -- the control went dead under the hand using it.
+// The wheel has no release, so it keeps the timer, and that is all the timer is
+// for now.
 //
 // Not schema-bound and not a SettingRow: the caller says what the value is and
 // what to do with a new one, and the row stacks its control below the label in
@@ -59,6 +66,16 @@ Item {
        return colors[Math.min(index, colors.length - 1)];
    }

+    // Hands the pending value to the caller. Called on release, and by the
+    // wheel's idle timer, which is the one input that never has one.
+    function commit(): void {
+        commitTimer.stop();
+        if (root.pending === null)
+            return;
+        root.committed(root.pending);
+        releaseTimer.restart();
+    }
+
    function quantise(ratio: real): real {
        const raw = root.minimum + ratio * (root.maximum - root.minimum);
        const snapped = Math.round(raw / root.step) * root.step;
@@ -166,7 +183,6 @@ Item {
                function move(positionX: real): void {
                    root.pending = root.quantise(
                        Math.max(0, Math.min(1, (positionX - 8) / track.width)));
-                    commitTimer.restart();
                }

                onPressed: event => drag.move(event.x)
@@ -174,6 +190,14 @@ Item {
                    if (drag.pressed)
                        drag.move(event.x);
                }
+                onReleased: root.commit()
+                // A grab taken away mid-drag is not a decision. The row goes
+                // back to showing whatever is really in effect rather than
+                // keeping a number nobody asked it to apply.
+                onCanceled: {
+                    commitTimer.stop();
+                    root.pending = null;
+                }
                onWheel: event => {
                    const direction = event.angleDelta.y > 0 ? 1 : -1;
                    root.pending = Math.max(root.minimum,
@@ -208,15 +232,13 @@ Item {
        color: Theme.alpha(Theme.fg, 0.065)
    }

+    // The wheel's release. A notch is a whole gesture on its own, and a hand
+    // spinning through a range means one commit at the end of the spin rather
+    // than one per notch -- there is no button coming up to say when that is.
    Timer {
        id: commitTimer
        interval: root.commitDelay
-        onTriggered: {
-            if (root.pending === null)
-                return;
-            root.committed(root.pending);
-            releaseTimer.restart();
-        }
+        onTriggered: root.commit()
    }

    // Hands the readout back to whatever is really in effect. If the change was
@@ -60,6 +60,24 @@ SettingsPage {
            setting: "overAmplification"
            divider: false
        }
+
+        // Turning over-amplification off has to bring the level down with it.
+        // The preference only moves the top of the range; PipeWire keeps
+        // whatever was set, so a device left at 140% stays there with a slider
+        // that now ends at 100 and shows itself pinned at full. Nothing the
+        // page can do afterwards expresses the difference, and the reading is
+        // wrong until something else happens to move the volume.
+        Connections {
+            target: Settings
+
+            function onOverAmplificationChanged(): void {
+                if (Settings.overAmplification)
+                    return;
+                const audio = root.currentOutput?.audio ?? null;
+                if (audio && audio.volume > 1)
+                    audio.volume = 1;
+            }
+        }
    }

    SettingsCard {
@@ -165,6 +165,12 @@ SettingRow {
                            root.moveTo(drag.ratioAt(event.x));
                    }
                    onReleased: root.apply()
+                    // A grab taken away mid-drag never releases, so nothing
+                    // would ever apply the reading left on screen -- and the
+                    // next commit would measure its ratio against a number the
+                    // palette was never re-saturated to. Back to the last
+                    // reading that was really applied.
+                    onCanceled: root.shown = root.committed
                    onWheel: event => {
                        root.moveTo((root.shown + (event.angleDelta.y > 0 ? 0.05 : -0.05))
                            / root.maximum);