Merge pull request 'fix(client/ios): a click wins the pointer back after Escape drops it' (#34) from worktree-ipad-click-relock into main

Reviewed-on: #34

clients/apple/Sources/PunktfunkKit/Gamepad/DualSenseHID.swift

@@ -21,12 +21,8 @@ import os
 
 private let log = Logger(subsystem: "io.unom.punktfunk", category: "gamepad")
 
-/// Opens one connected Sony DualSense and forwards motor rumble to it over raw HID.
-///
-/// A caller that owns a particular pad passes the location id it wants (see
-/// `open(preferringLocationID:)`); the renderer takes that from the `GCController` it is bound to,
-/// so with two DualSenses attached each renderer drives its own device. Without a preference the
-/// lowest location id wins — an arbitrary but *stable* choice, where `Set.first` was neither.
+/// Opens the first connected Sony DualSense and forwards motor rumble to it over raw HID.
+/// Single-pad model (we forward exactly one controller), so the first match is the right one.
 final class DualSenseHID {
     private let manager: IOHIDManager
     private var device: IOHIDDevice?
@@ -47,57 +43,9 @@ final class DualSenseHID {
 
     deinit { close() }
 
-    /// The IOKit location id of the device this instance opened — the handle a caller correlates
-    /// with its `GCController`. `nil` until a successful `open`.
-    private(set) var locationID: UInt32?
-
-    /// A device's location id, or `nil` if IOKit does not report one.
-    static func locationID(of dev: IOHIDDevice) -> UInt32? {
-        IOHIDDeviceGetProperty(dev, kIOHIDLocationIDKey as CFString) as? UInt32
-    }
-
-    /// Every connected DualSense/Edge, by location id — what a caller pairs against its controllers.
-    static func attachedLocationIDs() -> [UInt32] {
-        let mgr = IOHIDManagerCreate(kCFAllocatorDefault, IOOptionBits(kIOHIDOptionsTypeNone))
-        let matches = productIDs.map { pid in
-            [kIOHIDVendorIDKey: vendorSony, kIOHIDProductIDKey: pid] as CFDictionary
-        }
-        IOHIDManagerSetDeviceMatchingMultiple(mgr, matches as CFArray)
-        guard IOHIDManagerOpen(mgr, IOOptionBits(kIOHIDOptionsTypeNone)) == kIOReturnSuccess else {
-            return []
-        }
-        defer { IOHIDManagerClose(mgr, IOOptionBits(kIOHIDOptionsTypeNone)) }
-        let devices = IOHIDManagerCopyDevices(mgr) as? Set<IOHIDDevice> ?? []
-        return devices.compactMap(locationID(of:)).sorted()
-    }
-
-    /// Which attached device to drive, as an index into `ids` — the whole selection rule, pure so
-    /// it can be tested without an `IOHIDDevice` (which cannot be constructed).
-    ///
-    /// `IOHIDManagerCopyDevices` returns an unordered `Set`, so the previous `Set.first` was not
-    /// merely arbitrary — it can differ between two calls in one process. With two DualSenses that
-    /// made each renderer's pad→device binding a coin flip: both could land on the same device
-    /// (one pad's rumble coming out of the other, and the two per-instance write dedupes fighting
-    /// over it) or split by luck. An explicit location id makes the binding deterministic; the
-    /// lowest-id fallback at least makes it stable. `nil` ids sort last so a device IOKit cannot
-    /// place never displaces one it can.
-    static func preferredIndex(among ids: [UInt32?], preferring wanted: UInt32?) -> Int? {
-        if let wanted, let hit = ids.firstIndex(where: { $0 == wanted }) { return hit }
-        return ids.indices.min { (ids[$0] ?? .max) < (ids[$1] ?? .max) }
-    }
-
-    /// Pick the device to drive from everything attached (see [`preferredIndex`]).
-    static func pick(_ devices: Set<IOHIDDevice>, preferring wanted: UInt32?) -> IOHIDDevice? {
-        let ordered = Array(devices)
-        guard let i = preferredIndex(among: ordered.map(locationID(of:)), preferring: wanted) else {
-            return nil
-        }
-        return ordered[i]
-    }
-
-    /// Find and open a connected DualSense, preferring the one at `preferredLocationID`. Returns
-    /// false if none is present or it can't be opened (caller then falls back to CoreHaptics).
-    func open(preferringLocationID preferred: UInt32? = nil) -> Bool {
+    /// Find and open the first connected DualSense. Returns false if none is present or it can't
+    /// be opened (caller then falls back to CoreHaptics).
+    func open() -> Bool {
         let matches = Self.productIDs.map { pid in
             [kIOHIDVendorIDKey: Self.vendorSony, kIOHIDProductIDKey: pid] as CFDictionary
         }
@@ -107,21 +55,13 @@ final class DualSenseHID {
             return false
         }
         guard let devices = IOHIDManagerCopyDevices(manager) as? Set<IOHIDDevice>,
-              let dev = Self.pick(devices, preferring: preferred)
+              let dev = devices.first
         else {
             log.info("rumble: no DualSense HID device found — falling back to CoreHaptics")
             IOHIDManagerClose(manager, IOOptionBits(kIOHIDOptionsTypeNone))
             return false
         }
         device = dev
-        locationID = Self.locationID(of: dev)
-        if let preferred, locationID != preferred {
-            // Not fatal — one pad still gets rumble — but with two pads attached it means this
-            // renderer is driving the wrong one, and it is invisible without the log line.
-            log.error(
-                "rumble: wanted DualSense at location \(preferred, privacy: .public) but opened \(self.locationID.map(String.init) ?? "unknown", privacy: .public)"
-            )
-        }
         let transport = IOHIDDeviceGetProperty(dev, kIOHIDTransportKey as CFString) as? String
         bluetooth = transport?.lowercased().contains("bluetooth") ?? false
         log.info("rumble: DualSense raw-HID rumble active (transport=\(self.transport, privacy: .public))")
@@ -130,16 +70,8 @@ final class DualSenseHID {
 
     /// Drive the motors. `low` = left/heavy (low-frequency), `high` = right/light (high-frequency),
     /// each 0...255. (0, 0) stops.
-    ///
-    /// Returns whether the write reached the device. The caller needs this: it used to be logged
-    /// and swallowed, so a failed write still counted as a successful render. That matters most
-    /// for a **stop**, which has nothing behind it — the renderer stamps its write clock even on
-    /// failure, the keepalive re-write only fires for non-zero levels, and the ticker is cancelled
-    /// once the target is `(0, 0)`. On USB there is no firmware timeout either, so a swallowed
-    /// stop left the motors running with nothing scheduled to try again.
-    @discardableResult
-    func rumble(low: UInt8, high: UInt8) -> Bool {
-        guard let dev = device else { return false }
+    func rumble(low: UInt8, high: UInt8) {
+        guard let dev = device else { return }
         let report = bluetooth
             ? Self.bluetoothReport(low: low, high: high)
             : Self.usbReport(low: low, high: high)
@@ -149,9 +81,7 @@ final class DualSenseHID {
         }
         if rc != kIOReturnSuccess {
             log.error("rumble: IOHIDDeviceSetReport failed (0x\(String(format: "%08x", rc), privacy: .public))")
-            return false
         }
-        return true
     }
 
     func close() {

clients/apple/Sources/PunktfunkKit/Gamepad/GamepadFeedback.swift

@@ -117,15 +117,7 @@ public final class GamepadFeedback {
             reset(slot.controller)
             slots[pad] = nil
             let renderer = withRouting { rumbleByPad.removeValue(forKey: pad) }
-            // OFF the main actor. `RumbleRenderer.stop()` is a `queue.sync`, and its body is a
-            // per-motor `CHHapticEngine.stop()` — an XPC round trip to gamecontrollerd, which the
-            // renderer's own notes record as able to hang — plus `DualSenseHID.close()`, whose
-            // blocking `IOHIDDeviceSetReport` goes to a device that has just departed. It also
-            // queues behind any in-flight `setup()`. This runs on every unplug and every pin
-            // change, and the main thread is what drives the presenter's CADisplayLink, so
-            // blocking here hitches the picture mid-stream. The renderer is already detached from
-            // routing above, so nothing observes it after this point.
-            if let renderer { Task.detached { renderer.stop() } }
+            renderer?.stop()
         }
         for (pad, controller) in want {
             if let slot = slots[pad] {
@@ -290,12 +282,6 @@ public final class GamepadFeedback {
     private func reset(_ controller: GCController?) {
         guard let c = controller else { return }
         c.playerIndex = .indexUnset
-        // Put the lightbar out too. This class is what turned it on (see the `Led` and
-        // `PlayerLeds` arms), and every DS write is valid-flag-selective, so a colour the game
-        // set stays lit in firmware after the stream ends — back at the launcher, or for a pad
-        // that merely left the forwarded set. A DS4 is cleared incidentally because its player
-        // indicator IS the lightbar; a DualSense is not.
-        c.light?.color = GCColor(red: 0, green: 0, blue: 0)
         if let ds = c.extendedGamepad as? GCDualSenseGamepad {
             ds.leftTrigger.setModeOff()
             ds.rightTrigger.setModeOff()

clients/apple/Sources/PunktfunkKit/Gamepad/RumbleRenderer.swift

@@ -459,18 +459,6 @@ final class RumbleRenderer: @unchecked Sendable {
         if split {
             low = makeMotor(haptics, .leftHandle, sharpness: RumbleTuning.sharpnessLow)
             high = makeMotor(haptics, .rightHandle, sharpness: RumbleTuning.sharpnessHigh)
-            // HALF a split is worse than none, and it used to pass silently: only the all-nil case
-            // below counts as failure, so one surviving handle left `ok` true and `reportHealth(nil)`
-            // announced HEALTHY. What actually rendered was wrong in a direction that depends on
-            // which handle died — lose `high` and `render` falls to the combined branch (selected
-            // purely by `high != nil`), playing max(low, high) on the LEFT handle at the combined
-            // sharpness; lose `low` and the split branch's reconcile no-ops on the nil slot, so the
-            // heavy motor is discarded outright. Tear the survivor down and take the combined path,
-            // which at least renders both motors somewhere.
-            if low == nil || high == nil {
-                log.warning("rumble: only one split-handle engine came up — falling back to combined")
-                teardown() // disarms handlers, stops the survivor's players + engine, nils both
-            }
         } else {
             low = makeMotor(haptics, .default, sharpness: RumbleTuning.sharpnessCombined)
         }
@@ -599,9 +587,7 @@ final class RumbleRenderer: @unchecked Sendable {
         #if os(macOS)
         guard let c, c.extendedGamepad is GCDualSenseGamepad else { return false }
         let hid = DualSenseHID()
-        // Ask for the device this renderer's controller actually is, so two attached DualSenses
-        // do not both get driven through whichever one an unordered Set happened to yield first.
-        guard hid.open(preferringLocationID: Self.hidLocationID(for: c)) else { return false }
+        guard hid.open() else { return false }
         dualSenseHID = hid
         return true
         #else
@@ -609,24 +595,6 @@ final class RumbleRenderer: @unchecked Sendable {
         #endif
     }
 
-    #if os(macOS)
-    /// Correlate a `GCController` with an IOKit location id.
-    ///
-    /// GameController exposes no location id, so there is no direct mapping. What it does expose is
-    /// a stable per-controller ordering, and IOKit's location ids are stable per port: pairing the
-    /// two by rank makes each renderer pick a *distinct* device, which is the property that was
-    /// missing. With one pad attached this is the same device it always was.
-    static func hidLocationID(for c: GCController) -> UInt32? {
-        let ids = DualSenseHID.attachedLocationIDs()
-        guard ids.count > 1 else { return ids.first }
-        let peers = GCController.controllers().filter { $0.extendedGamepad is GCDualSenseGamepad }
-        guard let rank = peers.firstIndex(where: { $0 === c }), rank < ids.count else {
-            return ids.first
-        }
-        return ids[rank]
-    }
-    #endif
-
     /// Write the target to the DualSense over HID if that's the active backend; false → not a
     /// HID pad, so the caller renders via CoreHaptics. Deduped on the pad's 0...255 resolution,
     /// with a periodic keepalive re-write while nonzero (the ticker calls back in here).
@@ -637,20 +605,8 @@ final class RumbleRenderer: @unchecked Sendable {
         let keepalive = levels != (0, 0)
             && seconds(since: lastHidWrite.at) > RumbleTuning.hidKeepaliveSeconds
         if levels != lastHidWrite.levels || keepalive {
-            if hid.rumble(low: levels.0, high: levels.1) {
-                lastHidWrite = (levels, .now())
-            } else {
-                // The write did not reach the device. Do NOT stamp the clock — that would claim a
-                // render that never happened, and for a stop there is nothing behind it: the
-                // keepalive only re-writes non-zero levels and the ticker is cancelled once the
-                // target is (0, 0), so the motors would keep running with nothing scheduled.
-                // Drop the handle instead: the pad reverts to CoreHaptics, and a reconnect
-                // rebuilds it. Health is reported so the state is visible rather than silent.
-                log.error("rumble: HID write failed — dropping the handle, falling back")
-                closeHID()
-                reportHealth("Lost the direct connection to this DualSense; using the system path.")
-                return false
-            }
+            hid.rumble(low: levels.0, high: levels.1)
+            lastHidWrite = (levels, .now())
         }
         return true
         #else

clients/apple/Sources/PunktfunkKit/Views/StreamViewIOS.swift

@@ -186,6 +186,16 @@ public final class StreamViewController: StreamViewControllerBase {
     // pointer back to iPadOS, so an unwanted drop is re-requested below. The DELIBERATE releases
     // (⌘⎋, ⌃⌥⇧Q, the Stream menu, backgrounding) all clear `captured` first, so `wantsPointerLock`
     // is already false when their drop is observed and none of them are fought here.
+    //
+    // Recovery is TWO-STAGE, because either stage alone leaves a hole:
+    //   1. the burst below, fired the instant the drop is observed — wins back a lock the system
+    //      is willing to return immediately (a transient drop that wasn't Escape at all);
+    //   2. a CLICK into the video while still captured (`onPointerButton`) — the fallback for the
+    //      Escape case proper, where the platform declines during the moment right after its own
+    //      release gesture and the burst therefore expires having achieved nothing.
+    // Stage 2 is what keeps a lost burst from being permanent: `captured` is still true, so no
+    // other path would ever ask again, and the capture would spend the rest of its life on the
+    // absolute pointer — clicking correctly, aiming not at all.
     /// Whether this capture ever actually held the lock. Only a lock we HELD is worth winning back
     /// — never having been granted one means the scene doesn't qualify, not that Esc took it.
     /// Cleared when capture ends, so each capture starts from a clean slate.
@@ -446,6 +456,31 @@ public final class StreamViewController: StreamViewControllerBase {
             }
             guard self.inputCapture?.gcMouseForwarding == false else { return }
             self.inputCapture?.sendMouseButton(button, pressed: down)
+            // …and if we're captured but NOT locked, this click is also the recovery gesture for an
+            // Escape-drop the burst lost. iPadOS refuses to re-lock in the moment right after its
+            // own "let me out" gesture, so the burst fired at the drop can spend its whole budget
+            // and give up while the capture is still wanted. Nothing else would ever re-ask —
+            // setCaptured is the only other requester and a bare Esc never clears `captured` — so
+            // without this the session stays on the absolute path for the rest of the capture:
+            // clicks still land where you aim (absolute positions keep forwarding) but the game
+            // gets no relative deltas, so camera look is dead. A click is a real user gesture,
+            // which is exactly what the platform wants before it will hand the lock back.
+            //
+            // On the button UP, so the click has fully forwarded on ONE transport first: asking on
+            // the DOWN can flip `gcMouseForwarding` mid-click and strand the release on the GCMouse
+            // path. Gated on `pointerLockWasEngaged` exactly as the drop path is, so a scene that
+            // never qualifies (Stage Manager, Split View) is never bursted at, and on a burst not
+            // already being in flight — a pending burst mutes absolute motion, so re-arming one on
+            // every click of a menu the user is still aiming around would freeze the cursor between
+            // clicks. Only once it has settled does a further click buy a fresh budget (clearing the
+            // attempt counter, so a gesture isn't refused inside the 2 s window the drop's own burst
+            // may have just spent).
+            if !down, self.wantsPointerLock, self.pointerLockWasEngaged,
+                !self.pointerRelockPending, self.pointerLockEngaged() != true {
+                self.pointerRelockAttempt = 0
+                self.updatePointerLockChain() // a reparent since the drop would break the walk to us
+                self.requestPointerRelock()
+            }
         }
         // Scroll is the ONE indirect channel that is NOT gated on the lock. The scroll pan keeps
         // firing while the scene is pointer-locked (it is the only way trackpad two-finger scrolling

clients/apple/Tests/PunktfunkKitTests/DualSenseHIDTests.swift

@@ -43,33 +43,5 @@ final class DualSenseHIDTests: XCTestCase {
         let crc = DualSenseHID.crc32(seed: UInt8(ascii: "1"), Array("23456789".utf8))
         XCTAssertEqual(crc, 0xCBF4_3926)
     }
-
-    // MARK: - Device selection (B14)
-
-    /// With two DualSenses attached, each renderer must drive its OWN device. The old code took
-    /// `Set.first` from an unordered set, so the pad→device binding was a coin flip that could
-    /// point both renderers at the same pad.
-    func testPreferredIndexHonoursAnExplicitLocation() {
-        let ids: [UInt32?] = [0x1D18_0000, 0x1420_0000, 0x1411_0000]
-        XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: 0x1420_0000), 1)
-        XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: 0x1D18_0000), 0)
-    }
-
-    /// No preference (or one the pad no longer has): fall back to the LOWEST id — arbitrary, but
-    /// stable across calls, which `Set.first` was not.
-    func testPreferredIndexFallsBackToTheLowestIdDeterministically() {
-        let ids: [UInt32?] = [0x1D18_0000, 0x1420_0000, 0x1411_0000]
-        XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: nil), 2)
-        // A wanted id that is gone (pad unplugged between enumeration and open) must not fail the
-        // open — it degrades to the same stable fallback.
-        XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: 0xDEAD_BEEF), 2)
-    }
-
-    /// A device IOKit reports no location for must never displace one it can place.
-    func testPreferredIndexSortsUnplaceableDevicesLast() {
-        XCTAssertEqual(DualSenseHID.preferredIndex(among: [nil, 0x1420_0000], preferring: nil), 1)
-        XCTAssertEqual(DualSenseHID.preferredIndex(among: [nil, nil], preferring: nil), 0)
-        XCTAssertNil(DualSenseHID.preferredIndex(among: [], preferring: nil))
-    }
 }
 #endif