From 8aacd1f753c7ef21a7e1173cc08c5b0a85a94c6b Mon Sep 17 00:00:00 2001 From: Tommaso Negri Date: Wed, 14 Oct 2026 22:05:54 +0200 Subject: fix block interaction capture warning --- Sources/bar/Views/BlockView.swift | 78 +++++++++++++++++++++--------- Tests/barTests/BlockInteractionTests.swift | 48 ++++++++++++++++++ tasks/20261014-192419/TASK.md | 22 ++++++++- 3 files changed, 124 insertions(+), 24 deletions(-) create mode 100644 Tests/barTests/BlockInteractionTests.swift diff --git a/Sources/bar/Views/BlockView.swift b/Sources/bar/Views/BlockView.swift index 00d36aa..a97c776 100644 --- a/Sources/bar/Views/BlockView.swift +++ b/Sources/bar/Views/BlockView.swift @@ -3,41 +3,73 @@ import SwiftUI struct BlockView: View { @ObservedObject var block: Block - @State private var hoveredCell: Cell? - @State private var eventsMonitor: Any? + @State private var interaction = BlockInteraction() var body: some View { HStack(spacing: 0) { - ForEach(block.cells) { cell in - CellView(cell: cell) + ForEach(block.cells.indices, id: \.self) { index in + CellView(cell: block.cells[index]) .onHover { hovering in if hovering { - hoveredCell = cell - } else if hoveredCell?.id == cell.id { - hoveredCell = nil + interaction.hoveredIndex = index + } else if interaction.hoveredIndex == index { + interaction.hoveredIndex = nil } } } } - .onAppear { - let monitor = NSEvent.addLocalMonitorForEvents(matching: [ - .leftMouseDown, - .rightMouseDown, - .otherMouseDown, - .scrollWheel, - ]) { [weak block] event in - guard let block else { return event } - return self.handle(event: event, block: block) - } - self.eventsMonitor = monitor as Any + .onAppear { [weak block, weak interaction] in + guard let block else { return } + interaction?.start(block: block) + } + .onDisappear { [weak interaction] in + interaction?.stop() } - .onDisappear { - if let eventsMonitor { - NSEvent.removeMonitor(eventsMonitor) - self.eventsMonitor = nil + .onChange(of: block.cells.count) { _, count in + if let index = interaction.hoveredIndex, index >= count { + interaction.hoveredIndex = nil } } } +} + +/// Per-view state: the application-owned monitor must not retain its owner +/// or the block. Disappearance removes it; destruction is a fallback when +/// a hosting view is discarded without a disappearance callback. +@MainActor +final class BlockInteraction { + var hoveredIndex: Int? + private(set) var eventsMonitor: Any? + + func start(block: Block) { + guard eventsMonitor == nil else { return } + eventsMonitor = NSEvent.addLocalMonitorForEvents(matching: [ + .leftMouseDown, + .rightMouseDown, + .otherMouseDown, + .scrollWheel, + ]) { [weak block, weak self] event in + guard let block, let self else { return event } + return self.handle(event: event, block: block) + } + } + + func stop() { + if let eventsMonitor { + NSEvent.removeMonitor(eventsMonitor) + self.eventsMonitor = nil + } + hoveredIndex = nil + } + + isolated deinit { + if let eventsMonitor { NSEvent.removeMonitor(eventsMonitor) } + } + + func hoveredCell(in block: Block) -> Cell? { + guard let index = hoveredIndex, block.cells.indices.contains(index) else { return nil } + return block.cells[index] + } /// Processes an event when a cell is hovered, re-running the script /// with the appropriate environment variable and the hovered cell's @@ -45,7 +77,7 @@ struct BlockView: View { /// /// - Returns: The original event so the system continues normal delivery. private func handle(event: NSEvent, block: Block) -> NSEvent { - guard let cell = hoveredCell else { return event } + guard let cell = hoveredCell(in: block) else { return event } switch event.type { case .leftMouseDown, .rightMouseDown, .otherMouseDown: diff --git a/Tests/barTests/BlockInteractionTests.swift b/Tests/barTests/BlockInteractionTests.swift new file mode 100644 index 0000000..7ffc9d4 --- /dev/null +++ b/Tests/barTests/BlockInteractionTests.swift @@ -0,0 +1,48 @@ +import AppKit +import Testing +@testable import bar + +@Test @MainActor func monitorLifetime() { + let block = Block(name: "test", placement: .left, command: "") + var interaction: BlockInteraction? = BlockInteraction() + weak let owner = interaction + interaction?.start(block: block) + let first = interaction?.eventsMonitor as AnyObject? + #expect(first != nil) + interaction?.start(block: block) + #expect(first === (interaction?.eventsMonitor as AnyObject?)) + interaction?.hoveredIndex = 0 + interaction?.stop() + #expect(interaction?.eventsMonitor == nil) + #expect(interaction?.hoveredIndex == nil) + interaction?.stop() + interaction?.start(block: block) + #expect(interaction?.eventsMonitor != nil) + interaction = nil + #expect(owner == nil) +} + +@Test @MainActor func monitorDoesNotOwnBlock() { + let interaction = BlockInteraction() + var block: Block? = Block(name: "test", placement: .left, command: "") + weak let observedBlock = block + interaction.start(block: block!) + block = nil + #expect(observedBlock == nil) + interaction.stop() +} + +@Test @MainActor func hoverResolvesCurrentContent() { + let block = Block(name: "test", placement: .left, command: "") + let interaction = BlockInteraction() + block.cells = [Cell(rawValue: "old"), Cell(rawValue: "second")] + interaction.hoveredIndex = 0 + #expect(interaction.hoveredCell(in: block)?.rawValue == "old") + block.cells = [Cell(rawValue: "new"), Cell(rawValue: "new")] + #expect(interaction.hoveredCell(in: block)?.rawValue == "new") + interaction.hoveredIndex = 1 + block.cells = [Cell(rawValue: "remaining")] + #expect(interaction.hoveredCell(in: block) == nil) + interaction.stop() + #expect(interaction.hoveredCell(in: block) == nil) +} diff --git a/tasks/20261014-192419/TASK.md b/tasks/20261014-192419/TASK.md index c909da9..50f9919 100644 --- a/tasks/20261014-192419/TASK.md +++ b/tasks/20261014-192419/TASK.md @@ -1,6 +1,6 @@ # Resolve BlockView capture warning and clarify monitor lifetime -- STATUS: OPEN +- STATUS: CLOSED - PRIORITY: 190 - TAGS: review,quality,ui @@ -13,3 +13,23 @@ Approach: Inspect the actual closure ownership chain and choose explicit, intent Completion checks: A clean swift build/test has no warning from this capture. Click/scroll receives correct current hover data. Hide/show and any applied screen-rebuild patch do not accumulate monitors, duplicate command dispatch, or retain discarded views/windows. Manually verify lifecycle behavior before claiming leak prevention. Design constraint: Follow README.md: keep source-based configuration, no dependencies or plugin framework, and use the smallest readable change that makes the existing behavior reliable. This is a suggestion for individual review, not a deadline. Findings were recorded on 2026-10-14; re-check the current code before implementing. + +## Implementation and validation + +Implemented explicit weak captures through a per-view BlockInteraction owner. +Registration is idempotent; disappearance removes the monitor and clears hover, +and an isolated deinitializer removes any remaining monitor when its owner is +released. Rendering and hover use line positions, resolving CELL against current +block output and rejecting positions removed by a refresh. This supplies the +BlockView portion of task 20261014-191457 without changing Cell's public model. + +A clean swift test build passes the monitor registration/restart, weak owner and +block lifetime, and refreshed/removed hover regression tests. No capture warning +remains. The optional screen-hotplug patch is not applied (it targets the old +Models/AppDelegate.swift path). + +Status remains OPEN pending manual UI verification: repeat hide/show, click and +scroll after output refresh/shrink, check multiple windows and hover transitions, +and discard/rebuild hosting windows while checking monitor dispatch and window +release. Automated owner lifetime tests do not establish that SwiftUI releases +hosting state in every UI lifecycle path; leak prevention is not claimed. -- cgit v1.2.3