diff --git a/CLAUDE.md b/CLAUDE.md index 7f54315..f9780af 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,7 +44,7 @@ Three SwiftPM targets (`Package.swift`): ## Things that are easy to get wrong - **Firmware lies**: it answers `{"ok":1}` to any payload, malformed or not. The LEDs are the only ground truth. `PadEmulator` reproduces this on purpose (and the stock keymap, and silent unbound keys) — keep it faithful when changing it. -- **Only one HID client at a time**: Work Louder's Input app and the Codex desktop app fight over the same lighting. A reply with a response id we never issued is how `contendingClient` is detected (`noteResponse(id:)` and `noteIssued(id:)`, also the test seams). Detection is passive, so the flag outlives the other app until `start()`, `stop()` or the panel's Recheck (`recheckContention()`: clear, then repaint if running) resets it. A recheck must never clear `issuedIDs`: a reply to our own call still in flight would look foreign and re-raise the warning at once. The active registry check is planned in `design/pad-contention.md`. The Inspector is a separate process and counts as another client. +- **Only one HID client at a time**: Work Louder's Input app and the Codex desktop app fight over the same lighting. `contendingClient` has two signals, combined only in `updateContention()`: a reply with a response id we never issued sets `trafficSeen` (`noteResponse(id:)` and `noteIssued(id:)`, also the test seams), and an IORegistry scan (`WLDevice.otherClients()`, behind the `scanClients` seam) lists the other processes holding the pad. An `.authoritative` scan (USB vendor interface) with others raises the flag by itself, unless Recheck acknowledged them (`acknowledgedPIDs`: the pids a Recheck's scan listed, so an app that opens every HID device and never sends cannot pin the warning; pruned by every scan that no longer lists them, so a relaunch raises again; kept by `.unavailable`; reset by `start()`/`stop()`; their replies still raise through `trafficSeen`); an `.advisory` one (Bluetooth) only names them; an empty list from either clears `trafficSeen`; `.unavailable` (emulator, failed walk) leaves the replies to decide. Scans run on panel open (`scanContention()`), on Recheck (`recheckContention()`: clear traffic, scan, acknowledge, repaint if running), after `openDevice()` connects, and after a foreign reply (at most once per `replyScanInterval`); never while off. `contenders` names them in the warning. A recheck must never clear `issuedIDs`: a reply to our own call still in flight would look foreign and re-raise the warning at once. The Inspector is a separate process and counts as another client. - **Signing matters**: Input Monitoring is granted per code signature, so ad-hoc builds need re-granting every rebuild. `bundle.sh` prefers a real Apple Development / Developer ID identity (`WL_SIGN_IDENTITY` overrides). `swift run` works only because it inherits the terminal's grant. - **A remote Herdr is an owned ssh**: `SSHTunnel` runs its own `/usr/bin/ssh` (`ControlMaster=no`, so the process lifetime *is* the tunnel's) with a remote `cat >/dev/null` on a stdin pipe we hold, so even a crash ends it by EOF. `HERDR_SOCKET_PATH` beats any tunnel, so the panel disables the target picker when it is set — and Herdr itself sets it for processes it spawns, so a `swift run` from inside a Herdr pane gets the disabled picker (and the `LiveBridgeTargetTests` skip). - **The socket path is global, so a switch must not leak.** `setSocketPath` bumps a generation; a reply that lands after a switch becomes `HerdrError.targetChanged`, and the bridge guards every async callback (tunnel state, lifecycle restart, status-stream `onClosed`, poll, in-flight `agent.list`) with its own generation counter plus a `herdrActive` flag. Event streams resolve the path in `start()`, not `init`. Pane ids are only unique per server — never carry agent state across targets. diff --git a/README.md b/README.md index a83d7e6..dae9679 100644 --- a/README.md +++ b/README.md @@ -289,12 +289,27 @@ is opened shared, so we receive other clients' replies, and a response id we never issued is a reliable tell. The **Inspector** counts too: it is a separate process with its own connection to the pad. -Detection is passive, so once raised the warning stays, even after the other -app has quit. Quit it, then press **Recheck** next to the warning: that clears -it and repaints the pad with the manager's colours. If the other app is still -there, the warning comes back as soon as it next sends to the pad. Switching -the manager off clears the warning too. An active check that clears it by -itself is planned in [design/pad-contention.md](design/pad-contention.md). +The manager also asks macOS which other processes hold the pad open, each time +the panel opens. Over USB that answer is exact: the warning names the other app +("Also driving this pad: Input — colours may fight."), and it clears by itself +the next time you open the panel after that app has quit. **Recheck** next to +the warning checks again on the spot and repaints the pad with the manager's +colours. Switching the manager off clears the warning too. + +Some apps (input remappers, macro or streaming tools) open every HID device +without ever touching the lighting, and would keep the warning up for as long +as they run. **Recheck** accepts the apps holding the pad at that moment: they +no longer raise the warning on their own, though they are still named if it +comes up for another reason. It comes back when one of them sends to the pad, +when a different app opens it, or when an accepted app quits and is started +again. Switching the manager off and on forgets what was accepted. + +Over Bluetooth the pad is a single device that also carries the keyboard, so +apps that only listen for keys (chat apps, for one) may hold it open too. There the +check can name the apps but not raise the warning by itself: it still takes a +reply we never asked for, and it stays until the other app has quit and you +press **Recheck**, or the check finds nobody else at all. See +[design/pad-contention.md](design/pad-contention.md) for the details. ## Development diff --git a/Sources/WLKit/BridgeController.swift b/Sources/WLKit/BridgeController.swift index c2dae94..261d89c 100644 --- a/Sources/WLKit/BridgeController.swift +++ b/Sources/WLKit/BridgeController.swift @@ -33,10 +33,14 @@ public final class BridgeController: ObservableObject { @Published public private(set) var keyEffects: [Int: OAI.Effect] = [:] @Published public private(set) var aggregateState: String? @Published public private(set) var lastError: String? - /// Another process is talking to the same pad. A shared HID open means we - /// receive its replies too, so a response id we never issued is a reliable - /// tell. + /// Another process is driving the same pad. Two signals feed it: a reply + /// id we never issued (a shared HID open means we receive other clients' + /// replies too), and a scan of the IORegistry for other processes holding + /// the pad open. `updateContention()` has the rule that combines them. @Published public private(set) var contendingClient = false + /// The other processes the last registry scan found, to name them in the + /// warning. Empty when the scan was unavailable. + @Published public private(set) var contenders: [HIDClient] = [] /// Whether the GitButler stack is on screen. Only the key light cares — /// the panel itself lives in the app layer. @Published public private(set) var stackPanelOpen = false @@ -90,7 +94,8 @@ public final class BridgeController: ObservableObject { // MARK: - Internals - private var device = WLDevice() + /// Internal-read so tests can unplug the virtual pad. + private(set) var device = WLDevice() /// Non-nil while the bridge is driving a virtual pad instead of hardware. @Published public private(set) var emulator: PadEmulator? private var lifecycle: HerdrEventStream? @@ -101,6 +106,29 @@ public final class BridgeController: ObservableObject { private var reopenTask: Task? private var lastFingerprint: String? private var issuedIDs = Set() + /// A reply id we never issued has been seen since the last clear. + private var trafficSeen = false + /// The last registry scan's verdict; `.unavailable` until one has run + /// on an open device. + private var lastScan: HIDClientScan = .unavailable + /// Other clients the user let stay with Recheck. They still hold the + /// pad, but no longer raise the warning on their own: an app that opens + /// every HID device and never sends would otherwise keep it up for good. + /// Their replies still do, through `trafficSeen`. A pid drops out once a + /// scan no longer lists it, so a relaunched app raises the warning again. + private var acknowledgedPIDs = Set() + /// When the last reply-triggered scan ran, on the `uptime` clock. + private var lastReplyScan: TimeInterval? + /// Who else holds the pad open. A seam for tests; reads the device at + /// call time, since `useEmulator` replaces it. + lazy var scanClients: @MainActor () -> HIDClientScan = { [weak self] in + self?.device.otherClients() ?? .unavailable + } + /// A monotonic clock for the reply-triggered scan limit. A seam for tests. + var uptime: @MainActor () -> TimeInterval = { ProcessInfo.processInfo.systemUptime } + /// The least time between two scans triggered by foreign replies, so a + /// chatty client cannot turn every reply into a registry walk. + static let replyScanInterval: TimeInterval = 1 private var warnedPermission = false /// Owned while the target is remote and the bridge runs. private var tunnel: SSHTunnel? @@ -156,6 +184,9 @@ public final class BridgeController: ObservableObject { guard let self else { return } self.deviceConnected = false self.lastFingerprint = nil + // Who held a pad that is gone says nothing; the reopen rescans. + self.lastScan = .unavailable + self.updateContention() if self.isRunning { self.scheduleReopen() } } device.onTX = { [weak self] _, _, id in @@ -181,22 +212,100 @@ public final class BridgeController: ObservableObject { /// reply id the bridge never issued, as another client's would. func noteResponse(id: Int) { // A reply to an id we never sent came from another client. - if issuedIDs.remove(id) == nil { contendingClient = true } - } - - /// The panel's Recheck: clears the warning, and repaints so our colours - /// replace whatever the other app left on the pad. Detection is passive, - /// so this proves nothing by itself — the warning comes back as soon as - /// the other app, if it is still there, sends to the pad again. + guard issuedIDs.remove(id) == nil else { return } + trafficSeen = true + // Scan to put a name on it, at most once a second. A reply within + // the window still raises the warning; it just keeps the last names. + let now = uptime() + if lastReplyScan.map({ now - $0 >= Self.replyScanInterval }) ?? true { + lastReplyScan = now + applyScan() + } else { + updateContention() + } + } + + /// Re-reads who else holds the pad open, without repainting. The panel + /// calls this every time it opens, so the warning clears by itself once + /// the other app has quit. + public func scanContention() { + applyScan() + } + + /// The panel's Recheck: forgets the replies seen so far, rescans, + /// acknowledges the other clients the scan still lists, and repaints so + /// our colours replace whatever the other app left on the pad. This + /// proves nothing by itself — the warning comes back as soon as the + /// other app, if it is still there, sends to the pad again, or a client + /// that was not acknowledged opens it. public func recheckContention() async { // `issuedIDs` is left alone on purpose: a reply to one of our own // calls still in flight would otherwise look foreign and raise the // warning again at once. - contendingClient = false + trafficSeen = false + applyScan() + if case .authoritative(let others) = lastScan { + acknowledgedPIDs = Set(others.map(\.pid)) + updateContention() + } guard isRunning else { return } await forceRepaint() } + /// Scans the registry, if there is an open pad to scan. An off bridge + /// drives nothing, so its scan is `.unavailable` without asking. + private func applyScan() { + lastScan = isRunning && deviceConnected ? scanClients() : .unavailable + switch lastScan { + case .authoritative(let others), .advisory(let others): + // Nobody else holds the pad now, so whoever sent those replies + // has gone. Only a fresh scan clears: replies seen after it + // raise the warning again until the next one. + if others.isEmpty { trafficSeen = false } + // An acknowledged app that has quit is forgotten, so the same app + // relaunched (a new pid) raises the warning again. + acknowledgedPIDs.formIntersection(others.map(\.pid)) + case .unavailable: + break + } + updateContention() + } + + /// The one place `contendingClient` is decided, from the replies seen + /// and the last scan: + /// + /// - unavailable: the replies alone (passive detection); + /// - authoritative (a dedicated vendor interface, USB): anyone else + /// there can drive the lighting, so a client Recheck did not + /// acknowledge raises it; + /// - advisory (the pad's only interface, Bluetooth): others may only be + /// listening for keys, so the list names but cannot raise. + /// + /// An empty list has already cleared the replies, in `applyScan()`. + private func updateContention() { + switch lastScan { + case .unavailable: + contenders = [] + contendingClient = trafficSeen + case .authoritative(let others): + contenders = others + contendingClient = trafficSeen || others.contains { !acknowledgedPIDs.contains($0.pid) } + case .advisory(let others): + contenders = others + contendingClient = trafficSeen + } + } + + /// Clears both signals and the acknowledgement; for `start()` and + /// `stop()`. + private func resetContention() { + trafficSeen = false + lastScan = .unavailable + acknowledgedPIDs = [] + lastReplyScan = nil + updateContention() + } + // MARK: - Lifecycle public func toggle() async { @@ -207,7 +316,7 @@ public final class BridgeController: ObservableObject { guard !isRunning else { return } isRunning = true lastError = nil - contendingClient = false + resetContention() keyBindings = KeyBindings.load() openingDevice = true @@ -228,7 +337,7 @@ public final class BridgeController: ObservableObject { // Cleared after the teardown, whose lights-off calls wait on replies // that another client's may land among; a start() that came in // meanwhile owns the flag. - if !isRunning { contendingClient = false } + if !isRunning { resetContention() } } /// Points the pad at another Herdr server. Takes effect at once while @@ -469,6 +578,9 @@ public final class BridgeController: ObservableObject { warnedPermission = false deviceName = device.info?.product ?? "Work Louder device" lastError = nil + // A client that was already holding the pad is seen now, not + // only once it next sends something. + applyScan() } catch { deviceConnected = false let message = error.localizedDescription diff --git a/Sources/WLMicroManager/MenuPanelView.swift b/Sources/WLMicroManager/MenuPanelView.swift index 77152a5..9fb08a1 100644 --- a/Sources/WLMicroManager/MenuPanelView.swift +++ b/Sources/WLMicroManager/MenuPanelView.swift @@ -40,17 +40,14 @@ struct MenuPanelView: View { if bridge.contendingClient { Divider() HStack(alignment: .firstTextBaseline, spacing: 8) { - Label( - "Another app is also driving this pad — colours may fight.", - systemImage: "exclamationmark.triangle" - ) + Label(contentionText, systemImage: "exclamationmark.triangle") .font(.caption) .foregroundStyle(.orange) .fixedSize(horizontal: false, vertical: true) Spacer(minLength: 0) Button("Recheck") { Task { await bridge.recheckContention() } } .controlSize(.small) - .help("Clear the warning. It comes back if the other app sends to the pad again.") + .help("Check again for other apps, accept the ones still holding the pad, and repaint it. The warning comes back if one of them sends to the pad, or another app opens it.") } .padding(.horizontal, 14).padding(.vertical, 8) } @@ -63,15 +60,27 @@ struct MenuPanelView: View { // relaunch. The window may be kept alive between openings, in which // case only becoming key marks a new one — this panel's window, not // the emulator or any other window of the app, which would re-read - // the config (and maybe restart a tunnel) for no reason. - .onAppear { reloadRemotes() } + // the config (and maybe restart a tunnel) for no reason. Who else + // holds the pad is re-read with it, so the warning clears by itself + // once the other app has quit. + .onAppear { reloadRemotes(); bridge.scanContention() } .background(WindowReader { panel.window = $0 }) .onReceive(NotificationCenter.default.publisher(for: NSWindow.didBecomeKeyNotification)) { note in guard let window = note.object as? NSWindow, window === panel.window else { return } reloadRemotes() + bridge.scanContention() } } + /// Names the other apps when the registry scan knew them. + private var contentionText: String { + guard !bridge.contenders.isEmpty else { + return "Another app is also driving this pad — colours may fight." + } + let names = bridge.contenders.map(\.name).joined(separator: ", ") + return "Also driving this pad: \(names) — colours may fight." + } + // MARK: - Header private var header: some View { diff --git a/Tests/WLKitTests/BridgeContentionTests.swift b/Tests/WLKitTests/BridgeContentionTests.swift index bb6d49e..01a4298 100644 --- a/Tests/WLKitTests/BridgeContentionTests.swift +++ b/Tests/WLKitTests/BridgeContentionTests.swift @@ -96,4 +96,305 @@ final class BridgeContentionTests: XCTestCase { bridge.noteResponse(id: 500) // its reply lands after the recheck XCTAssertFalse(bridge.contendingClient) } + + // MARK: - Registry scan + + private let inspector = HIDClient(pid: 4242, name: "Inspector") + /// What the stubbed scan returns next, and how often it was asked. + private var scanResult: HIDClientScan = .unavailable + private var scans = 0 + private var clock: TimeInterval = 1000 + + /// Starts the bridge with the registry scan and the clock stubbed out. + private func startScanned(_ initial: HIDClientScan = .unavailable) async { + scanResult = initial + bridge.scanClients = { [unowned self] in + self.scans += 1 + return self.scanResult + } + bridge.uptime = { [unowned self] in self.clock } + await bridge.start() + } + + /// Raises the warning by traffic alone: a foreign reply, with a scan + /// that cannot tell (so `scanResult` must be `.unavailable`). + private func seeTraffic() { + precondition(scanResult == .unavailable) + bridge.noteResponse(id: foreignID) + // Out of the reply-scan window, for whatever the test does next. + clock += BridgeController.replyScanInterval + } + + /// The emulator has no registry entry, so the real scan is unavailable + /// and nothing is named: Phase 1 behaviour, which the tests above check. + func testEmulatorScanIsUnavailable() async { + await bridge.start() + XCTAssertEqual(bridge.scanClients(), .unavailable) + bridge.noteResponse(id: foreignID) + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, []) + } + + /// Row 1: an unavailable scan leaves the replies to decide, and does not + /// clear what they raised. + func testUnavailableScanFollowsTraffic() async { + await startScanned() + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient) + + seeTraffic() + bridge.scanContention() + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, []) + } + + /// Row 2: nobody else on a dedicated vendor interface clears the + /// warning, and forgets the replies behind it. + func testAuthoritativeEmptyClearsTraffic() async { + await startScanned() + seeTraffic() + XCTAssertTrue(bridge.contendingClient) + + scanResult = .authoritative([]) + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient) + + scanResult = .unavailable + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient, "the replies were forgotten, not just outvoted") + } + + /// Row 3: anyone else on the vendor interface raises it with no traffic + /// at all, and is named. + func testAuthoritativeOthersRaise() async { + await startScanned() + XCTAssertFalse(bridge.contendingClient) + + scanResult = .authoritative([inspector]) + bridge.scanContention() + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, [inspector]) + + // And it goes once the other app has. + scanResult = .authoritative([]) + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, []) + } + + /// Row 4: an advisory empty list clears the warning and the replies. + func testAdvisoryEmptyClearsTraffic() async { + await startScanned() + seeTraffic() + + scanResult = .advisory([]) + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient) + + scanResult = .unavailable + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient, "the replies were forgotten, not just outvoted") + } + + /// Row 5: on a pad that also carries the keyboard, others may only be + /// listening for keys: the list names them but cannot raise the warning. + func testAdvisoryOthersOnlyLabel() async { + await startScanned(.advisory([inspector])) + XCTAssertFalse(bridge.contendingClient, "an advisory list alone does not raise it") + XCTAssertEqual(bridge.contenders, [inspector]) + + bridge.noteResponse(id: foreignID) + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, [inspector]) + } + + /// A foreign reply scans to name its sender, but at most once a second. + /// A reply inside the window still raises the warning. + func testReplyScansAreLimitedToOnePerSecond() async { + await startScanned() + let afterStart = scans + XCTAssertEqual(afterStart, 1, "openDevice() scanned once") + + bridge.noteResponse(id: foreignID) + XCTAssertEqual(scans, afterStart + 1, "the first foreign reply scans") + + clock += BridgeController.replyScanInterval * 0.9 + bridge.noteResponse(id: foreignID) + XCTAssertEqual(scans, afterStart + 1, "a second reply within the second does not") + XCTAssertTrue(bridge.contendingClient) + + // The panel's scan neither counts towards the limit nor is held by it. + bridge.scanContention() + XCTAssertEqual(scans, afterStart + 2) + + clock += BridgeController.replyScanInterval * 0.1 + bridge.noteResponse(id: foreignID) + XCTAssertEqual(scans, afterStart + 3, "a reply a second after the last reply scan scans again") + + // Replies to our own calls never scan. + await bridge.forceRepaint() + XCTAssertEqual(scans, afterStart + 3) + } + + /// The reply-triggered scan is what puts a name on a client first seen + /// by its traffic. + func testForeignReplyNamesTheSender() async { + await startScanned() + scanResult = .authoritative([inspector]) + bridge.noteResponse(id: foreignID) + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, [inspector]) + } + + /// A client already holding the pad is seen as the bridge opens it. + func testOpenScans() async { + await startScanned(.authoritative([inspector])) + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, [inspector]) + } + + /// Who held a pad that has gone says nothing about the pad that comes + /// back: an unplug drops the scan, and the reopen scans afresh. + func testUnplugDropsTheScan() async { + await startScanned(.authoritative([inspector])) + XCTAssertTrue(bridge.contendingClient) + + bridge.device.disconnect(reason: "unplugged") + XCTAssertFalse(bridge.deviceConnected) + XCTAssertFalse(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, []) + } + + /// Recheck forgets the replies and rescans: an empty authoritative list + /// clears the warning, and so does one that still has the other app, + /// which Recheck acknowledges. It repaints either way. + func testRecheckScans() async throws { + await startScanned() + seeTraffic() + let before = scans + + scanResult = .authoritative([inspector]) + await bridge.recheckContention() + XCTAssertEqual(scans, before + 1) + XCTAssertFalse(bridge.contendingClient, "the app still there is acknowledged") + XCTAssertEqual(bridge.contenders, [inspector], "and still listed") + + scanResult = .authoritative([]) + await bridge.recheckContention() + XCTAssertFalse(bridge.contendingClient) + + scanResult = .unavailable + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient, "the replies seen earlier stay forgotten") + } + + // MARK: - Acknowledged clients + + private let relaunched = HIDClient(pid: 4343, name: "Inspector") + private let input = HIDClient(pid: 5151, name: "Input") + + /// An app that holds the vendor interface but never sends (one that opens + /// every HID device, say) raises the warning on every scan. Recheck lets + /// it stay: later scans that still list it keep the warning down, while + /// it is still named should the warning come up for another reason. + func testRecheckAcknowledgesAPassiveHolder() async { + await startScanned(.authoritative([inspector])) + XCTAssertTrue(bridge.contendingClient) + + await bridge.recheckContention() + XCTAssertFalse(bridge.contendingClient) + + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient, "a later scan still listing it") + XCTAssertEqual(bridge.contenders, [inspector]) + } + + /// Acknowledging one app does not excuse another that opens the pad + /// later. + func testAnUnacknowledgedClientStillRaises() async { + await startScanned(.authoritative([inspector])) + await bridge.recheckContention() + XCTAssertFalse(bridge.contendingClient) + + scanResult = .authoritative([inspector, input]) + bridge.scanContention() + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, [inspector, input]) + } + + /// An acknowledged app that sends to the pad raises the warning through + /// its replies, and is named. + func testAnAcknowledgedClientsReplyStillRaises() async { + await startScanned(.authoritative([inspector])) + await bridge.recheckContention() + XCTAssertFalse(bridge.contendingClient) + + bridge.noteResponse(id: foreignID) + XCTAssertTrue(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, [inspector]) + } + + /// The acknowledgement is by pid. Once a scan no longer lists the app it + /// is forgotten, so the same app relaunched raises the warning again — + /// even if it comes back under the old pid. + func testAQuitAppIsForgotten() async { + await startScanned(.authoritative([inspector])) + await bridge.recheckContention() + + scanResult = .authoritative([]) + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient) + + scanResult = .authoritative([relaunched]) + bridge.scanContention() + XCTAssertTrue(bridge.contendingClient, "a relaunch under a new pid") + + scanResult = .authoritative([]) + bridge.scanContention() + scanResult = .authoritative([inspector]) + bridge.scanContention() + XCTAssertTrue(bridge.contendingClient, "the old pid, after a scan without it") + } + + /// An unavailable scan says nothing about who left, so it keeps the + /// acknowledgement. + func testUnavailableScanKeepsTheAcknowledgement() async { + await startScanned(.authoritative([inspector])) + await bridge.recheckContention() + + scanResult = .unavailable + bridge.scanContention() + scanResult = .authoritative([inspector]) + bridge.scanContention() + XCTAssertFalse(bridge.contendingClient) + } + + /// Switching off and on forgets what Recheck acknowledged. + func testStopForgetsTheAcknowledgement() async { + await startScanned(.authoritative([inspector])) + await bridge.recheckContention() + XCTAssertFalse(bridge.contendingClient) + + await bridge.stop() + await bridge.start() + XCTAssertTrue(bridge.contendingClient, "the open scan lists it again") + } + + /// Switched off, nothing is scanned and nothing is named, whatever the + /// registry would say. + func testOffBridgeDoesNotScan() async { + await startScanned(.authoritative([inspector])) + XCTAssertTrue(bridge.contendingClient) + + await bridge.stop() + XCTAssertFalse(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, []) + + let before = scans + bridge.scanContention() + await bridge.recheckContention() + XCTAssertEqual(scans, before) + XCTAssertFalse(bridge.contendingClient) + XCTAssertEqual(bridge.contenders, []) + } } diff --git a/design/pad-contention.md b/design/pad-contention.md index c5aa0d9..1b86990 100644 --- a/design/pad-contention.md +++ b/design/pad-contention.md @@ -1,6 +1,6 @@ # Pad contention: a warning that can clear -Status: Phases 1 and 2 implemented. +Status: Phases 1–3 implemented. The panel warns "Another app is also driving this pad — colours may fight." when something else talks to the pad. Once raised, the warning stays until @@ -181,18 +181,35 @@ State, in `BridgeController`: |---|---|---|---| | `.unavailable` | — | `trafficSeen` | none (Phase 1 behaviour) | | `.authoritative` | empty | false | cleared: whoever sent it is gone | -| `.authoritative` | some | true | none | +| `.authoritative` | some | true, unless all acknowledged | none | | `.advisory` | empty | false | cleared | | `.advisory` | some | `trafficSeen` | none: the list labels, it cannot raise | +Acknowledged clients (added after PR #20's review): `recheckContention()` +records the pids of the `.authoritative` others it finds in a private +`acknowledgedPIDs: Set`, and only others outside that set raise the +warning. Without it, an app that opens every HID device (an input remapper, +a macro or streaming tool) but never sends would raise the warning on every +scan, and nothing short of quitting it would clear it: Recheck rescans and +finds it again, and so does an off/on toggle. Acknowledged clients: + +- are still listed in `contenders`, so they are named if the warning comes + up for another reason; +- still raise it through `trafficSeen` when they send; +- drop out of the set when a scan (`.authoritative` or `.advisory`) no + longer lists them, so a relaunched app raises the warning again; + `.unavailable` keeps the set, since it says nothing about who left; +- are forgotten by `start()` and `stop()` (`resetContention()`). + Scans run: - when the panel opens: the existing `.onAppear` and own-window `didBecomeKeyNotification` hooks in `MenuPanelView`, which already call `reloadRemotes()`, also call a new `bridge.scanContention()` (scan only, no repaint); -- from **Recheck**: `recheckContention()` clears `trafficSeen`, scans, then - repaints as in Phase 1; +- from **Recheck**: `recheckContention()` clears `trafficSeen`, scans, + acknowledges the `.authoritative` others it found, then repaints as in + Phase 1; - after a foreign reply, to put a name on it: at most one scan per second, so a chatty client cannot turn every reply into a registry walk; - after the device (re)opens in `openDevice()`. @@ -378,34 +395,96 @@ Notes: Branch `feat/contention-3-active-check` · PR title `feat: clear the "another app" warning once the other app is gone` -- [ ] Phase 2's gate in Notes says go (or Part 1 was adjusted first). -- [ ] `trafficSeen`, `contenders`, `updateContention()` and the table in +- [x] Phase 2's gate in Notes says go (or Part 1 was adjusted first). +- [x] `trafficSeen`, `contenders`, `updateContention()` and the table in Part 1; `contendingClient` stays the flag the panel reads. -- [ ] `scanClients` seam; `scanContention()` (scan only) and +- [x] `scanClients` seam; `scanContention()` (scan only) and `recheckContention()` (clear traffic, scan, repaint). -- [ ] Scans on panel open (both existing hooks), on **Recheck**, after a +- [x] Scans on panel open (both existing hooks), on **Recheck**, after a foreign reply (at most once a second) and after `openDevice()` succeeds. -- [ ] Panel copy with names, as in Part 1; **Recheck** kept. -- [ ] Tests (`BridgeContentionTests`, with the `scanClients` seam): every row +- [x] Panel copy with names, as in Part 1; **Recheck** kept. +- [x] Tests (`BridgeContentionTests`, with the `scanClients` seam): every row of the table; the one-per-second limit on reply-triggered scans; a recheck that finds an authoritative empty list clears traffic seen earlier; `.unavailable` behaves exactly as Phase 1. -- [ ] Each test mutation-checked. -- [ ] `README.md` "Only one bridge at a time": the panel names the other +- [x] Each test mutation-checked. +- [x] `README.md` "Only one bridge at a time": the panel names the other app, and the warning clears by itself when the panel next opens after that app has quit (USB). Explain the Bluetooth caveat if Phase 2 found one. -- [ ] `CLAUDE.md`: the "Only one HID client at a time" bullet describes +- [x] `CLAUDE.md`: the "Only one HID client at a time" bullet describes both signals (reply ids, registry scan) and the table's rule. - [ ] User checks listed in the PR: Inspector open → named warning; quit it, reopen the panel → warning gone without pressing anything; same with Input or Codex if installed. -- [ ] `swift build -c release` (zero warnings) and +- [x] `swift build -c release` (zero warnings) and `env -u HERDR_SOCKET_PATH swift test` green. Notes: +- The table's "effect on `trafficSeen`" column is applied only when a scan + runs (`applyScan()`), not on every recompute. `updateContention()` is then + `trafficSeen || (authoritative && !others.isEmpty)` for the flag, which + gives every row of the table at scan time and keeps a reply that lands + *after* an empty scan raising the warning. Clearing on every recompute + would let a stale `.authoritative([])` swallow each later foreign reply + that falls inside the reply-scan window. +- The one-per-second limit counts reply-triggered scans only, on a + monotonic clock (`ProcessInfo.systemUptime`, behind the `uptime` seam). + Panel, Recheck and open scans neither count towards it nor are held by + it. A reply inside the window still raises the warning (it sets + `trafficSeen`); it just keeps the names from the last scan. There is no + trailing scan at the end of the window: a chatty client is named by the + first scan anyway, and the panel rescans whenever it opens. +- Scans only reach the registry while running with the device connected; + otherwise they are `.unavailable` without calling `scanClients`, so an off + bridge never shows the warning (Phase 1's rule). `start()` and `stop()` + reset both signals and the reply-scan clock. +- Beyond the plan: an unplug (`device.onDisconnect`) drops the last scan to + `.unavailable`, since who held a pad that has gone says nothing; the + reopen scans afresh. Tested by unplugging the virtual pad, which needed + `device` to become `private(set)` (internal read) instead of `private`. +- `scanClients` is a `lazy var` so its default can capture `self` weakly + and read `device` at call time. +- **Recheck**'s help text now reads "Check again for other apps and repaint + the pad. The warning comes back if the other app is still there." — + Phase 1's text ("It comes back if the other app sends to the pad again") + no longer described what it does. +- Mutation checks, each restored afterwards (the failing tests in + brackets): an empty scan keeps `trafficSeen` (authoritative-empty, + advisory-empty); authoritative others ignored (authoritative-others, + open, recheck, off); advisory others raise (advisory-others); no reply + scan limit (limit); replies never scan (names-the-sender, limit); an off + bridge scans (off); `openDevice()` does not scan (open, advisory-others, + off, limit); Recheck does not scan (recheck); `stop()` keeps the last scan + (off); an unavailable scan clears `trafficSeen` (unavailable, emulator + and the Phase 1 tests); panel scans count towards the limit (limit); + `.unavailable` keeps the old names (off); an unplug keeps the scan + (unplug). +- No hardware or UI check was done in-session; the named warning and the + clear-on-reopen round trip are user checks in the PR. Bluetooth remains + unverified (Phase 2 Notes). +- Review follow-up (PR #20): an app that holds the vendor interface but never + sends made the warning permanent, since every scan (Recheck's and the + reopen's included) found it again. Not verified on hardware: it follows + from IOKit matching, where a manager with nil or broad matching opens the + 0xFF00 interface like any other. Fixed by letting Recheck acknowledge the + clients it finds (`acknowledgedPIDs`, Part 1). Recheck's help text now + reads "Check again for other apps, accept the ones still holding the pad, + and repaint it. The warning comes back if one of them sends to the pad, or + another app opens it." `testRecheckScans` changed accordingly: a Recheck + that still finds the Inspector now clears the warning. +- Mutation checks for the follow-up, each restored afterwards: Recheck does + not acknowledge (recheck, passive holder, unacknowledged, acknowledged + reply, unavailable keeps, stop forgets); no pruning (quit app forgotten); + `.unavailable` clears the set (unavailable keeps); `resetContention()` + keeps the set (stop forgets); the set ignored when raising (as "does not + acknowledge"); every other acknowledged, i.e. authoritative others never + raise (authoritative-others, open, unplug, off, passive holder, + unacknowledged, quit app forgotten, stop forgets); an acknowledged client + hides `trafficSeen` (acknowledged reply). + ### Done - [ ] All PRs merged (or Phase 3 dropped at the gate, with the reason in