Skip to content

Commit 4bcbc78

Browse files
committed
Fix coordinate conversion and resource leaks from code review
Coordinate conversion (SimpleViewModel.swift): - Fixed multi-monitor coordinate conversion to use global maxY - Previous per-screen logic was incorrect and caused wrong Y positions - Now properly accounts for entire virtual screen space Resource management (SimpleHotkeyManager.swift): - Restored stopMonitoring() to MainActor with proper cleanup - Fixed stopMouseMonitoring() to dispatch cleanup to MainActor - Both methods now properly nil out monitor properties and callbacks - Prevents memory leaks from retained closures in singleton - Removed nonisolated(unsafe) annotations (no longer needed) These fixes address all issues identified in code review.
1 parent cbb0303 commit 4bcbc78

File tree

2 files changed

+31
-28
lines changed

2 files changed

+31
-28
lines changed

Sources/ClickIt/Lite/SimpleHotkeyManager.swift

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@ final class SimpleHotkeyManager {
1515

1616
// MARK: - Properties
1717

18-
nonisolated(unsafe) private var globalMonitor: Any?
19-
nonisolated(unsafe) private var localMonitor: Any?
18+
private var globalMonitor: Any?
19+
private var localMonitor: Any?
2020
private var onEmergencyStop: (() -> Void)?
2121

22-
nonisolated(unsafe) private var globalMouseMonitor: Any?
23-
nonisolated(unsafe) private var localMouseMonitor: Any?
22+
private var globalMouseMonitor: Any?
23+
private var localMouseMonitor: Any?
2424
private var onRightMouseClick: (() -> Void)?
2525
private var lastClickTime: TimeInterval = 0
2626
private let clickDebounceInterval: TimeInterval = 0.1
@@ -60,13 +60,16 @@ final class SimpleHotkeyManager {
6060
}
6161

6262
/// Stop monitoring
63-
nonisolated func stopMonitoring() {
63+
func stopMonitoring() {
6464
if let monitor = globalMonitor {
6565
NSEvent.removeMonitor(monitor)
66+
globalMonitor = nil
6667
}
6768
if let monitor = localMonitor {
6869
NSEvent.removeMonitor(monitor)
70+
localMonitor = nil
6971
}
72+
onEmergencyStop = nil
7073
}
7174

7275
/// Start monitoring for right mouse clicks (Live Mouse Mode)
@@ -91,11 +94,17 @@ final class SimpleHotkeyManager {
9194

9295
/// Stop mouse monitoring
9396
nonisolated func stopMouseMonitoring() {
94-
if let monitor = globalMouseMonitor {
95-
NSEvent.removeMonitor(monitor)
96-
}
97-
if let monitor = localMouseMonitor {
98-
NSEvent.removeMonitor(monitor)
97+
// Dispatch cleanup to the main actor to safely access and nil out properties.
98+
Task { @MainActor in
99+
if let monitor = self.globalMouseMonitor {
100+
NSEvent.removeMonitor(monitor)
101+
self.globalMouseMonitor = nil
102+
}
103+
if let monitor = self.localMouseMonitor {
104+
NSEvent.removeMonitor(monitor)
105+
self.localMouseMonitor = nil
106+
}
107+
self.onRightMouseClick = nil
99108
}
100109
}
101110

Sources/ClickIt/Lite/SimpleViewModel.swift

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -127,28 +127,22 @@ final class SimpleViewModel: ObservableObject {
127127

128128
private extension NSPoint {
129129
func asCGPoint() -> CGPoint {
130-
// Convert from AppKit coordinates (bottom-left origin) to CG coordinates (top-left origin)
131-
// Find the screen containing this point
132-
let screens = NSScreen.screens
133-
134-
// Find which screen contains this point
135-
for screen in screens {
136-
let frame = screen.frame
137-
if frame.contains(self) {
138-
// Convert using this screen's frame
139-
let screenY = frame.origin.y
140-
let screenHeight = frame.height
141-
return CGPoint(x: x, y: screenY + screenHeight - y)
142-
}
130+
// Convert from AppKit coordinates (bottom-left origin) to CG coordinates (top-left origin).
131+
// This must account for the entire virtual screen space in multi-monitor setups.
132+
133+
// The total height of the virtual screen space is the max Y coordinate across all screens.
134+
// The conversion is then to subtract the AppKit Y from this total height.
135+
if let globalMaxY = NSScreen.screens.map({ $0.frame.maxY }).max() {
136+
return CGPoint(x: x, y: globalMaxY - y)
143137
}
144138

145-
// Fallback: use the screen with the highest Y (topmost screen)
146-
// This handles the full desktop coordinate space
147-
if let maxY = screens.map({ $0.frame.maxY }).max() {
148-
return CGPoint(x: x, y: maxY - y)
139+
// As a fallback, use the main screen's height, which works for single-monitor setups.
140+
if let mainScreen = NSScreen.main {
141+
let screenHeight = mainScreen.frame.height
142+
return CGPoint(x: x, y: screenHeight - y)
149143
}
150144

151-
// Ultimate fallback
145+
// Ultimate fallback if no screen information is available.
152146
return CGPoint(x: x, y: y)
153147
}
154148
}

0 commit comments

Comments
 (0)