Skip to content

Add hs.audiodevice#37

Open
cmsj wants to merge 7 commits intomainfrom
hs.audiodevice
Open

Add hs.audiodevice#37
cmsj wants to merge 7 commits intomainfrom
hs.audiodevice

Conversation

@cmsj
Copy link
Copy Markdown
Owner

@cmsj cmsj commented Mar 24, 2026

  • Checkpoint
  • Fix some compiler warnings
  • Fix channelCount property
  • Fix docstrings issues and regenerate docs

@cmsj cmsj self-assigned this Mar 24, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 24, 2026

Greptile Summary

This PR introduces hs.audiodevice, a new Hammerspoon module that exposes CoreAudio device enumeration, property inspection, volume/mute/balance/sample-rate control, data-source selection, default-device assignment, and property-change watchers — both at the per-device level and at the system level — to JavaScript. The implementation follows the established module pattern (ModuleRoot registration, JSExport protocol, getOrCreate lifecycle) and includes a comprehensive integration test suite that gracefully degrades on CI runners with no audio hardware.

  • HSAudioDevice.swift — core device object with CoreAudio helper layer and a per-device watcher that uses AudioObjectAddPropertyListenerBlock.
  • HSAudioDeviceManager.swift — singleton weak-reference cache ensuring identity stability for the same AudioObjectID across JS calls.
  • HSAudioDeviceModule.swift — module entry point with system-level watcher for device add/remove and default-device changes.
  • One logic bug: in HSAudioDevice.startWatcher(), selfRetain = self is set unconditionally before the listener-registration loop. If every candidate property fails to register (e.g. on a device that exposes none of the watched selectors, or if all AudioObjectAddPropertyListenerBlock calls fail), watcherRegistrations stays empty, stopWatcher() short-circuits before clearing selfRetain, and the device leaks permanently.
  • Two style issues: both HSAudioDevice.deinit and HSAudioDeviceModule.deinit use raw print() rather than the project-standard AKTrace.

Confidence Score: 4/5

  • Safe to merge after fixing the selfRetain leak in startWatcher() and replacing the two debug print calls with AKTrace.
  • The module is well-structured and comprehensive, with CI-resilient tests and clean CoreAudio wrappers. The selfRetain issue is a real memory leak but only manifests in the unlikely edge case where a device has none of the seven watched properties (or all listener registrations fail) — normal audio devices will always have at least one. The print calls are a minor polish issue. Everything else looks solid.
  • Hammerspoon 2/Modules/hs.audiodevice/HSAudioDevice.swiftstartWatcher() self-retain placement.

Important Files Changed

Filename Overview
Hammerspoon 2/Modules/hs.audiodevice/HSAudioDevice.swift New file implementing the per-device JS API. Two issues: a memory leak in startWatcher() where selfRetain = self is set before confirming any listener was registered (if none register, stopWatcher() never clears it), and a leftover print in deinit that should use AKTrace.
Hammerspoon 2/Modules/hs.audiodevice/HSAudioDeviceManager.swift New file providing a shared weak-reference cache for HSAudioDevice instances. Clean implementation with correct weak-ref pattern, prune helper, and stopAllWatchers for module shutdown.
Hammerspoon 2/Modules/hs.audiodevice/HSAudioDeviceModule.swift New module entry point with system-level watcher and device enumeration. Minor: leftover print in deinit should use AKTrace. System watcher's selfRetain pattern is not used here (module lifetime is managed externally), so no leak concern.
Hammerspoon 2/Engine/ModuleRoot.swift Minimal change: adds audiodevice property to both the protocol and the implementation, consistent with the existing pattern.
Hammerspoon 2Tests/IntegrationTests/HSAudioDeviceIntegrationTests.swift Comprehensive integration tests with CI-safe guards that skip device-dependent tests when no audio hardware is present. Good coverage of enumeration, identity, channels, volume, data sources, and watcher lifecycle.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.audiodevice/HSAudioDevice.swift
Line: 373-375

Comment:
**Debug `print` statement in `deinit`**

This `print(...)` call looks like a leftover debug trace. Other lifecycle logs in the module use `AKTrace`, and shipping production code with `print` to stdout is generally noisy. Consider replacing with `AKTrace` or removing it entirely.

```suggestion
    deinit {
        AKTrace("deinit of HSAudioDevice id=\(objectID)")
    }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.audiodevice/HSAudioDevice.swift
Line: 590-618

Comment:
**`selfRetain` set before confirming any listener was registered**

`selfRetain = self` is assigned unconditionally at the top of `startWatcher()`, before the loop that adds CoreAudio property listeners. If every candidate either lacks the property or fails `AudioObjectAddPropertyListenerBlock`, `watcherRegistrations` remains empty. At that point:

- `watcherIsActive()` returns `false` (correct)
- `stopWatcher()` early-returns immediately because `watcherRegistrations.isEmpty` is true — so it never clears `selfRetain`

The device therefore retains itself permanently, causing a memory leak. The fix is to move `selfRetain = self` to after the loop, gated on at least one successful registration:

```swift
@objc func startWatcher() {
    guard unsafe watcherRegistrations.isEmpty else { return }

    let candidates: [(AudioObjectPropertySelector, AudioObjectPropertyScope)] = [ ... ]
    for (selector, scope) in candidates {
        // ... same as before
    }

    guard !watcherRegistrations.isEmpty else { return }
    selfRetain = self  // prevent GC while watcher is active
    AKTrace(...)
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.audiodevice/HSAudioDeviceModule.swift
Line: 110-112

Comment:
**Debug `print` statement in `deinit`**

Same pattern as in `HSAudioDevice` — this `print(...)` should use `AKTrace` (consistent with the rest of the module) or be removed before merge.

```suggestion
    deinit {
        AKTrace("Deinit of \(name)")
    }
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Fix docstrings issues and regenerate doc..." | Re-trigger Greptile

Comment on lines +373 to +375

// MARK: - Capabilities

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Debug print statement in deinit

This print(...) call looks like a leftover debug trace. Other lifecycle logs in the module use AKTrace, and shipping production code with print to stdout is generally noisy. Consider replacing with AKTrace or removing it entirely.

Suggested change
// MARK: - Capabilities
deinit {
AKTrace("deinit of HSAudioDevice id=\(objectID)")
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.audiodevice/HSAudioDevice.swift
Line: 373-375

Comment:
**Debug `print` statement in `deinit`**

This `print(...)` call looks like a leftover debug trace. Other lifecycle logs in the module use `AKTrace`, and shipping production code with `print` to stdout is generally noisy. Consider replacing with `AKTrace` or removing it entirely.

```suggestion
    deinit {
        AKTrace("deinit of HSAudioDevice id=\(objectID)")
    }
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +590 to +618

AKTrace(unsafe "HSAudioDevice id=\(objectID): watcher started (\(watcherRegistrations.count) listeners)")
}

@objc func stopWatcher() {
guard unsafe !watcherRegistrations.isEmpty else { return }
for unsafe var registration in unsafe watcherRegistrations {
unsafe AudioObjectRemovePropertyListenerBlock(objectID, &registration.address, .main, registration.block)
}
unsafe watcherRegistrations.removeAll()
selfRetain = nil // allow GC if no other strong refs
AKTrace("HSAudioDevice id=\(objectID): watcher stopped")
}

@objc func watcherIsActive() -> Bool { unsafe !watcherRegistrations.isEmpty }

/// Called on the main thread from the watcher block.
private func handleDevicePropertyChange(addresses: [AudioObjectPropertyAddress]) {
guard let callback = watcherCallback, callback.isObject else { return }
for address in addresses {
let event: String
switch (address.mSelector, address.mScope) {
case (kAudioDevicePropertyVolumeScalar, kAudioDevicePropertyScopeOutput): event = "vmout"
case (kAudioDevicePropertyVolumeScalar, kAudioDevicePropertyScopeInput): event = "vmin"
case (kAudioDevicePropertyMute, kAudioDevicePropertyScopeOutput): event = "mout"
case (kAudioDevicePropertyMute, kAudioDevicePropertyScopeInput): event = "min"
case (kAudioDevicePropertyNominalSampleRate, _): event = "rate"
case (kAudioDevicePropertyDataSource, kAudioDevicePropertyScopeOutput): event = "dsout"
case (kAudioDevicePropertyDataSource, kAudioDevicePropertyScopeInput): event = "dsin"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 selfRetain set before confirming any listener was registered

selfRetain = self is assigned unconditionally at the top of startWatcher(), before the loop that adds CoreAudio property listeners. If every candidate either lacks the property or fails AudioObjectAddPropertyListenerBlock, watcherRegistrations remains empty. At that point:

  • watcherIsActive() returns false (correct)
  • stopWatcher() early-returns immediately because watcherRegistrations.isEmpty is true — so it never clears selfRetain

The device therefore retains itself permanently, causing a memory leak. The fix is to move selfRetain = self to after the loop, gated on at least one successful registration:

@objc func startWatcher() {
    guard unsafe watcherRegistrations.isEmpty else { return }

    let candidates: [(AudioObjectPropertySelector, AudioObjectPropertyScope)] = [ ... ]
    for (selector, scope) in candidates {
        // ... same as before
    }

    guard !watcherRegistrations.isEmpty else { return }
    selfRetain = self  // prevent GC while watcher is active
    AKTrace(...)
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.audiodevice/HSAudioDevice.swift
Line: 590-618

Comment:
**`selfRetain` set before confirming any listener was registered**

`selfRetain = self` is assigned unconditionally at the top of `startWatcher()`, before the loop that adds CoreAudio property listeners. If every candidate either lacks the property or fails `AudioObjectAddPropertyListenerBlock`, `watcherRegistrations` remains empty. At that point:

- `watcherIsActive()` returns `false` (correct)
- `stopWatcher()` early-returns immediately because `watcherRegistrations.isEmpty` is true — so it never clears `selfRetain`

The device therefore retains itself permanently, causing a memory leak. The fix is to move `selfRetain = self` to after the loop, gated on at least one successful registration:

```swift
@objc func startWatcher() {
    guard unsafe watcherRegistrations.isEmpty else { return }

    let candidates: [(AudioObjectPropertySelector, AudioObjectPropertyScope)] = [ ... ]
    for (selector, scope) in candidates {
        // ... same as before
    }

    guard !watcherRegistrations.isEmpty else { return }
    selfRetain = self  // prevent GC while watcher is active
    AKTrace(...)
}
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +110 to +112

deinit {
print("Deinit of \(name)")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Debug print statement in deinit

Same pattern as in HSAudioDevice — this print(...) should use AKTrace (consistent with the rest of the module) or be removed before merge.

Suggested change
deinit {
print("Deinit of \(name)")
deinit {
AKTrace("Deinit of \(name)")
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.audiodevice/HSAudioDeviceModule.swift
Line: 110-112

Comment:
**Debug `print` statement in `deinit`**

Same pattern as in `HSAudioDevice` — this `print(...)` should use `AKTrace` (consistent with the rest of the module) or be removed before merge.

```suggestion
    deinit {
        AKTrace("Deinit of \(name)")
    }
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant